You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When a run file's benchmarks[] array contains two entries with the same benchmark name but different ids and different mv-params (e.g. two concurrent perftest engine pairs, one doing --verb=write on one NIC port and another doing --verb=write on a second port), the second entry's params are silently lost — both engine-id groups end up running with the same params instead of their own.
Discovered while validating a new bench-perftest benchmark (crucible PERFNFV-473) against real ConnectX-6 Lx hardware, trying to run two concurrent engine pairs.
Root cause (two compounding issues)
util/blockbreaker.py's get_mv_params() resolves a benchmark entry by name only:
next() always returns the first array entry matching name, regardless of which occurrence is actually being processed. rickshaw-run.py's _process_from_file() calls this once per occurrence (correctly counted via get_bench_ids()'s name:ids stream), but every call for the same name returns the same (first) entry's mv-params. It also wrote each occurrence's intermediate mv-params.json/bench-params.json files to the same name-keyed path, so occurrences silently overwrote each other's files on disk too.
Even with Update README.md #1 fixed, RunState.load_bench_params() flattens every occurrence's params into one shared per-iteration list with no indication of which engine-id group a given param belongs to. dump_params() already supports a per-param "id" field to scope a param to a specific engine id, but nothing was populating it automatically — so with two occurrences of the same name, every param from every occurrence applied to (or, depending on getopt parsing order in the benchmark script, silently overrode) every engine id sharing that benchmark name.
Fix
get_mv_params(input_json, name, index=0): select by array index (reusing the existing unused-for-this-path --index CLI flag) instead of a name-only search, with a sanity check that the entry at that index actually has the expected name.
rickshaw-run.py: track each occurrence's index, pass --index to blockbreaker.py, and key each occurrence's intermediate files by index (not just name) to avoid the overwrite collision.
RunState.load_bench_params(): auto-scope a benchmark instance's params to its own declared ids (expanded via a new shared expand_id_ranges() helper, "+"-joined for multi-id instances) by setting param["id"] — but only when the benchmark name is actually ambiguous (appears more than once in the run file), and only for params that don't already carry an explicit id, so the common single-instance case and any deliberate per-param override are both unaffected.
dump_params(): generalized the id filter to treat it as a `"+"-joined set of applicable engine ids (backward compatible with the pre-existing single-value case).
Verified live against real ConnectX-6 Lx hardware through the full crucible run pipeline (two concurrent perftest pairs on separate NIC ports, each correctly running its own params).
Testing
20 new unit tests added (72 total pass, nothing pre-existing broken):
tests/test_dump_params.py: id-scoping cases for dump_params()
tests/test_load_bench_params_duplicate_names.py (new): expand_id_ranges() plus mocked RunState.load_bench_params() covering single-instance (unaffected), duplicate-name auto-scoping, and explicit-id-override preservation
util/tests/test-blockbreaker.py + new fixture: get_mv_params() index-based resolution and get_bench_ids() occurrence listing
No crucible run/integration-level tests were added — all new coverage is at the unit level, consistent with the existing test suite's conventions (mocked toolbox, real temp files for I/O boundaries).
Fix implemented on branch fix-duplicate-benchmark-name-mv-params, ready for PR.
Summary
When a run file's
benchmarks[]array contains two entries with the same benchmark name but differentidsand differentmv-params(e.g. two concurrentperftestengine pairs, one doing--verb=writeon one NIC port and another doing--verb=writeon a second port), the second entry's params are silently lost — both engine-id groups end up running with the same params instead of their own.Discovered while validating a new
bench-perftestbenchmark (crucible PERFNFV-473) against real ConnectX-6 Lx hardware, trying to run two concurrent engine pairs.Root cause (two compounding issues)
util/blockbreaker.py'sget_mv_params()resolves a benchmark entry by name only:next()always returns the first array entry matchingname, regardless of which occurrence is actually being processed.rickshaw-run.py's_process_from_file()calls this once per occurrence (correctly counted viaget_bench_ids()'sname:idsstream), but every call for the same name returns the same (first) entry'smv-params. It also wrote each occurrence's intermediatemv-params.json/bench-params.jsonfiles to the same name-keyed path, so occurrences silently overwrote each other's files on disk too.Even with Update README.md #1 fixed,
RunState.load_bench_params()flattens every occurrence's params into one shared per-iteration list with no indication of which engine-id group a given param belongs to.dump_params()already supports a per-param"id"field to scope a param to a specific engine id, but nothing was populating it automatically — so with two occurrences of the same name, every param from every occurrence applied to (or, depending on getopt parsing order in the benchmark script, silently overrode) every engine id sharing that benchmark name.Fix
get_mv_params(input_json, name, index=0): select by array index (reusing the existing unused-for-this-path--indexCLI flag) instead of a name-only search, with a sanity check that the entry at that index actually has the expected name.rickshaw-run.py: track each occurrence's index, pass--indextoblockbreaker.py, and key each occurrence's intermediate files by index (not just name) to avoid the overwrite collision.RunState.load_bench_params(): auto-scope a benchmark instance's params to its own declaredids(expanded via a new sharedexpand_id_ranges()helper, "+"-joined for multi-id instances) by settingparam["id"]— but only when the benchmark name is actually ambiguous (appears more than once in the run file), and only for params that don't already carry an explicitid, so the common single-instance case and any deliberate per-param override are both unaffected.dump_params(): generalized theidfilter to treat it as a `"+"-joined set of applicable engine ids (backward compatible with the pre-existing single-value case).Verified live against real ConnectX-6 Lx hardware through the full
crucible runpipeline (two concurrentperftestpairs on separate NIC ports, each correctly running its own params).Testing
20 new unit tests added (72 total pass, nothing pre-existing broken):
tests/test_dump_params.py:id-scoping cases fordump_params()tests/test_load_bench_params_duplicate_names.py(new):expand_id_ranges()plus mockedRunState.load_bench_params()covering single-instance (unaffected), duplicate-name auto-scoping, and explicit-id-override preservationutil/tests/test-blockbreaker.py+ new fixture:get_mv_params()index-based resolution andget_bench_ids()occurrence listingNo
crucible run/integration-level tests were added — all new coverage is at the unit level, consistent with the existing test suite's conventions (mockedtoolbox, real temp files for I/O boundaries).Fix implemented on branch
fix-duplicate-benchmark-name-mv-params, ready for PR.AI-assisted-by: Claude Sonnet 5 noreply@anthropic.com