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.

Sign up for GitHub

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

Jump to bottom

[community][embeddings] infinity embedding local option #17671

Merged
merged 15 commits into from Feb 22, 2024
Merged

[community][embeddings] infinity embedding local option #17671

merged 15 commits into from Feb 22, 2024

Conversation

michaelfeil
Copy link
Contributor

@michaelfeil michaelfeil commented Feb 17, 2024 •
edited

Ready for review - drop-in-replacement for sentence-transformers inference.

#17670

tldr from the discussion above -> around a 4x-22x speedup over using SentenceTransformers / huggingface embeddings. For more info: https://github.com/michaelfeil/infinity (pure-python dependency)

Checklist:

  • [ x] PR message: Delete this entire template message and replace it with the following bulleted list
    • Description: a description of the change
    • Issue: the issue # it fixes, if applicable
    • Dependencies: infinity_emb, will raise a soft runtime error if N/A
    • Twitter handle: No twitter.
  • Pass lint and test: DONE that
  • Add tests and docs: If you're adding a new integration, please include
    1. a test for the integration, preferably unit tests that do not rely on network access, I ADDED A UNIT TEST
    2. an example notebook showing its use. It lives in docs/docs/integrations directory. I ADDED and tested a notebook.

Additional guidelines:

  • Make sure optional dependencies are imported within a function.
  • Please do not add dependencies to pyproject.toml files (even optional ones) unless they are required for unit tests.
  • Most PRs should not touch more than one package.
  • Changes should be backwards compatible.
  • If you are adding something to community, do not re-import it in langchain.

If no one reviews your PR within a few days, please @-mention one of baskaryan, efriis, eyurtsev, hwchase17.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Feb 17, 2024
@vercel Vercel
Copy link

vercel bot commented Feb 17, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchain ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 21, 2024 11:40pm

@dosubot dosubot bot added Ɑ: embeddings Related to text embedding models module 🤖:improvement Medium size change to existing code to handle new use-cases labels Feb 17, 2024
@michaelfeil michaelfeil mentioned this pull request Feb 17, 2024
Open
@michaelfeil
Copy link
Contributor Author

michaelfeil commented Feb 17, 2024 •
edited

Finally made it past the CI again!
Push for Pydatnic v2: (also made my package backwards compatible to pydantic v1, which might be a common error. pydantic v1 is not required to execute the code above, but the install of v1 makes the optional parts behave weird.)

@michaelfeil michaelfeil mentioned this pull request Feb 18, 2024
Merged
@michaelfeil michaelfeil mentioned this pull request Feb 20, 2024
Open
@michaelfeil
Copy link
Contributor Author

@baskaryan Looking forward to your review again.

baskaryan
baskaryan reviewed Feb 20, 2024
View reviewed changes
docs/docs/integrations/text_embedding/infinity_local.ipynb Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

would it make sense to put this in the existing Infinity notebook, may be easier to find that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, since they are so different, I prefer them in two notebooks. Is it possible with keeping them separate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, but then we should change the first header for this one (otherwise they will both show up as Infinity in the side bar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, consolidated into one notebook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baskaryan this is done, thanks ;)

baskaryan
baskaryan approved these changes Feb 20, 2024
View reviewed changes
Copy link
Collaborator

@baskaryan baskaryan left a comment

Choose a reason for hiding this comment

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

one comment, otherwise lgtm!

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Feb 20, 2024
@michaelfeil michaelfeil mentioned this pull request Feb 21, 2024
Open
@michaelfeil
Copy link
Contributor Author

@baskaryan Whats up with the vecel deployment, cant see this part of the CI, but it also should not be related to this PR (At most updated the notebook)

efriis
efriis approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

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

Notebook by mistake had some python code in a markdown cell! Caused docusaurus to read the import numpy as np statement as a wrong javascript import (because docusaurus is mdx)

Should be fixed now

@baskaryan baskaryan merged commit 242981b into langchain-ai:master Feb 22, 2024
65 of 67 checks passed
@michaelfeil michaelfeil deleted the infinity-local branch February 22, 2024 02:24
@michaelfeil
Copy link
Contributor Author

@efriis Thanks for the fix - that was an obvious one. Saw you're also biking in SF - feel free to message me at michaelfeil.eu, got to train for a half iron later in August.

k8si pushed a commit to Mozilla-Ocho/langchain that referenced this pull request Feb 22, 2024
@michaelfeil @efriis @k8si
community[minor]: infinity embedding local option (langchain-ai#17671)
7fd5729 
**drop-in-replacement for sentence-transformers
inference.**

langchain-ai#17670

tldr from the discussion above -> around a 4x-22x speedup over using
SentenceTransformers / huggingface embeddings. For more info:
https://github.com/michaelfeil/infinity (pure-python dependency)

---------

Co-authored-by: Erick Friis <erick@langchain.dev>
al1p pushed a commit to al1p/langchain that referenced this pull request Feb 27, 2024
@michaelfeil @efriis @al1p-R
community[minor]: infinity embedding local option (langchain-ai#17671)
6b1e41b 
**drop-in-replacement for sentence-transformers
inference.**

langchain-ai#17670

tldr from the discussion above -> around a 4x-22x speedup over using
SentenceTransformers / huggingface embeddings. For more info:
https://github.com/michaelfeil/infinity (pure-python dependency)

---------

Co-authored-by: Erick Friis <erick@langchain.dev>
haydeniw pushed a commit to haydeniw/langchain that referenced this pull request Feb 27, 2024
@michaelfeil @efriis
community[minor]: infinity embedding local option (langchain-ai#17671)
97303f0 
**drop-in-replacement for sentence-transformers
inference.**

langchain-ai#17670

tldr from the discussion above -> around a 4x-22x speedup over using
SentenceTransformers / huggingface embeddings. For more info:
https://github.com/michaelfeil/infinity (pure-python dependency)

---------

Co-authored-by: Erick Friis <erick@langchain.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baskaryan baskaryan baskaryan approved these changes

@efriis efriis efriis approved these changes

Assignees
No one assigned
Labels
Ɑ: embeddings Related to text embedding models module 🤖:improvement Medium size change to existing code to handle new use-cases lgtm PR looks good. Use to confirm that a PR is ready for merging. size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@michaelfeil @efriis @baskaryan