-
Notifications
You must be signed in to change notification settings - Fork 177
fixed super.init in SentenceTransformerPatched #617
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
base: main
Are you sure you want to change the base?
fixed super.init in SentenceTransformerPatched #617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.pyby 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
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| 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) |
Related Issue
The problem stems from the
__init__method of the parent class,SentenceTransformer. This method usesself.__class__.__name__to set a configuration value.When subclass
SentenceTransformerPatchedcallssuper().__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 baseSentenceTransformerclass, 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: