Skip to content

Conversation

@tomaarsen
Copy link
Member

@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
Copy link
Contributor

Copilot AI left a comment

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
Contributor

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
Member 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 huggingface: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