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

[CHORE] added support for together ai embeddings endpoint #1828

Closed
wants to merge 3 commits into from

Conversation

Govind-S-B
Copy link

@Govind-S-B Govind-S-B commented Mar 6, 2024

Description of changes

Added support for together ai embeddings endpoint which was not present in chromadb
Ref : https://docs.together.ai/docs/embeddings-rest

  • New functionality
    • together ai embedding endpoint support

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.

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation 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

Copy link

github-actions bot commented Mar 6, 2024

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Copy link
Contributor

@atroyn atroyn left a 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]

chromadb/utils/embedding_functions.py Outdated Show resolved Hide resolved
chromadb/utils/embedding_functions.py Show resolved Hide resolved
@Govind-S-B Govind-S-B changed the title added support for together ai embeddings endpoint [CHORE] added support for together ai embeddings endpoint Mar 11, 2024
@Govind-S-B
Copy link
Author

@atroyn resolved

@Govind-S-B Govind-S-B requested a review from atroyn March 11, 2024 06:14
@jeffchuber
Copy link
Contributor

@Govind-S-B thanks for adding this! in case i missed it - any desire to support this for JS as well?

@Govind-S-B
Copy link
Author

@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 Govind-S-B requested a review from jeffchuber April 10, 2024 02:48
@tazarov
Copy link
Contributor

tazarov commented Apr 10, 2024

@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.

@Govind-S-B
Copy link
Author

@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.

@tazarov
Copy link
Contributor

tazarov commented Apr 10, 2024

@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.

@jeffchuber jeffchuber mentioned this pull request Sep 15, 2024
@jeffchuber
Copy link
Contributor

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.

@jeffchuber jeffchuber closed this Sep 15, 2024
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.

4 participants