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] Generate IDs when not given in add #2699

Closed
wants to merge 58 commits into from
Closed

[ENH] Generate IDs when not given in add #2699

wants to merge 58 commits into from

Conversation

spikechroma
Copy link

@spikechroma spikechroma commented Aug 21, 2024 •
edited
Loading

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes
    • ...
  • New functionality
    • when a user uses add on an collection, they are no longer required to pass in an array of IDs. They will be automatically generated if not given on the server side.

Test plan

How are these changes tested?

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

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 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)

@github-actions GitHub Actions
Copy link

Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas

@spikechroma Graphite App
Copy link
Author

spikechroma commented Aug 21, 2024 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @spikechroma and the rest of your teammates on Graphite Graphite

@spikechroma spikechroma changed the title generate ids [ENH] Refactor code to extract out unpacking embeddings set from existing validation logic Aug 21, 2024
@spikechroma spikechroma marked this pull request as ready for review August 21, 2024 21:34
@spikechroma spikechroma changed the title [ENH] Refactor code to extract out unpacking embeddings set from existing validation logic [ENH] Generate IDs when not given in upsert and add Aug 21, 2024
@spikechroma spikechroma mentioned this pull request Aug 21, 2024
Closed
1 task
atroyn
atroyn reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@atroyn atroyn left a comment

Choose a reason for hiding this comment

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

We discussed offline about moving the ID generation down into the server logic, and whether that would mean having to move the validators further down.

I think the right thing is to move both down into the server; we may want to make the validation logic change before making this one. Let's discuss further what the right answer might be.

chromadb/api/types.py Outdated
@@ -292,6 +296,10 @@ def validate_ids(ids: IDs) -> IDs:
for id_ in ids:
if not isinstance(id_, str):
raise ValueError(f"Expected ID to be a str, got {id_}")

if len(id_) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

On the fence about this still.

Copy link
Author

Choose a reason for hiding this comment

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

we can discuss offline

Copy link
Contributor

Choose a reason for hiding this comment

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

The empty string is a valid identifier, and would still be unique - I have no idea why anyone would want to do this, but let's let them.

chromadb/api/models/CollectionCommon.py Outdated
@@ -181,7 +181,7 @@ def _unpack_embedding_set(

def _validate_embedding_set(
self,
ids: IDs,
ids: Optional[IDs],
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think ids can be Optional here, since validate_ids fails on None

Copy link
Author

Choose a reason for hiding this comment

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

true. however, if we are generating ids on the server level, ids should be allowed to be optional here

chromadb/api/models/Collection.py Outdated Show resolved Hide resolved
chromadb/api/models/AsyncCollection.py Outdated Show resolved Hide resolved
chromadb/api/models/AsyncCollection.py Outdated Show resolved Hide resolved
atroyn
atroyn reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@atroyn atroyn left a comment

Choose a reason for hiding this comment

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

We discussed offline about moving the ID generation down into the server logic, and whether that would mean having to move the validators further down.

I think the right thing is to move both down into the server; we may want to make the validation logic change before making this one. Let's discuss further what the right answer might be.

HammadB
HammadB reviewed Aug 22, 2024
View reviewed changes
chromadb/test/property/strategies.py Outdated
@@ -458,17 +458,26 @@ def recordsets(
num_unique_metadata: Optional[int] = None,
min_metadata_size: int = 0,
max_metadata_size: Optional[int] = None,
# ids can only be optional for add operations
for_add: bool = False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this parameter name is confusing and underspecified - can we clarify it or somehow make this less option-heavy?

HammadB
HammadB reviewed Aug 22, 2024
View reviewed changes
chromadb/test/test_api.py Outdated Show resolved Hide resolved
HammadB
HammadB reviewed Aug 22, 2024
View reviewed changes
chromadb/server/fastapi/__init__.py Outdated
@staticmethod
def _generate_ids_when_not_present(
ids: Optional[List[str]],
documents: Optional[List[Optional[str]]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: why do you need to pass docs/urs/embeddings into this, it makes more sense to me that this would take the value N that it needs

HammadB
HammadB requested changes Aug 22, 2024
View reviewed changes
Copy link
Collaborator

@HammadB HammadB left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if this change is partways (or if I have missed something), but as is this change can't be merged as its broken on local (not client/server or "single-node") chroma. Putting the id generation in server/fastapi is not the right way to get this to be uniform across local/deployed single node. We should put the change at the level of "ServerAPI" so that it runs at the conceptual server layer. I am happy to go over the code architecture tomorrow to make this clearer / discuss more.

I also think we need more testing against this functionality since its additive and doesn't break existing tests. The property test modifications are good, and I think basic point unit tests are warranted too.

@HammadB
Copy link
Collaborator

HammadB commented Aug 22, 2024 •
edited
Loading

This stack addresses #164 in part, where we agreed validation duplication is preferred.

cc @levand @jeffchuber

This was referenced Aug 22, 2024
Closed
Closed
@spikechroma spikechroma mentioned this pull request Aug 22, 2024
Closed
1 task
@spikechroma @atroyn
revert changes
8a6b13d
@spikechroma @atroyn
create a new func for ensuring record set consistency
4093400
@spikechroma @atroyn
fix broken tests
2fef1ec
@spikechroma @atroyn
update property tests to handle validation error
d127cef
@spikechroma @atroyn
fix test fail
658179f
@spikechroma @atroyn
fix broken tests
0f76054
@spikechroma @atroyn
revert changes
9fcab83
@spikechroma @atroyn
fix conflicts
3d8d8e9
@spikechroma @atroyn
update doc strings, error messages and ignore tags
3954c87
@spikechroma @atroyn
fix tests
ac9051c
@spikechroma @atroyn
fix tags and update ids handling logic in cross version test
b5baff1
@spikechroma @atroyn
update count records function
b6b9fe8
@spikechroma @atroyn
refactor
6013e3f
@spikechroma @atroyn
add additional validations
2d96f03
@spikechroma @atroyn
update logic for getting n items from record set state
a4d9f03
@spikechroma @atroyn
modify ann accuracy and count
da08646
@spikechroma @atroyn
fix error
2927c01
@spikechroma @atroyn
fix tag
e9837f1
@spikechroma @atroyn
fix tag
9060462
@spikechroma @atroyn
fix tag
5d66811
@spikechroma @atroyn
turn on can ids be empty for test add medium
f04bc38
@spikechroma @atroyn
update persist test to take in state record set
3215e7d
@spikechroma @atroyn
add id validations
33ecb52
@spikechroma @atroyn
add tests for upsert and update with none ids
90932a5
@spikechroma @atroyn
change func header
0d29496
@spikechroma @atroyn
change func header
d2dcb57
@spikechroma @atroyn
Cleanup & Organization
b726474
@atroyn atroyn force-pushed the spike/generate_ids_move_validation branch from cea408e to 4e27cd5 Compare October 2, 2024 22:45
@atroyn atroyn closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HammadB HammadB HammadB requested changes

@atroyn atroyn atroyn requested changes

@codetheweb codetheweb Awaiting requested review from codetheweb

Assignees

@atroyn atroyn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spikechroma @HammadB @atroyn @codetheweb