-
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
[ENH] Migrate away from MetadataReader and VectorReader #2942
[ENH] Migrate away from MetadataReader and VectorReader #2942
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @Sicheng-Pan and the rest of your teammates on Graphite |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
8feb481
to
c0087ce
Compare
e830211
to
ff4c9a9
Compare
c0087ce
to
453d40d
Compare
ff4c9a9
to
91f3865
Compare
453d40d
to
f288c56
Compare
91f3865
to
119250e
Compare
f288c56
to
28f5d22
Compare
@@ -142,7 +143,7 @@ def test_sync_threshold(settings: Settings) -> None: | |||
) | |||
|
|||
manager = system.instance(SegmentManager) | |||
segment = manager.get_segment(collection.id, VectorReader) | |||
segment = manager.get_segment(collection.id, LocalHnswSegment) |
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.
curious why this has to be a more specific type now?
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.
I'm deprecating the VectorReader
abstract class because it is only going to be used by local Chroma in the future. MetadataReader
is deprecated as well.
119250e
to
d1f91de
Compare
28f5d22
to
205a4e6
Compare
options=None, | ||
request_version_context=request_version_context, | ||
return self._executor.knn( | ||
KNNPlan( |
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.
Beautiful!
@@ -46,4 +47,4 @@ def trigger_vector_segments_max_seq_id_migration( | |||
|
|||
for collection_id in collection_ids_with_unmigrated_segments: | |||
# Loading the segment triggers the migration on init | |||
segment_manager.get_segment(UUID(collection_id), VectorReader) | |||
segment_manager.get_segment(UUID(collection_id), LocalHnswSegment) |
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.
This is wrong. The segment could be LocalPersistentHnswSegment.
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.
Update: the code is correct by coincidence. I'm going to rework on this PR a bit to re-introduce the VectorReader
and MetadataReader
scope = SegmentScope.METADATA | ||
elif type == VectorReader: | ||
elif type == LocalHnswSegment: |
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.
We are missing a type here - what about LocalPersistentHnswSegment?
This reverts commit f0019e7.
Description of changes
Summarize the changes made by this PR.
MetadataReader
andVectorReader
interfaceTest plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?