Skip to content

Fix #463: apply nested index_params and default-valued config keys#497

Merged
russfellows merged 4 commits into
mainfrom
devasena/issue-463-nested-index-params
Jun 23, 2026
Merged

Fix #463: apply nested index_params and default-valued config keys#497
russfellows merged 4 commits into
mainfrom
devasena/issue-463-nested-index-params

Conversation

@idevasena

Copy link
Copy Markdown
Contributor

Fix #463: vdb_benchmark config index params (nested + default-valued) not applied:

  1. Root cause 1 — nested index_params: is silently dropped. merge_config_with_args loops for section, params in config.items() then for key, value in params.items(), i.e. exactly two levels. When tuning params are nested as index: { index_params: { M, ef_construction } }, the inner loop sees key="index_params" with a dict value. index_params is not an argparse destination, so the whole block is skipped and M/ef_construction stay at their argparse defaults (16/200). That is precisely the log line in the issue: index_type updates to HNSW (it sits one level up, so it merges), but M/efConstruction revert to 16/200.

  2. Root cause 2 — config values equal to the argparse default are ignored. The merge only writes a config value when the arg is None or is_default[key] is True. It can't distinguish "user typed --M 16" from "config asked for 16," and a config value that happens to equal the default is treated as still-default and dropped. This bites flat configs too.

Note main's flat configs (M/ef_construction directly under index:) merge correctly today because they aren't nested — so the bug surfaces whenever someone uses the index_params: nesting shown in the issue, or sets a value matching a default.

Fix

In vdb_benchmark/vdbbench/config_loader.py:

  1. Added _flatten_config() that recursively lifts every leaf key to the top level, so index_params.M becomes M.
    Reworked precedence to: explicit CLI flag > YAML config > argparse default. A YAML value is now always applied unless the user explicitly set that flag on the CLI (detected via the existing args.is_default map). Unknown config keys (e.g. max_receive_message_length) are ignored exactly as before, and callers that consume those directly from the config dict are unaffected.

This single change fixes every caller (load_vdb, simple_bench, enhanced_bench, mpi_wrapper, compact_and_watch), since they all share this function. Added 8 regression tests in 1tests/tests/test_issue_463_nested_index_params.py`.

@idevasena idevasena requested a review from a team June 23, 2026 05:41
@github-actions

Copy link
Copy Markdown

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@ram-sangle

Copy link
Copy Markdown
Contributor

@idevasena the fix looks good to me, consider updating diskann indexing parameters in configs/vectordbbench/default.yaml and configs/vectordbbench/10m.yaml as well instead of using HNSW parameters.

  index_params:
    max_degree: 64
    search_list_size: 200

@ram-sangle ram-sangle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check any possibility of key collision as _flatten_config() walks all sections. If in future database.batch_size and dataset.batch_size both exist, the last value will be retained. Recursing within a specific section might work here.

@idevasena

Copy link
Copy Markdown
Contributor Author

@idevasena the fix looks good to me, consider updating diskann indexing parameters in configs/vectordbbench/default.yaml and configs/vectordbbench/10m.yaml as well instead of using HNSW parameters.

  index_params:
    max_degree: 64
    search_list_size: 200

@ram-sangle Good catch. Done. Both configs/vectordbbench/default.yaml``and configs/vectordbbench/10m.yamldeclared index_type: DISKANN but carried HNSW'sM/ef_constructionunderindex_params:— and since load_vdb readsmax_degree/search_list_size` for DISKANN, those keys were dead and the index was silently falling back to the argparse defaults. Replaced them with:

yamlindex_params:
  max_degree: 64
  search_list_size: 200

in both files. Added test_diskann_config_uses_diskann_params to confirm these propagate through merge_config_with_args and that the HNSW params stay untouched.

@idevasena

Copy link
Copy Markdown
Contributor Author

Please check any possibility of key collision as _flatten_config() walks all sections. If in future database.batch_size and dataset.batch_size both exist, the last value will be retained. Recursing within a specific section might work here.

Agreed, and this turned out to be live rather than hypothetical — configs/vectordbbench/default.yaml already has both dataset.batch_size: 10 and benchmark.batch_size: 1, and the global flatten was collapsing them last-wins, so the datagen insert batch size was silently becoming 1.

Reworked per your suggestion to flatten per section rather than across the whole document:

Cross-section duplicate leaves now keep the first occurrence in document order (so dataset wins over benchmark for the shared batch_size dest) and emit a WARNING so the duplication is visible.
An intra-section collision — e.g. a key set at the section level and again under index_params: — now raises ValueError instead of being silently resolved.

Added test_cross_section_same_leaf_does_not_collide and test_intra_section_index_params_collision_raises to lock this in.

@russfellows russfellows left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

@russfellows russfellows merged commit 491c5dc into main Jun 23, 2026
3 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vdb_benchmark : indexing parameter values from config files are not getting used

4 participants