MaartenGr
Fix/static embedding error
What does this PR do?
Fixes import error if sentence_transformers isn't v3.2.0 or higher
Fixes #2331
Before submitting
- [ ] This PR fixes a typo or improves the docs (if yes, ignore all other checks!).
- [x] Did you read the contributor guideline?
- [x] Was this discussed/approved via a Github issue? Please add a link to it if that's the case.
- [ ] Did you make sure to update the documentation with your changes (if applicable)?
- [ ] Did you write any new necessary tests?
Not sure whether it should be a ValueError or ImportError. Technically it's an ImportError but only because an invalid value was passes (model2vec=True)
That's an interesting question. I would also think it's closer to the parameter model2vec=True and as such would indeed consider ValueError over ImportError. In all honesty, they are both "correct" to me considering they both showcase an method of resolving the issue (using a different parameter ValueError versus upgrading sentence-transformers ImportError).
Other than that, it all looks good to me. Anything else to consider?
I am facing the same issue and maybe it would be clearer to increase the min. version of sentence-transformers?
"sentence-transformers>=0.4.1" -> "sentence-transformers>=3.2.0"
3.2.0 where the StaticEmbedding class was added to sentence-transformers.
I understand that it may bring in additional dependencies, but clearer to me than moving this import under a conditional branch.
@dschwalm Ah, I have a tendency to keep dependencies as they are. Sorry! Going with 0.4.1 is perhaps a bit of a stretch here. I'm actually quite alright here with simply upping the version. That would indeed solve all issues and considering sentence-transformers is already at >5.0 that seems like a good way forward.
Totally makes sense. In our case we needed to stick with 0.16.4 as our dependencies could not be resolved in a combination that allows sentence-transformers to be >=3.
@stevetracvc What do you think? Should we merely up sentence-transformers here? Especially with the many major updates it has had over the years I feel like we should look forward.
Sorry, lost track of this one. I generally just run my own forks anyway.
I don't upgrade packages very often, since it is an insane hassle to get everything I need into one environment (pytorch, tensorflow, cuML, and many others). I'm still using sentence-transformers 2.6.1 and bertopic 0.17.0. So we can close this and not use it if you want to push the requirement higher, but is there really a reason why it needed the higher version? I generally pre-run my embeddings since I store them in a Milvus server.
Upping the version helps maintain the stability of this package as well as prevent the need to maintain all older versions. I think upping the version here is the way to go, so this can indeed be closed.