-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[STACKED #1255] [ENH] Add multitenancy #1244
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
d21e6c1
to
dda4a61
Compare
47c73e9
to
ee2849b
Compare
dda4a61
to
2ae2b51
Compare
2ae2b51
to
99b8b75
Compare
99b8b75
to
340c956
Compare
340c956
to
0321fe8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for walking me through this
raise ValueError("Chroma API implementation must be set in settings") | ||
elif api_impl == "chromadb.api.segment.SegmentAPI": | ||
if settings.is_persistent: | ||
identifier = settings.persist_directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somewhat iffy on relying on these as unique id's; why not generate uuids? is it just so the user is able to uniquely address them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the path has to be a unique database, I don't see a way around that
## Description of changes *Summarize the changes made by this PR.* - Improvements & Bug fixes - Fixes a small bug that was introduced by a bad merge in #1244 . Get_coll should look at the metadata in the model, not from the input collection. - New functionality - None. ## Test plan *How are these changes tested?* These are test changes. - [x] Tests pass locally with `pytest` for python, `yarn test` for js ## Documentation Changes None required.
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
Unit Tests were added for new client tenant/database behavior
Property tests were added for the new tenant/database behavior by subclassing the collection state machine and switching the tenant/database as a state machine transition.
pytest
for python,yarn test
for jsDocumentation Changes
I will add a section to the docs about multitenancy and how to use it.
We can remove warnings about the client being a singleton.