Add optional model_revision to SentenceTransformer

[plynch-chwy]

Problem

The current SentenceTransformer class doesn't allow initialization with a specific model revision.

Related issue: #1373

Solution

This PR adds an optional parameter model_revision to the SentenceTransformer class that gets passed through to the sentence_transformers.util.snapshot_download call which already allows specifying a revision today and defaults to None as implemented here.

[plynch-chwy]

@nreimers any chance this could get reviewed to extend flexibility of fixing the model version to use?

[plynch-chwy]

@nreimers any chance you could take a look at this? Small change, no conflicts.

[yoko92]

@nreimers I would also appreciate this

[amine-mf]

+1
The review looks to be straight forward (I'm not a python/sf specialist I must admit)

[plynch-chwy]

Hey @nreimers, we have a couple people with the use case for adding this small change and at least one other person wanted it enough they created another PR (#1850).

I added revision as the last parameter so it should not break previous workflows that others have used with positional args. Please take a look and consider adding the change, thanks!

[amine-mf]

@plynch-chwy I just realised it has been a year since this was open, it seems like it's not happening. Are you using a workaround to handle this? (A fork maybe?)

[plynch-chwy]

@amine-mf yes this PR is my own fork of the repo. At the time I forked this repo and added a line in my requirements.txt file to pip install from my fork instead of pypi. You can add a line like this and pip will install your own fork.

git+https:/plynch-chwy/sentence-transformers@master

Even though I opened this a year ago... It doesn't look like there are any conflicts still with master, so you should be able to use this change on top of the latest changes to this repo.

[amine-mf]

@plynch-chwy That's what I thought, the things is, a fork will need to be maintained and kept up to date. I was hoping there was a different workaround (env variable, static setting....)
Thanks a lot, seems like no choice for now. But I'm curious as to why the pull request is being ignored, as I have seen a few merges since last year.

[plynch-chwy]

I asked around in the huggingface discord and was informed that the maintainer @nreimers went on to found another company and the development of this repo is effectively dead.

@amine-mf well this hasn't had a conflict show up in over a year so it should be hopefully as easy as doing a rebase once and a while to pull in new upstream changes. I am using fixed versions for other libraries I install from pypi, so this kinda works in the same way. I do wish we didn't have to go through this whole ceremony though of updating and I could just point to a pypi version with the chage.

[amine-mf]

@nreimers any chance you get to this PR soon? It's been here for a while and it seems that there's a few people waiting/hoping for it. We would all very much appreciate your effort.

[amine-mf]

I had a discussion with @nreimers and confirmed someone is taking over to maintain the repo. but the PR hasn't made any progress yet. From the recent commits, I'm guess it is you @tomaarsen ? Could you give this one a look?
🙏
@plynch-chwy Can you fix the conflict? 🙏

[tomaarsen]

@amine-mf Hello!

That's right! I've been going through the open PRs to pick out high priority ones. I'll bump this one up the queue for you.
The model loading has been refactored in #2345, so the conflicts are not trivial. Instead, I'll take over this PR and implement it.
Thanks for raising this!

• Tom Aarsen

[tomaarsen]

As mentioned in my previous comment, this has been superseded by #2419. The revision keyword argument will be included in the next release.
Thanks for your work on this PR @plynch-chwy, and thank you @yoko92 & @amine-mf for reminding me of this.

• Tom Aarsen