Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Fix breaking change in PyLate when loading modules #3110

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

tomaarsen
Copy link
Collaborator

@tomaarsen tomaarsen commented Dec 3, 2024

Resolves #3108

Hello!

Pull Request overview

  • Prevent crash when calling get_class_from_dynamic_module with incorrect class_ref.

Details

If the class_ref in _load_module_class_from_ref does not start with sentence_transformers., and it's not a remote module, but e.g. pylate's Dense, then get_class_from_dynamic_module will fail due to a, b = c.split(".") where c has more than 1 dot.

I think the cleanest solution is to ignore the ValueError - as otherwise we risk missing some edge cases if we e.g. expand on the if trust_remote_code ... before the get_class_from_dynamic_module call.

@NohTow I believe you already verified that this solution works, right?

  • Tom Aarsen

@tomaarsen tomaarsen requested a review from Copilot December 3, 2024 10:22

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (1)

sentence_transformers/SentenceTransformer.py:1563

  • The comment should be more specific. Suggestion: 'Ignore the error if 1) the file does not exist, or 2) the class_ref does not start with 'sentence_transformers.' or is otherwise incorrectly formatted.'
# Ignore the error if 1) the file does not exist, or 2) the class_ref is not correctly formatted/found
@NohTow
Copy link

NohTow commented Dec 3, 2024

Hello,
Yes, I can confirm that this solve the issue. I checked the results with exported jina-colbert-v2 and they are identical to the original model.
Thanks!

Small nitpick: the PR does not "prevent calling get_class_from_dynamic_module", it actually calls it but fallback to the other loading method when crashing.

@tomaarsen
Copy link
Collaborator Author

Small nitpick: the PR does not "prevent calling get_class_from_dynamic_module", it actually calls it but fallback to the other loading method when crashing.

Yes, let me update my phrasing. I meant "prevent calling ... from resulting in crash", but it wasn't clear:

Prevent calling get_class_from_dynamic_module with incorrect class_ref resulting in a crash.

  • Tom Aarsen

@tomaarsen tomaarsen merged commit e7641c8 into UKPLab:master Dec 3, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breaking change for module loading in PyLate
2 participants