feat: add search level to Milvus AutoIndex - #876
Conversation
Signed-off-by: YangYanbin <warlock.yyb@alibaba-inc.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: yanbinyang 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 |
|
/assign @XuanYang-cn Hi, when you have time, could you please take a look? The workflow is awaiting maintainer approval. Thanks! |
There was a problem hiding this comment.
Thanks for the contribution!
I think the idea is good, but if you have some free time, it will be really great if we can fix zilliz's autoindex together with this pr.
-
Unify the search-level entry point with Zilliz Cloud: make
zilliz_cloud/config.py'sAutoIndexConfiga thin subclass of the renewed MilvusAutoIndexConfigso both providers share onelevelfield and its 1–10 validation, while keeping Zilliz'slevel=1default / always-send behavior so existingzillizautoindexruns and result files are unchanged.ZillizCloudFtsConfigstays onMilvusFtsConfigwith its ownlevel: int = 1. This warrants a refactor but I will do it later. -
Share the CLI option as a TypedDict fragment so
zillizautoindex --levelgets the sameIntRange(1,10)validation (it is currentlytype=strwith a manualint()conversion). -
Add regression coverage for the omitted-level and
milvusflat-rejects---levelcontracts, plus out-of-range rejection. -
README placement nit + note that
zillizautoindexshares--level.
Thanks in advance!
|
|
||
| class AutoIndexConfig(MilvusIndexConfig, DBCaseConfig): | ||
| index: IndexType = IndexType.AUTOINDEX | ||
| level: int | None = Field(default=None, ge=1, le=10) |
There was a problem hiding this comment.
Parity suggestion: Zilliz Cloud already has its own AutoIndexConfig with a separate level: int = 1 (zilliz_cloud/config.py:30), and zillizautoindex --level is type=str with no range check. Consider making the Zilliz class a thin subclass of this one (class AutoIndexConfig(MilvusAutoIndexConfig)) so both providers share one level field + the 1–10 validation, keeping Zilliz's level=1 default and always-send search_param() so existing zillizautoindex runs/results are unchanged. Boundary: ZillizCloudFtsConfig must stay on MilvusFtsConfig (BM25/sparse machinery) with its own level: int = 1 — it must not inherit this dense config.
There was a problem hiding this comment.
Addressed in aac8019: Zilliz Cloud AutoIndexConfig now subclasses the Milvus AutoIndex config and shares the same constrained level type. Its default remains 1 and the inherited search_param still always sends it. ZillizCloudFtsConfig remains on MilvusFtsConfig.
|
|
||
| class MilvusAutoIndexTypedDict(CommonTypedDict, MilvusTypedDict): ... | ||
| class MilvusAutoIndexTypedDict(CommonTypedDict, MilvusTypedDict): | ||
| level: Annotated[ |
There was a problem hiding this comment.
Consider extracting this option into a shared TypedDict fragment (same pattern as HNSWFlavor3 / IVFFlatTypedDictN in vectordb_bench/cli/cli.py) so zillizautoindex reuses the same --level definition. Zilliz's current --level is type=str with a manual int() conversion — non-numeric input crashes with a traceback and out-of-range values pass through unvalidated — while click.IntRange(1, 10) rejects both cleanly.
There was a problem hiding this comment.
Addressed in aac8019: extracted a shared AutoIndexLevelTypedDict with click.IntRange(1, 10). Both milvusautoindex and zillizautoindex reuse it, and the Zilliz manual string-to-int conversion has been removed.
| ) | ||
|
|
||
| assert result.exit_code == 0, result.output | ||
| assert captured["db_case_config"].level == 2 |
There was a problem hiding this comment.
Set-path coverage only. The PR's two backward-compat claims are untested: (1) omitted --level → level is None and search_param() has no params key (server default preserved); (2) milvusflat rejects --level (the stated reason for the MilvusFlatTypedDict split). Suggest sibling assertions for the omitted case, the MilvusFlat --level rejection, and out-of-range --level 0 / 11.
There was a problem hiding this comment.
Addressed in aac8019: added focused coverage for omitted Milvus level/no params, milvusflat rejecting --level, and Milvus rejecting out-of-range values 0 and 11.
| <other options> | ||
| ``` | ||
|
|
||
| `milvusautoindex` accepts `--level` from 1 to 10 (YAML: `level`) on servers that |
There was a problem hiding this comment.
Minor: this paragraph sits inside the --note-file discussion right after a zillizautoindex example. Suggest moving it next to a milvusautoindex example, and (if the Zilliz subclass suggestion lands) noting that zillizautoindex shares the same --level (1–10).
There was a problem hiding this comment.
Addressed in aac8019: moved the level documentation next to a milvusautoindex example and documented the shared 1-10 range plus the different omitted-value behavior for Milvus and Zilliz Cloud.
Reuse the validated AutoIndex level contract across Milvus and Zilliz Cloud, add focused compatibility regression coverage, and clarify the CLI defaults in the README. Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-6-astra AI-Contributed/Feature: 84/84 AI-Contributed/UT: 44/44 Signed-off-by: YangYanbin <warlock.yyb@alibaba-inc.com>
9b7aa59 to
aac8019
Compare
|
Hi @jamesgao-jpg , I’ve addressed all review comments in aac8019 and replied inline. When convenient, could you please take another look? |
|
/lgtm |
…ge-cli Resolve conflicts with PR zilliztech#876 (feat: add search level to Milvus AutoIndex): - vectordb_bench/backend/clients/milvus/config.py: combine imports — keep Field from zilliztech#876 and field_validator from zilliztech#871. - vectordb_bench/backend/clients/milvus/cli.py: MilvusAutoIndex now uses _apply_milvus_case_defaults(AutoIndexConfig(level=parameters["level"]), parameters) so both the search level and the force-merge defaults apply. - tests/test_milvus_zilliz_cli.py: keep both the force-merge tests and the search-level tests. Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>
3 participants
Fixes #875. Milvus AutoIndex supports an optional search level on compatible deployments, but VectorDBBench did not expose it through its configuration or CLI. As a result, CLI and YAML runs could not reproduce or record level-specific AutoIndex searches.
I added an optional
levelfield toAutoIndexConfigand exposed it throughmilvusautoindexas--level, orlevelin YAML, with validation from 1 through 10. The search parameter includes the level only when it is configured, so omitting the option preserves the server default and index creation remains unchanged. The FLAT command now uses a separate option type somilvusflatdoes not accept an AutoIndex-only parameter.Added a CLI unit test that verifies the configured level reaches the Milvus AutoIndex case configuration. A live search-only run on a level-capable Milvus deployment also reproduced the previous recall across all eight Cohere and BioASQ cases at 1M and 10M, each with K=10 and K=100.