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.

Sign up for GitHub

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

Jump to bottom

[ENH] JS Client Refactor (Full Stack) #2542

Merged
merged 14 commits into from
Jul 30, 2024
Merged

[ENH] JS Client Refactor (Full Stack) #2542

merged 14 commits into from
Jul 30, 2024

Conversation

AlabasterAxe
Copy link
Contributor

This PR is the full set of changes for the JS Refactoring work. See #2384 for the full context for this effort.

@github-actions 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 (Readability, Modularity, Intuitiveness)

@AlabasterAxe AlabasterAxe mentioned this pull request Jul 19, 2024
Closed
2 tasks
codetheweb
codetheweb reviewed Jul 23, 2024
View reviewed changes
docs/docs.trychroma.com/pages/integrations/google-gemini.md
@@ -38,18 +38,26 @@ For more info - please visit the [official Google python docs](https://ai.google
{% /tab %}
{% tab label="Javascript" %}

This embedding function relies on the `@google/generative-ai` npm package, which you can install with `yarn add @google/generative-ai`.
This embedding function relies on the `@google/generative-ai` npm package, which you can install with e.g. `npm install @google/generative-ai`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This embedding function relies on the `@google/generative-ai` npm package, which you can install with e.g. `npm install @google/generative-ai`.
This embedding function relies on the `@google/generative-ai` npm package, which you can install with `npm install @google/generative-ai`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should leave e.g. here just to make it clear that this is an example of one way they can install the dependency rather than the way that we're specifically advocating.

clients/js/src/ChromaClient.ts
async countCollections(): Promise<number> {
await this.init();

return (await this.api.countCollections(
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why this isn't already typed as a number?

@codetheweb
Copy link
Contributor

looks pretty good to me after the merge conflicts are addressed

@tazarov
Copy link
Contributor

tazarov commented Jul 24, 2024

Adding some refs from the discord chats.

AlabasterAxe
AlabasterAxe commented Jul 24, 2024
View reviewed changes
docs/docs.trychroma.com/pages/deployment/migration.md Outdated
@@ -20,6 +20,32 @@ We will aim to provide:

## Migration Log

### v0.6.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codetheweb @atroyn what version of the JS API client will this go out in? I originally put this in here as a placeholder.

I'm thinking maybe this should say something like:

"Javascript Client Refactor (v2.0.0) - July 2024"

Since this is a backward incompatible change and the latest version is 1.9.1. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

that sounds good to me

@AlabasterAxe
Copy link
Contributor Author

@atroyn @codetheweb, ping on this. I think the last open question is how we want to title the migration section and then we can merge this, I believe.

@atroyn atroyn requested review from atroyn and codetheweb July 28, 2024 21:56
@codetheweb
Copy link
Contributor

after the version number is updated in the docs (above comment) this looks good to me!

@AlabasterAxe
Copy link
Contributor Author

Done!

@codetheweb codetheweb merged commit cbc499a into chroma-core:main Jul 30, 2024
67 checks passed
@jeffchuber jeffchuber mentioned this pull request Aug 23, 2024
Merged
jeffchuber added a commit that referenced this pull request Sep 9, 2024
@jeffchuber
[ENH] reset back to previous JS api, keeping other improvements (#2714)
7be79ec 
## Description of changes

Upon further discussion - we decided we preferred the object-oriented JS
API given our design constraints.

This PR reverts the api to the previous design while keeping the other
improvements introduced in
#2542

## Test plan
Existing JS client test coverage

## Documentation Changes
*Are all docstrings for user-facing APIs updated if required? Do we need
to make documentation changes in the [docs
repository](https://github.com/chroma-core/docs)?*

Yes
spikechroma pushed a commit that referenced this pull request Sep 12, 2024
@jeffchuber @spikechroma
[ENH] reset back to previous JS api, keeping other improvements (#2714)
075fe0c 
## Description of changes

Upon further discussion - we decided we preferred the object-oriented JS
API given our design constraints.

This PR reverts the api to the previous design while keeping the other
improvements introduced in
#2542

## Test plan
Existing JS client test coverage

## Documentation Changes
*Are all docstrings for user-facing APIs updated if required? Do we need
to make documentation changes in the [docs
repository](https://github.com/chroma-core/docs)?*

Yes
spikechroma pushed a commit that referenced this pull request Sep 12, 2024
@jeffchuber @spikechroma
[ENH] reset back to previous JS api, keeping other improvements (#2714)
6dc2cc7 
Upon further discussion - we decided we preferred the object-oriented JS
API given our design constraints.

This PR reverts the api to the previous design while keeping the other
improvements introduced in
#2542

Existing JS client test coverage

*Are all docstrings for user-facing APIs updated if required? Do we need
to make documentation changes in the [docs
repository](https://github.com/chroma-core/docs)?*

Yes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codetheweb codetheweb codetheweb approved these changes

@atroyn atroyn Awaiting requested review from atroyn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AlabasterAxe @codetheweb @tazarov