Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
This comment has been minimized.
This comment has been minimized.
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Action required due to a critical cross-version API selection issue and a failing test mock with Rust bindings.
Status: Action Required | Risk: High
Issues Identified & Suggestions
- Ensure old versions use SegmentAPI or always apply override:
chromadb/test/property/test_cross_version_persist.py - Align test mocks with Rust bindings or keep SegmentAPI:
chromadb/test/test_chroma.py
Review Details
📁 7 files reviewed | 💬 2 comments
👍 / 👎 individual comments to help improve reviews for you
| chroma_api_impl="chromadb.api.rust.RustBindingsAPI" | ||
| if "CHROMA_RUST_BINDINGS_TEST_ONLY" in os.environ | ||
| else "chromadb.api.segment.SegmentAPI", | ||
| chroma_api_impl="chromadb.api.rust.RustBindingsAPI", |
There was a problem hiding this comment.
[Logic] configurations() now unconditionally uses chromadb.api.rust.RustBindingsAPI, but the runtime patch in persist_generated_data_with_old_version() (lines 187-192) that overrides to SegmentAPI for versions < 0.7.0 only fires when CHROMA_RUST_BINDINGS_TEST_ONLY is set. Without that env var (e.g., local development), old versions like 0.4.1 and 0.5.3 will receive RustBindingsAPI which they don't have, causing test failures.
Options:
- Set
chroma_api_implbased on the target version inconfigurations()(e.g.,SegmentAPIfor versions < 0.7.0). - Remove the
CHROMA_RUST_BINDINGS_TEST_ONLYguard from the runtime patch so it always overrides for old versions. - If the intent is that these tests only run with
CHROMA_RUST_BINDINGS_TEST_ONLYset (as CI does), document this requirement and/or add an early skip.
Context for Agents
`configurations()` now unconditionally uses `chromadb.api.rust.RustBindingsAPI`, but the runtime patch in `persist_generated_data_with_old_version()` (lines 187-192) that overrides to `SegmentAPI` for versions < 0.7.0 only fires when `CHROMA_RUST_BINDINGS_TEST_ONLY` is set. Without that env var (e.g., local development), old versions like 0.4.1 and 0.5.3 will receive `RustBindingsAPI` which they don't have, causing test failures.
Options:
1. Set `chroma_api_impl` based on the target version in `configurations()` (e.g., `SegmentAPI` for versions < 0.7.0).
2. Remove the `CHROMA_RUST_BINDINGS_TEST_ONLY` guard from the runtime patch so it always overrides for old versions.
3. If the intent is that these tests only run with `CHROMA_RUST_BINDINGS_TEST_ONLY` set (as CI does), document this requirement and/or add an early skip.
File: chromadb/test/property/test_cross_version_persist.py
Line: 133
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
|
Found 14 test failures on Blacksmith runners: Failures
|
There was a problem hiding this comment.
Minor maintainability concern noted around an unused test configuration variable.
Status: Minor Suggestions | Risk: Low
Issues Identified & Suggestions
- Remove or wire unused test config flag to avoid dead logic:
chromadb/test/property/test_persist.py
Review Details
📁 7 files reviewed | 💬 1 comments
👍 / 👎 individual comments to help improve reviews for you
|
|
||
| configurations = ( | ||
| [ | ||
| is_rust_bindings_mode = "CHROMA_RUST_BINDINGS_TEST_ONLY" in os.environ |
There was a problem hiding this comment.
[Maintainability] The new is_rust_bindings_mode variable is never used in this module, which makes the test configuration harder to reason about and can mask dead logic. Remove it or wire it into the configuration selection if it is intended to affect behavior.
| is_rust_bindings_mode = "CHROMA_RUST_BINDINGS_TEST_ONLY" in os.environ |
Context for Agents
The new `is_rust_bindings_mode` variable is never used in this module, which makes the test configuration harder to reason about and can mask dead logic. Remove it or wire it into the configuration selection if it is intended to affect behavior.
```suggestion
```
File: chromadb/test/property/test_persist.py
Line: 39
kylediaz
force-pushed
the
kylediaz03-23-_chore_drop_support_for_segmentapi
branch
from
fd6bec2 to
343c751
Compare
1 participant
Description of changes
Summarize the changes made by this PR.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustMigration plan
Are there any migrations, or any forwards/backwards compatibility changes needed in order to make sure this change deploys reliably?
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?