Skip to content

Conversation

mileszim
Copy link

@mileszim mileszim commented Oct 23, 2023

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • Adds CloudflareWorkersAIEmbeddingFunction to embedding_functions.py, which lets you use Cloudflare Workers AI embedding models in Chroma

Test plan

How are these changes tested?

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

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

@github-actions
Copy link

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 (Readbility, Modularity, Intuitiveness)

@HammadB
Copy link
Collaborator

HammadB commented Dec 4, 2023

Hey @mileszim this is marked as draft but is it ready to be reviewed + merged?

@mileszim
Copy link
Author

mileszim commented Dec 5, 2023

Hey @mileszim this is marked as draft but is it ready to be reviewed + merged?

Probably? I haven't had time to run through the checklist. Lemme know if there is anything specific to address / scaffolding for tests or something I can bring that in. Otherwise its up to your discretion.

@mileszim mileszim marked this pull request as ready for review December 5, 2023 01:13
@tazarov tazarov changed the title add CloudflareWorkersAIEmbeddingFunction to embedding_functions.py [ENH] CloudflareWorkersAIEmbeddingFunction Apr 5, 2024
@tazarov
Copy link
Contributor

tazarov commented Apr 5, 2024

@mileszim, I hope you don't mind if I do a little cleanup, testing + docs and JS EF in this PR

tazarov added a commit to chroma-core/docs that referenced this pull request Apr 6, 2024
- Also added a note about the max batch size.

Ref: chroma-core/chroma#1271
- Removed batching, the choice and control of batching and any failures arising from is better left to the users and handled outside the EF
- Added JS implementation of the EF
- Added tests for both python and JS EF
@tazarov
Copy link
Contributor

tazarov commented Apr 7, 2024

There is a Cloudflare SDK for node, but given the simplicity of the API, it is overkill to include it in our depths. We can update the EF should the API change - we have the tests for it.

@tazarov tazarov requested a review from jeffchuber April 7, 2024 18:22
@tazarov tazarov self-assigned this Jun 21, 2024
@tazarov tazarov added the EF Embedding Functions label Jun 21, 2024
@tazarov
Copy link
Contributor

tazarov commented Jun 21, 2024

@mileszim, thank you for the patience. We've refactored the EFs into separate files (#2034) so now we'll move ahead with all EF-related PRs.

I'll pick this one up and make all the necessary changes to merge it.

Copy link
Contributor

@tazarov tazarov left a comment

Choose a reason for hiding this comment

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

LGTM now

@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

EF Embedding Functions ready-to-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants