[ENH] JS Client Refactor Part 1 - Refactoring the Client
Description of changes
The changes in this PR are described in detail in the "Phase 1" section of this doc: https://docs.google.com/document/d/1DH9Gl5UQeuDr6vyGgoYUw0TkBxITgXIutZSU-ZYdP_E/edit#heading=h.wnaf3x8gqzzl
The TL;DR is that this PR consists of the following changes:
- turns Collection into a data object (with the exception of the embedding function) and flattens its methods into the top-level ChromaClient
- Improves the typing of the methods on the client so that we do a better job of handling single vs multiple cases.
- validates tenant and database on client startup
- removes some unnecessary code in the client that was just used to enable certain test assertions
This PR corresponds with this issue: https://github.com/chroma-core/chroma/issues/2333
This is the first PR in a series that comprise the whole client refactor:
Test plan
- [x] all unit tests are passing
Documentation Changes
- [x] will need to update docs as this is a breaking change with the previous API
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)
Adding @codetheweb for review; will cover this this week!
Initial review pass looks good, @codetheweb is reviewing docs currently.
@AlabasterAxe could we please split this up into a stack of PRs for easier in-depth review of each part?
Initial review pass looks good, @codetheweb is reviewing docs currently.
@AlabasterAxe could we please split this up into a stack of PRs for easier in-depth review of each part?
@atroyn I don't have access to Chroma's Graphite instance so I did this manually. Let me know if that works for you. Also this PR is in kind of a weird state. I don't have permission to push branches to the chroma repo directly so I started it from my personal chroma fork. That means the subsequent PRs are not considered as being against the chroma repo yet but I think that should be okay because once you all give the go ahead I can merge them in quick succession.
Also, it sounds like it may be good to sync about all of the behavior changes this PR includes because it sounds like I may have diverged a bit too much from the Python client. Let me know if you'd like to sync quickly and then I can make the final changes to this PR so it's ready for the final round of reviews.
@atroyn
I've recorded my thoughts about the various suggested changes to client behavior in these isues: https://github.com/chroma-core/chroma/issues/2483 (the queryTexts vs query issue from above) https://github.com/chroma-core/chroma/issues/2492 https://github.com/chroma-core/chroma/issues/2493 https://github.com/chroma-core/chroma/issues/2494
As discussed yesterday these are just for posterity/a jumping off point for larger conversations about client behavior as opposed to proposals that I'm advocating we implement in the near term. I still need to revert the behavior changes as discussed from yesterday though. Will ping on this PR when those changes are in.
@atroyn Okay, I've made the requested changes so these PRs should be ready for another look. Specifically:
- revert the merging of
queryTextsandqueryEmbeddinginto a singlequeryparam - revert the singular field naming
- revert the parameter-dependent return type (i.e. if user supplies a singular type, unwrap resulting array)
Also, I introduced a some style regressions along the way so I've just added a final formatting PR at the end of the stack.
@codetheweb validation PR here: https://github.com/chroma-core/chroma/pull/2537
This PR is rebased on top of that PR but I'm still using the chroma repo main as its base for this PR.
@codetheweb
I've propagated all of the PR feedback through the stack and put together the "Full stack" PR here: https://github.com/chroma-core/chroma/pull/2542
All PR checks are passing there so I'm thinking that as long as you approve of each of the PRs in the stack, we should be good to merge the Full Stack PR (even though e.g. this PR is failing checks) WDYT?
These changes were covered in another PR