-
Notifications
You must be signed in to change notification settings - Fork 2k
[ENH] Add eventual consistency to the frontend #5946
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
542b0a4 to
a4bcf3a
Compare
b46d7a5 to
bf3841d
Compare
|
Add Eventual Consistency Read Levels to Frontend and Clients Introduces a Key Changes• Defined Affected Areas• chromadb/api/types.py This summary was automatically generated by @propel-code-bot |
a4bcf3a to
c516e91
Compare
bf3841d to
b8dae44
Compare
| /// Specifies the read level for consistency vs performance tradeoffs. | ||
| /// Defaults to IndexAndWal (full consistency). | ||
| #[serde(default)] | ||
| pub read_level: ReadLevel, |
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.
thoughts on having this per SearchPayload v/s one level for the entire request?
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 did consider this. I kind of prefer it to be homogenous. Reason is that if you are making decisions off this data, you want a consistent read across all the queries.
|
Some tests are failing, lgtm with one question |
@sanketkedia Please see the comment in the PR about the test failure - `
|
## Description of changes _Summarize the changes made by this PR._ - Improvements & Bug fixes - / - New functionality - Adds read level to the clients. ## Test plan _How are these changes tested?_ E2E Tests that check searches succeed were added. - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Migration plan None required, the server handles both cases fwd/back. ## Observability plan Client changes don't entail this for now ## Documentation Changes Will update docs in a seperate PR
| match self.read_level { | ||
| ReadLevel::IndexOnly => { | ||
| // For IndexOnly queries, skip log fetching and use empty logs | ||
| tracing::info!("Skipping log fetch for IndexOnly read level"); |
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.
[Performance] Using tracing::info! here will generate a log line for every search query executed with IndexOnly. In a high-throughput scenario (which is the primary use case for IndexOnly), this will flood the logs.
Consider using tracing::debug! instead.
Context for Agents
Using `tracing::info!` here will generate a log line for every search query executed with `IndexOnly`. In a high-throughput scenario (which is the primary use case for `IndexOnly`), this will flood the logs.
Consider using `tracing::debug!` instead.
File: rust/worker/src/execution/orchestration/knn_filter.rs
Line: 302|
|
||
| # Just verify the API works and returns a valid response structure | ||
| assert results["ids"] is not None | ||
| assert len(results["ids"]) == 1 |
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.
[Logic] The assertion len(results["ids"]) == 1 contradicts the comment below it: "Results may be empty if data hasn't been compacted yet".
Since INDEX_ONLY skips the WAL and you just performed an add() (which writes to the WAL), the data might not be in the compacted segments yet, leading to 0 results. Asserting exactly 1 result makes this test flaky or incorrect for eventual consistency testing.
Consider asserting len(results["ids"]) >= 0 or ensuring compaction runs before this assertion if you strictly require the data to be visible.
Context for Agents
The assertion `len(results["ids"]) == 1` contradicts the comment below it: "Results may be empty if data hasn't been compacted yet".
Since `INDEX_ONLY` skips the WAL and you just performed an `add()` (which writes to the WAL), the data might not be in the compacted segments yet, leading to 0 results. Asserting exactly 1 result makes this test flaky or incorrect for eventual consistency testing.
Consider asserting `len(results["ids"]) >= 0` or ensuring compaction runs before this assertion if you strictly require the data to be visible.
File: chromadb/test/api/test_search_api.py
Line: 76
Description of changes
Summarize the changes made by this PR.
Test plan
Will be handled by e2e tests at top of stack
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
IndexAndWal will be the default everywhere
Observability plan
What is the plan to instrument and monitor this change?
Documentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?