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

Simplify embedding model support and loading #569

Merged
merged 2 commits into from
Jul 31, 2023
Merged

Conversation

joshdevins
Copy link
Member

We were attempting to load SentenceTransformers by looking at the model prefix, however SentenceTransformers can also be loaded from other orgs in the model hub, as well as from local disk. This prefix checking failed in those two cases. To simplify the loading logic and deciding which wrapper to use, we’ve removed support for text_embedding tasks to load a plain Transformer. We now only support DPR embedding models and SentenceTransformer embedding models. If you try to load a plain Transformer model, it will be loaded by SentenceTransformers and a mean pooling layer will automatically be added by the SentenceTransformer library. Since we no longer automatically support non-DPR and non-SentenceTransformers, we should include somewhere example code for how to load a custom model without DPR or SentenceTransformers.

Note: This change will allow us to support E5 embeddings uploaded with eland. This change will not yet solve adding the preamble instructions to query and index embeddings.

See: https://github.com/UKPLab/sentence-transformers/blob/v2.2.2/sentence_transformers/SentenceTransformer.py#L801

Resolves #531

@joshdevins joshdevins added bug Something isn't working enhancement New feature or request topic:NLP Issue or PR about NLP model support and eland_import_hub_model labels Jul 26, 2023
@joshdevins joshdevins requested review from davidkyle and tveasey July 26, 2023 09:19
@joshdevins joshdevins self-assigned this Jul 26, 2023
@joshdevins joshdevins force-pushed the joshdevins/sbert-e5 branch 2 times, most recently from 4208a28 to d3b74b5 Compare July 26, 2023 11:15
@joshdevins
Copy link
Member Author

Blog post to follow, draft WIP

We were attempting to load SentenceTransformers by looking at the model
prefix, however SentenceTransformers can also be loaded from other
orgs in the model hub, as well as from local disk. This prefix checking
failed in those two cases. To simplify the loading logic and deciding
which wrapper to use, we’ve removed support for text_embedding tasks to
load a plain Transformer. We now only support DPR embedding models and SentenceTransformer embedding models. If you try to load a plain
Transformer model, it will be loaded by SentenceTransformers and a mean
pooling layer will automatically be added by the SentenceTransformer
library. Since we no longer automatically support non-DPR and
non-SentenceTransformers, we should include somewhere example code for
how to load a custom model without DPR or SentenceTransformers. 

See: https://github.com/UKPLab/sentence-transformers/blob/v2.2.2/sentence_transformers/SentenceTransformer.py#L801

Resolves #531
@joshdevins joshdevins force-pushed the joshdevins/sbert-e5 branch from d3b74b5 to d285822 Compare July 26, 2023 13:09
This is mostly to force a rebuild of the PR workflow actions.
@joshdevins joshdevins closed this Jul 26, 2023
@joshdevins joshdevins reopened this Jul 26, 2023
@davidkyle
Copy link
Member

@picandocodigo the old Jenkins ci is still present in Eland PRs, you mentioned last week that it will go away once the infra repository is updated. Is there any progress?

#561 (comment)

The good news is that buildkite is working

@joshdevins
Copy link
Member Author

@picandocodigo I've also rebased after you added that "ignore" file, and I've tried opening a new PR but I get the same build error.

@picandocodigo
Copy link
Member

@davidkyle @joshdevins great that Buildkite is working now! I removed clients-ci from the requested checks in the branch protection rules, so it should be possible to merge without that passing. But there's some work still pending on infrastructure for this to be fully removed, I'll update you soon!

Copy link

@tveasey tveasey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I tested loading ST models from local disk and they were correctly identified as sentence transformer models

@joshdevins joshdevins merged commit f26fb8a into main Jul 31, 2023
@joshdevins joshdevins deleted the joshdevins/sbert-e5 branch July 31, 2023 16:18
@serenachou
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request topic:NLP Issue or PR about NLP model support and eland_import_hub_model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NLP] Add option for using the pooling layer in Text Embedding models
5 participants