-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] simplify logic for when to persist index changes #2539
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
# Fields were added after the initial implementation | ||
self.total_elements_updated = 0 | ||
self.total_invalid_operations = 0 | ||
self.__dict__.update(state) |
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 tested with pickle and it seems to be fine if you remove a field from a class def
@@ -132,9 +119,6 @@ def __init__(self, system: System, segment: Segment): | |||
else: | |||
self._persist_data = PersistentData( | |||
self._dimensionality, | |||
self._total_elements_added, |
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.
confirming this was not used anywhere else
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.
confirmed
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.
turns out this was the issue, I should have fully reasoned through it :/
self._total_elements_added
isn't used in local_persistent_hnsw
, but it is used in local_hnsw
(the super class) and there's some non-obvious contracts between the two
This reverts commit e93bacf.
This reverts commit afe8947.
This reverts commit e93bacf.
This reverts commit afe8947.
This reverts commit e93bacf.
This reverts commit afe8947.
This reverts commit e93bacf.
This reverts commit afe8947.
This reverts commit e93bacf.
This reverts commit afe8947.
This reverts commit e93bacf.
This reverts commit afe8947.
… fix) (#2545) History: - #2539 originally introduced some of the changes here - it introduced a bug not caught by our test suite due to limitations of our suite at the time - changes were reverted with #2544 - and now this PR adds a test that [was previously re-producing the bug](https://github.com/chroma-core/chroma/actions/runs/10012532835/job/27678256604?pr=2545) before it was fixed here Should not be merged until hnswlib version is bumped (bug around persisting a single item was recently fixed).
@HammadB and I discussed this. Makes it much easier to model the WAL behavior in tests while also simplifying the logic.