Repository navigation
[WIP] refactor: finalize db_case_config at a single choke point - #872
Open
jamesgao-jpg wants to merge 1 commit into
Open
jamesgao-jpg wants to merge 1 commit into
jamesgao-jpg wants to merge 1 commit into
Conversation
All post-construction transformations of case configs now flow through finalize_db_case_config() in vectordb_bench/backend/db_case_config.py: - CLI-only FTS routing (previously select_cli_db_case_config in cli/cli.py) and the dataset-derived metric_type default (previously applied by mutating task.db_case_config in assembler.py) are composed in one resolver. - Both task entry points call it: cli.run() (with the CLI parameters dict) and the web UI generate_tasks() (without). - The assembler no longer mutates the config; consumers treat the finalized config as read-only. - select_cli_db_case_config remains as a thin backward-compatible wrapper for existing callers and tests. No behavior change: FTS-routing semantics, the metric_type default rule, and both entry-point results are preserved (covered by the new resolver tests plus the existing FTS-routing tests). Note: this refactor moves the FTS-compatible field whitelist (including force_merge_target_size_mb) into the resolver, so it subsumes the one-line whitelist fix carried by PR zilliztech#871's cli/cli.py. Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jamesgao-jpg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Unifies every post-construction modification of
db_case_configinto asingle choke point, so a case config is produced in exactly one place and
only read afterwards.
New
vectordb_bench/backend/db_case_config.py:finalize_db_case_config(db, case_type, base_config, *, parameters, dataset)composes the two existing post-construction transforms:
select_cli_db_case_configincli/cli.py: vector config → backend FTS config with compatible-fieldcopy, plus BM25 overrides);
metric_type(previously applied by mutatingtask.db_case_configinassembler.py), now viamodel_copyso theinput config is never mutated.
cli.run()(with the CLI parameters dict)and the web UI
generate_tasks()(without). The assembler no longermutates the config;
select_cli_db_case_configremains as a thinbackward-compatible wrapper.
Why
db_case_configwas touched in four layers (CLI construction,run()FTS routing, assembler in-place mutation, task-runner/client reads), and the
CLI-only routing was skipped entirely on the web UI path. This made changes
to config handling easy to break and the two entry points could diverge.
Routing both entry points through one resolver (and deleting the in-place
mutation) removes that class of bug.
Behavior / compatibility
metric_typedefault rule,and both entry-point results are preserved. Covered by the new resolver
tests plus the existing FTS-routing tests, which now exercise the same
code through the compatibility wrapper.
(including
force_merge_target_size_mb) into the resolver, so it subsumesthe one-line whitelist fix in PR feat: add CLI control for Milvus force-merge size and toggle #871 (
cli/cli.py). When both merge, dropfeat: add CLI control for Milvus force-merge size and toggle #871's
cli/cli.pyhunk — the field already lives in the resolver.Testing (Level 2)
tests/test_db_case_config.py(8 new),test_fts_cli_user_control.py,test_oss_opensearch_fts.py,test_milvus.py,test_milvus_zilliz_cli.py→ 53 passed, 1 integration deselected.
ruff check→ All checks passed;black --check→ 230files unchanged (both match the main baseline).
Follow-up (separate PR)
frozen=Trueenforcement across all case-config classes (~55 root classes in44 backend config files), reworking the self-assigning cached-field methods
in
aws_opensearch/oss_opensearchparse_metric(),hologres, andpgvector, with a cross-backend regression sweep.