fixed super.init in SentenceTransformerPatched - #617
Open
raul3820 wants to merge 1 commit into
Open
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
PR Summary
Fixed initialization issue in SentenceTransformerPatched where super().init() caused incorrect configuration due to class name checks in parent class.
- Implements workaround in libs/infinity_emb/infinity_emb/transformer/embedder/sentence_transformer.py by creating temporary SentenceTransformer instance and copying state
- Resolves warning 'No sentence-transformers model found' by avoiding direct super() call that triggered name-based config checks
- Potentially introduces future maintenance challenges due to state copying approach
- Tests are currently incomplete due to poetry dependency installation issues
1 file reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
Sorry, something went wrong.
Comment on lines
+72
to
80
| temp_model = SentenceTransformer(**dict( | ||
| model_name_or_path=engine_args.model_name_or_path, | ||
| revision=engine_args.revision, | ||
| trust_remote_code=engine_args.trust_remote_code, | ||
| device=ls.device_placement, | ||
| model_kwargs=model_kwargs, | ||
| ) | ||
| )) | ||
| self.__dict__.update(temp_model.__dict__) | ||
| self.to(ls.device_placement) |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
style: This pattern of creating a temp instance and copying state bypasses normal inheritance. Consider adding comment explaining why dictionary update is safer than inheritance here
Suggested change
| temp_model = SentenceTransformer(**dict( | |
| model_name_or_path=engine_args.model_name_or_path, | |
| revision=engine_args.revision, | |
| trust_remote_code=engine_args.trust_remote_code, | |
| device=ls.device_placement, | |
| model_kwargs=model_kwargs, | |
| ) | |
| )) | |
| self.__dict__.update(temp_model.__dict__) | |
| self.to(ls.device_placement) | |
| # Create temporary model instance and copy its state to bypass SentenceTransformer's | |
| # __init__ which doesn't support our extended configuration. This allows us to | |
| # customize initialization while preserving all the model's internal state. | |
| temp_model = SentenceTransformer(**dict( | |
| model_name_or_path=engine_args.model_name_or_path, | |
| revision=engine_args.revision, | |
| trust_remote_code=engine_args.trust_remote_code, | |
| device=ls.device_placement, | |
| model_kwargs=model_kwargs, | |
| )) | |
| self.__dict__.update(temp_model.__dict__) | |
| self.to(ls.device_placement) |
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
The problem stems from the __init__ method of the parent class, SentenceTransformer. This method uses self.__class__.__name__ to set a configuration value.
When subclass SentenceTransformerPatched calls super().__init__(), self.__class__.__name__ evaluates to "SentenceTransformerPatched". The parent class's internal logic is not designed for this and expects the name to be "SentenceTransformer", which leads to incorrect config and problems like this:
The fix avoids calling super().__init__() directly. Instead, it creates a temporary instance of the base SentenceTransformer class, which initializes correctly. It then copies the state from this temporary object to the current instance (self), effectively bypassing the problematic check while still properly initializing the object.
Checklist
Additional Notes
I tried to run the poetry tests but got stuck here: