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

Upgrade ChromaDB to >=0.6.0 and fix broken tests #530

Conversation

smokestacklightnin
Copy link
Contributor

This partially addresses #523 by updating the ChromaDB version to the latest 0.6.1 and patching particular test cases where ChromaDB's behavior differs from other databases.

@smokestacklightnin smokestacklightnin linked an issue Jan 7, 2025 that may be closed by this pull request
@smokestacklightnin
Copy link
Contributor Author

@pmeier For this test failure, should I typecast the return value of self._client.list_collections() to list[str], or should I change the type annotation of the return value of that function?

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks William, LGTM. I took the liberty and fixed the remaining stuff myself. To answer your question: actually casting the object to the right type, i.e. list[str] is the best way to go here. The return type is Sequence[CollectionName] with isinstance(CollectionName, str). Both the custom container type as well as the custom string type are "implementation details" of Chroma that we don't need to expose to the Ragna user.

The last thing before merge is to add a warning to the docstring of Chroma explaining that the NE and and NOT_IN metadata filters also return sources that don't have the corresponding key set at all and this is different than all other builtin source storages.

@pmeier pmeier changed the title Upgrade ChromaDB to 0.6.1 and fix broken tests Upgrade ChromaDB to >=0.6.0 and fix broken tests Jan 8, 2025
@pmeier pmeier marked this pull request as ready for review January 8, 2025 13:16
@smokestacklightnin
Copy link
Contributor Author

@pmeier I believe this PR is ready to be merged whenever you are ready

Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Approving so you can self-merge after addressing the final comments and after #534 is merged and CI is green. Thanks William!

@smokestacklightnin smokestacklightnin merged commit 37923c5 into Quansight:main Jan 10, 2025
4 of 11 checks passed
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.

investigate chroma changed filtering behavior
2 participants