-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ENH]: LangchainJS Embedding Function #2945
base: main
Are you sure you want to change the base?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
…lled ( error TS2307: Cannot find module)
@@ -47,7 +47,7 @@ | |||
"dist" | |||
], | |||
"scripts": { | |||
"test": "jest --runInBand", | |||
"test": "NODE_OPTIONS=--experimental-vm-modules jest --runInBand", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why this option is needed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's JS weirdness for you. We have circular import here chroma -> langchain-chroma -> chroma, so when the tests run, unless ESM modules (which is enabled with the --experimenta-vm-modules
) are specified, we get an error from langchain-chroma
that chromadb
is not installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I don't think --experimental-vm-modules
is related to esm, do you have a link to the docs? and if there's a circular import is that something users will run into?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's the official Jest info on the use of the flag with ESM modules - https://jestjs.io/docs/ecmascript-modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, jest itself switches modes if the flag is enabled
will users run into this or is this limited to our tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My experiments showed that a third party project importing both LC and Chroma will not need to add the flag. However the test was a simple vanilla JS script, let me test with jest itself.
Description of changes
Addresses an issue raised as part of #2129
Summarize the changes made by this PR.
chromadb/utils/embedding_functions/chroma_langchain_embedding_function.py
Test plan
How are these changes tested?
yarn test
for jsDocumentation Changes
Added the new EF to embeddings integrations