Skip to content
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

Updated to work with the latest version of sentence transformers #112

Conversation

SilasMarvin
Copy link
Contributor

The latest version of sentence transformers changed the arguments to the _load_sbert_model and changed the download system. This is a small update to make it compatible.

@tomaarsen
Copy link
Contributor

cc @hongjin-su I haven't tested this myself, but this seems quite important to prevent the critical failures currently encountered when installing, e.g. #106.

For others, I would recommend the following:

pip install git+https://github.com/SilasMarvin/instructor-embedding.git@silas-update-for-newer-sentence-transformers

After which your code should work with the most recent Sentence Transformers version again.

  • Tom Aarsen

@BBC-Esq
Copy link
Contributor

BBC-Esq commented Apr 4, 2024

Hello @SilasMarvin

First, thanks for this fork and thanks to @tomaarsen for pointing it out. I'd been trying to get instructor models to work with sentence-transformers library ever since they started updating it in January after not issuing a new release in a few years. Have you gotten response from the maintainers of instructorembedding? I've found them to be unresponsive and/or unhelpful. Apparently, their library was created as part of some college project in Washington State (my home state BTW) and presumably they've moved onto real jobs. ;-)

Regardless of the reason, I've asked sentence-transformers to update their code instead and perhaps you could help? This is not my expertise... Here's a link to our discussion. Again, thanks for this fix...

UKPLab/sentence-transformers#2567

@SilasMarvin
Copy link
Contributor Author

Hey! @BBC-Esq I have not gotten any response from the maintainers. This honestly is not the best fix. The new version of sentence-transformers has some nice utilities for loading in files and managing different versions which I did not implement, but this should work as a hot fix for people having issues. I'll take a look at the discussion!

@BBC-Esq
Copy link
Contributor

BBC-Esq commented Apr 5, 2024

Hey! @BBC-Esq I have not gotten any response from the maintainers. This honestly is not the best fix. The new version of sentence-transformers has some nice utilities for loading in files and managing different versions which I did not implement, but this should work as a hot fix for people having issues. I'll take a look at the discussion!

Please do take a look, and thanks again. I think you'll understand my challenge.

@racinmat
Copy link
Contributor

This is great, I hope it could get merged.

@hongjin-su
Copy link
Collaborator

Thanks for all your help! I will merge it after conflicts are resolved.

@SilasMarvin SilasMarvin force-pushed the silas-update-for-newer-sentence-transformers branch from 5f59e5d to 989b34d Compare April 12, 2024 17:45
@SilasMarvin
Copy link
Contributor Author

Thanks for all your help! I will merge it after conflicts are resolved.

Awesome! Conflicts resolved

@hongjin-su hongjin-su merged commit 5cca65e into xlang-ai:main Apr 12, 2024
@mjsonofharry
Copy link

@hongjin-su will there be a new PyPi release soon?

Also for anyone who needs this fix in the meantime, you can pin this:

git+https://github.com/xlang-ai/instructor-embedding.git@5cca65eb0ed78ab354b086a5386fb2c528809caa

@BBC-Esq
Copy link
Contributor

BBC-Esq commented Apr 13, 2024

Please a new pypi release. These maintainers are slow to act.

@racinmat
Copy link
Contributor

@SilasMarvin I just realized one thing: the https://github.com/UKPLab/sentence-transformers/blob/66e0ee30843dd411c64f37f65447bb38c7bf857a/sentence_transformers/util.py#L544 from which you took the code includes the https://github.com/UKPLab/sentence-transformers/blob/66e0ee30843dd411c64f37f65447bb38c7bf857a/sentence_transformers/util.py#L552-L554 too which lets you use the model locally without the connection to the internet, is there any reason why it's not included or why you haven't used the load_dir_path directly?

@BBC-Esq
Copy link
Contributor

BBC-Esq commented Apr 13, 2024

Is this why when I specify a path on my computer it gives an error saying it's not a correct repository ID on huggingface or what not?

@BBC-Esq
Copy link
Contributor

BBC-Esq commented Apr 13, 2024

It's not working, so i did this pull request to try and make it work. TO BE CLEAR, however, I don't think this addresses @racinmat 's message:

Here's my pull request:

#113

@BBC-Esq
Copy link
Contributor

BBC-Esq commented Apr 13, 2024

My most recent pull request solves it

@racinmat
Copy link
Contributor

yes, exactly, that's why it complains about the repository ID, because it went straight to the validation without prior check for local drive

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.

7 participants