[ENH]: Config to enable quantization on tenants#6297
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Tenant-scoped quantization toggle with schema safeguards Introduces a frontend configuration flag ( Key Changes• Added Affected Areas• rust/types/src/collection_schema.rs This summary was automatically generated by @propel-code-bot |
0dfe648 to
afa75d2
Compare
68bcf60 to
920cbc1
Compare
| /// - The tenant_id is in the list | ||
| fn should_enable_quantization_for_tenant(&self, tenant_id: &str) -> bool { | ||
| self.tenants_with_quantization_enabled | ||
| .contains(&"*".to_string()) |
There was a problem hiding this comment.
nit: would prefer a separate config flag if possible
cb0819a to
6a1d588
Compare
920cbc1 to
bf56c81
Compare
| tenants_to_migrate_immediately_threshold: None, | ||
| enable_schema, | ||
| min_records_for_invocation: default_min_records_for_invocation(), | ||
| tenants_with_quantization_enabled: vec![], |
There was a problem hiding this comment.
[Logic] Hardcoding an empty list here will forcibly disable quantization for all local Python users (setting quantize=false in the schema), even if they explicitly enable it in their collection configuration. This makes it impossible to test or use the feature locally.
Consider defaulting to vec!["*".to_string()] for local bindings to allow usage, or exposing this as a configuration parameter.
Context for Agents
Hardcoding an empty list here will forcibly disable quantization for all local Python users (setting `quantize=false` in the schema), even if they explicitly enable it in their collection configuration. This makes it impossible to test or use the feature locally.
Consider defaulting to `vec!["*".to_string()]` for local bindings to allow usage, or exposing this as a configuration parameter.
File: rust/python_bindings/src/bindings.rs
Line: 132bf56c81 to
e73a5eb
Compare
6a1d588 to
45b01a9
Compare
e73a5eb to
927575d
Compare
927575d to
cbe97d7
Compare
| None | ||
| }; | ||
|
|
||
| // Enable quantization for tenants in the config list (or all tenants if "*" is present) | ||
| if let Some(ref mut schema) = reconciled_schema { | ||
| if let Some(spann_config) = schema.get_spann_config_mut() { | ||
| spann_config.quantize = self.should_enable_quantization_for_tenant(&tenant_id); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[Reliability] Enabling quantization now unconditionally flips schema.get_spann_config_mut().quantize and later forces SegmentType::QuantizedSpann (around line 597) whenever the tenant matches. However, we never verify that the active executor actually advertises support for quantized segments – the capability check above (if !supported_segment_types.contains(Spann) && !supported_segment_types.contains(QuantizedSpann)) only ensures one of those types exists. In environments where the backend only supports plain SPANN (which was fine before this PR), turning on the new knob will push an unsupported SegmentType::QuantizedSpann into sysdb, and the executor will fail the collection creation at runtime.
Before toggling quantize/selecting SegmentType::QuantizedSpann, guard on supported_segment_types.contains(&SegmentType::QuantizedSpann) (or return a clear error/fall back to non-quantized SPANN). For example:
| None | |
| }; | |
| // Enable quantization for tenants in the config list (or all tenants if "*" is present) | |
| if let Some(ref mut schema) = reconciled_schema { | |
| if let Some(spann_config) = schema.get_spann_config_mut() { | |
| spann_config.quantize = self.should_enable_quantization_for_tenant(&tenant_id); | |
| } | |
| } | |
| let enable_quantization = self.should_enable_quantization_for_tenant(&tenant_id); | |
| if enable_quantization && !supported_segment_types.contains(&SegmentType::QuantizedSpann) { | |
| return Err(CreateCollectionError::SpannNotImplemented); | |
| } | |
| if let Some(ref mut schema) = reconciled_schema { | |
| if let Some(spann_config) = schema.get_spann_config_mut() { | |
| spann_config.quantize = enable_quantization; | |
| } | |
| } |
This keeps legacy deployments (without quantized SPANN support) from attempting to create unsupported segments while still enabling the feature where it’s available.
Context for Agents
Enabling quantization now unconditionally flips `schema.get_spann_config_mut().quantize` and later forces `SegmentType::QuantizedSpann` (around line 597) whenever the tenant matches. However, we never verify that the active executor actually advertises support for quantized segments – the capability check above (`if !supported_segment_types.contains(Spann) && !supported_segment_types.contains(QuantizedSpann)`) only ensures *one* of those types exists. In environments where the backend only supports plain SPANN (which was fine before this PR), turning on the new knob will push an unsupported `SegmentType::QuantizedSpann` into sysdb, and the executor will fail the collection creation at runtime.
Before toggling `quantize`/selecting `SegmentType::QuantizedSpann`, guard on `supported_segment_types.contains(&SegmentType::QuantizedSpann)` (or return a clear error/fall back to non-quantized SPANN). For example:
```suggestion
let enable_quantization = self.should_enable_quantization_for_tenant(&tenant_id);
if enable_quantization && !supported_segment_types.contains(&SegmentType::QuantizedSpann) {
return Err(CreateCollectionError::SpannNotImplemented);
}
if let Some(ref mut schema) = reconciled_schema {
if let Some(spann_config) = schema.get_spann_config_mut() {
spann_config.quantize = enable_quantization;
}
}
```
This keeps legacy deployments (without quantized SPANN support) from attempting to create unsupported segments while still enabling the feature where it’s available.
File: rust/frontend/src/impls/service_based_frontend.rs
Line: 555
rust/types/src/collection_schema.rs
Outdated
| (None, None) => None, | ||
| (Some(default), Some(user)) => { | ||
| // Validate that quantize is always false (should only be set programmatically by frontend) | ||
| if user.quantize || default.quantize { |
There was a problem hiding this comment.
[Logic] The validation logic here (if user.quantize || default.quantize) combined with forcing quantize: false (line 1650) creates a critical issue for the read path.
When a collection is created, ServiceBasedFrontend::create_collection sets quantize = true in the schema and persists it to SysDB. When this collection is later retrieved via get_collection, reconcile_schema_for_read() calls reconcile_with_defaults(), which invokes this merge logic.
- The validation check will fail because the persisted schema (treated as
user) hasquantize: true, causingget_collectionto return an error. - Even if the validation passed, line 1650 forces
quantize: falsein the result, effectively disabling quantization in the loaded schema object, which creates a mismatch with the actual segments.
Fix:
- Remove the validation from
merge_spann_configs(or scope it only to user input validation phase). - Preserve the
quantizevalue from theuser(source) schema instead of hardcodingfalse.
Context for Agents
The validation logic here (`if user.quantize || default.quantize`) combined with forcing `quantize: false` (line 1650) creates a critical issue for the read path.
When a collection is created, `ServiceBasedFrontend::create_collection` sets `quantize = true` in the schema and persists it to SysDB. When this collection is later retrieved via `get_collection`, `reconcile_schema_for_read()` calls `reconcile_with_defaults()`, which invokes this merge logic.
1. The validation check will fail because the persisted schema (treated as `user`) has `quantize: true`, causing `get_collection` to return an error.
2. Even if the validation passed, line 1650 forces `quantize: false` in the result, effectively disabling quantization in the loaded schema object, which creates a mismatch with the actual segments.
**Fix:**
1. Remove the validation from `merge_spann_configs` (or scope it only to user input validation phase).
2. Preserve the `quantize` value from the `user` (source) schema instead of hardcoding `false`.
File: rust/types/src/collection_schema.rs
Line: 1624There was a problem hiding this comment.
reconcile_schema_for_read does not call reconcile_with_defaults

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
None
Observability plan
None
Documentation Changes
None