chroma icon indicating copy to clipboard operation
chroma copied to clipboard

[CHORE] added support for together ai embeddings endpoint

Open Govind-S-B opened this issue 1 year ago • 7 comments

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.

  • [x] 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 : https://github.com/chroma-core/docs/pull/223

Govind-S-B avatar Mar 06 '24 12:03 Govind-S-B

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)

github-actions[bot] avatar Mar 06 '24 12:03 github-actions[bot]

@atroyn resolved

Govind-S-B avatar Mar 11 '24 01:03 Govind-S-B

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

jeffchuber avatar Mar 13 '24 21:03 jeffchuber

@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 avatar Apr 10 '24 02:04 Govind-S-B

@Govind-S-B, we have this one that uses the official client - https://github.com/chroma-core/chroma/pull/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.

tazarov avatar Apr 10 '24 06:04 tazarov

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

Govind-S-B avatar Apr 10 '24 06:04 Govind-S-B

@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 - https://github.com/chroma-core/chroma/pull/1625#issuecomment-2046768324

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.

tazarov avatar Apr 10 '24 07:04 tazarov

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 avatar Sep 15 '24 23:09 jeffchuber