-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[CHORE] added support for together ai embeddings endpoint #1828
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
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.
A couple of changes requested for better usability. Separately some changes requested on the docs. Please also update your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]
@atroyn resolved |
@Govind-S-B thanks for adding this! in case i missed it - any desire to support this for JS as well? |
Might do one later but I dont plan to port them immediately. Planning on adding a few other endpoints as well later as I get some free time. |
@Govind-S-B, we have this one that uses the official client - #1625. So we're a fork here. I like the simplicity of using the REST API, but it comes at a maintenance cost down the line. |
Makes sense I personally hated adding any other package dependency and have them break out of nowhere when the official package introduces some backward incompatible feature and its not easily traceable as well most often. With the REST API I know where my ground truth is that I can refer with and not have to touch my python code usually. This is just a personal take and might be flawed so yeah the maintainers are the ultimate ones deciding which fits their philosophy. I just wanted support for together embedding endpoints lol. |
I appreciate your take. I, too, prefer simplicity, but sometimes simple != useful. Here is my take regarding the use of together AI - #1625 (comment) Overall I think we should go with the officially supported package. The apparent simplicity of this PR hides complexities down the road, for which I think the together AI team has a better understanding than anyone else. As for the dependency, those who want to use together AI wouldn't mind the extra dependency. |
Our underlying impl has changed and so this PR is not landable as is. That being said - we'd still like to add this functionality and that is now tracked in this issue. |
Description of changes
Added support for together ai embeddings endpoint which was not present in chromadb
Ref : https://docs.together.ai/docs/embeddings-rest
Test plan
How are these changes tested?
The change cannot be tested automatically since it requires API keys but I have tried importing them and running it locally on my machine with my API key.
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?
There is a high possibility that i did not update things properly so please review the doc : chroma-core/docs#223