feat(config): add v6.1 ruleset and pin RNG seeds from submission ruleset#389
feat(config): add v6.1 ruleset and pin RNG seeds from submission ruleset#389arekay-nv wants to merge 6 commits into
Conversation
Adds the MLPerf Inference v6.1 RoundRuleset (schedule/sample_index seeds from loadgen/mlperf.conf; per-model latency targets unchanged from v5.1) and makes it CURRENT. The registry now registers every known round by version, so both v5.1 and v6.1 are resolvable. When a config carries submission_ref, _resolve_and_validate now pins the runtime RNG seeds from the selected ruleset during construction — before the config is dumped to the report dir, so the persisted config.yaml, RuntimeSettings, and report all reflect the seeds that actually ran. The ruleset value wins over any user-supplied seed (mirrors LoadGen locking core seeds from user.conf). Lenient by design: an unregistered ruleset leaves the config unchanged, so placeholder/non-submission configs are unaffected. Duration/latency overrides and the runtime-path integration (surfacing ruleset latency targets in the report) are deferred to a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the MLPerf Inference v6.1 ruleset, including its official RNG seeds, and updates the default submission template to reference v6.1. It also implements a mechanism in BenchmarkConfig to automatically override and pin runtime RNG seeds based on the selected submission ruleset, ensuring compliance. Feedback is provided regarding the definition of _v6_1, where copying the mutable benchmark_rulesets dictionary instead of sharing its reference with _v5_1 is recommended to prevent potential side effects.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review
Reviewed by: Codex + Claude | Depth: thorough
Codex: no actionable correctness issues. Claude: 5 posted (1 shared-dict finding on rules.py:282 was dropped as a duplicate of the existing gemini-code-assist comment). See the summary comment below for the tiered breakdown.
| return | ||
|
|
||
| updates: dict[str, int] = {} | ||
| if ruleset.scheduler_rng_seed is not None: |
There was a problem hiding this comment.
[Claude] medium (testing): The two is not None seed branches and the if not updates: return early-exit are never exercised — both registered MLCommons rulesets set both seeds. ruleset_base.py documents None as a valid "unseeded randomization" value, so a partial ruleset (one seed None) or a no-op ruleset (both None) is a supported, reachable state. Add a test that registers a throwaway RoundRuleset(scheduler_rng_seed=None, sample_index_rng_seed=<int>) (and one with both None) and asserts only the set seed is pinned / the config is left unchanged.
There was a problem hiding this comment.
There is a semantic issue beyond the branch coverage discussed above: None currently means two different things. BenchmarkSuiteRuleset documents it as unseeded randomization, and RoundRuleset.apply_user_config passes it through to random.Random(None); this config path instead skips the override and retains the default/user seed, usually 42. The same ruleset therefore behaves differently depending on its construction path.
Suggested fix: preserve the existing contract by making the runtime/report seed fields nullable and always applying both ruleset values, including None:
updates = {
"scheduler_random_seed": ruleset.scheduler_rng_seed,
"dataloader_random_seed": ruleset.sample_index_rng_seed,
}If nullable seeds are not intended, the alternative is to remove None from the ruleset contract and reject such rulesets explicitly. Either way, the two runtime-construction paths should agree.
| """Only the round seeds rotate; per-model targets are identical.""" | ||
| v5_1 = get_ruleset("mlperf-inference-v5.1") | ||
| v6_1 = get_ruleset("mlperf-inference-v6.1") | ||
| assert v6_1.benchmark_rulesets == v5_1.benchmark_rulesets |
There was a problem hiding this comment.
[Claude] low (testing): v6_1.benchmark_rulesets == v5_1.benchmark_rulesets is identity-trivially true — _v6_1 is built with benchmark_rulesets=_v5_1.benchmark_rulesets (the same dict object), so this passes without actually verifying the per-model targets. (If the shared reference is replaced by a copy — see the existing suggestion on rules.py:282 — this becomes a real structural check.) Consider asserting a concrete expected value, e.g. a specific model's max_tpot_latency_ms, instead.
Review Council — Multi-AI Code ReviewReviewed by: Codex (gpt-5.5, xhigh) + Claude | Depth: thorough Codex: "changes consistently register the new MLCommons round and apply the selected ruleset seeds into runtime configuration without introducing an evident functional regression. No actionable correctness issues." Claude: 6 findings; 1 dropped as a duplicate of the existing No critical/high issues. The core logic (frozen-model mutation via 🟡 Should Fix (medium)
🔵 Consider (low)
Commit hygiene: 1 commit, 0 fixups — clean. |
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review (re-review of 346bd6b)
Reviewed by: Codex (gpt-5.5, xhigh) + Claude | Depth: thorough
Codex: no introduced correctness issues. Claude: 7 findings — 6 posted inline below, 1 summary-only (outside diff). The prior run's 6 findings are all resolved by 346bd6b. See the summary comment for the tiered breakdown.
| ValueError: If a ruleset is already registered under ``name`` (guards | ||
| against two rounds silently sharing a version string). | ||
| """ | ||
| if name in _RULESET_REGISTRY: |
There was a problem hiding this comment.
[Claude] low (api-contract): This guard makes the module non-idempotent — _auto_register_mlcommons() runs at import time, so a second call or an importlib.reload of this module (previously a silent overwrite) now raises ValueError mid-import. No current caller reloads, so nothing breaks today, but reload-safety is gone. If that matters, use dict.setdefault/an in-check for the auto-register path while keeping the raise for external callers.
| if not updates: | ||
| return | ||
|
|
||
| new_runtime = self.settings.runtime.model_copy(update=updates) |
There was a problem hiding this comment.
[Claude] low (error-handling): self.settings.runtime.model_copy(update=updates) — pydantic v2 model_copy(update=...) bypasses validation and does not verify that update keys are real fields (it writes straight into __dict__, even with extra="forbid"). The keys "scheduler_random_seed"/"dataloader_random_seed" are hardcoded strings decoupled from the RuntimeConfig field definitions; if either field is renamed, this silently attaches a junk attribute and leaves the real seed at its default with no error. Consider deriving the names from the model (or a shared constant) so a rename fails loudly.
There was a problem hiding this comment.
Follow-up after the field-name guard: model_copy(update=...) still bypasses value validation. I registered a ruleset with scheduler_rng_seed="oops" and obtained a successfully validated BenchmarkConfig whose runtime seed was a str.
Suggested fix: rebuild through Pydantic validation instead of an unvalidated copy:
new_runtime = type(self.settings.runtime).model_validate(
{**self.settings.runtime.model_dump(), **updates}
)With extra="forbid", this also catches unknown fields, making the manual key guard redundant while validating seed values. Please add an invalid-seed regression test.
| ruleset_registry._RULESET_REGISTRY.pop(name, None) | ||
|
|
||
| @pytest.mark.unit | ||
| def test_partial_none_seed_pins_only_set_seed(self, register_temp_ruleset): |
There was a problem hiding this comment.
[Claude] low (testing): Nice coverage of the scheduler-None (test_partial_none_seed_pins_only_set_seed) and both-None (test_all_none_seed_leaves_config_unchanged) cases. The mirror partial — scheduler_rng_seed set, sample_index_rng_seed=None — is never exercised, so the sample_index_rng_seed is not None False branch is only hit when updates is already empty. Consider adding that combination for symmetry.
Review Council — Multi-AI Code Review (re-review of
|
| # | File | Line | Category | Summary |
|---|---|---|---|---|
| 1 | config/rulesets/mlcommons/rules.py |
276 | documentation | Comment references docs/RANDOM_SEEDS.md, which is not tracked in the repo — a dangling ref that already misled a reviewer (violates AGENTS.md). Commit the doc or inline the rationale. |
| 2 | config/schema.py |
1046 | data-integrity | A type=SUBMISSION config with an unregistered submission_ref.ruleset silently falls back to default seeds (42/42) → non-compliant submission that looks valid. Make it a hard error for SUBMISSION. |
🔵 Consider (low)
| # | File | Line | Category | Summary |
|---|---|---|---|---|
| 3 | config/rulesets/mlcommons/rules.py |
283 | design | rules.copy() is a 2-level copy, not the "Deep copy" the commit claims — leaf rulesets (with a mutable reported_metrics list) are still shared with v5.1. Latent; deepcopy or a shared-immutable note. |
| 4 | config/ruleset_registry.py |
52 | api-contract | New duplicate guard makes the module non-idempotent — a re-import / importlib.reload now raises mid-import instead of silently overwriting. |
| 5 | config/schema.py |
1062 | error-handling | model_copy(update=...) bypasses validation and field-name checks; the hardcoded "..._random_seed" keys are decoupled from RuntimeConfig — a future field rename fails silently. |
| 6 | tests/unit/config/test_schema.py |
827 | testing | The mirror partial-None case (scheduler set, sample_index_rng_seed=None) is untested; that is not None False branch is only hit with an already-empty updates. |
| 7 | tests/unit/config/test_schema.py |
167 | documentation | Pre-existing test pins ruleset="mlperf-inference-v6.0", which is not registered (only v5.1/v6.1). Outside the diff, so not posted inline — with the new override it now logs a "not registered" warning every run. Bump to v6.1. |
Commit hygiene: 2 commits, 0 fixups — clean.
Signed-off-by: Rashid Kaleem <230885705+arekay-nv@users.noreply.github.com>
|
with the following section in the client config file (for deepseek for instance):
|
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
Review Council — Multi-AI Code Review (independent)
Reviewed by Codex + Claude | depth: thorough | posted inline: 4
Codex reported no correctness issues. Claude's findings below (none critical/high — the PR is fundamentally sound).
| try: | ||
| ruleset = self.submission_ref.get_ruleset_instance() | ||
| except KeyError as e: | ||
| if self.type == TestType.SUBMISSION: |
There was a problem hiding this comment.
[Claude] medium / api-contract: Breaking change for existing v6.0 submission configs. This branch raises ValidationError at config-load when a type=submission config names an unregistered ruleset. The prior submission_template.yaml on main shipped ruleset: "mlperf-inference-v6.0", which is not registered (only v5.1 / v6.1 / mlcommons-current are). Any user config copied from the old template now hard-fails to load instead of running. The behavior is correct in principle (a submission must pin official seeds), but it warrants a changelog / migration note for in-flight v6.0 configs.
| """ | ||
| # Register every known round under its version-specific name | ||
| for ruleset in mlcommons_rounds: | ||
| _RULESET_REGISTRY.setdefault(f"mlperf-inference-{ruleset.version}", ruleset) |
There was a problem hiding this comment.
[Claude] medium / design: setdefault bypasses the duplicate guard added to register_ruleset in this same PR. If two entries in ALL_ROUNDS ever share a .version, the second is silently dropped (first-wins) with no error — the ValueError guard at line 53 is unreachable from this primary registration path. Production correctness then rests solely on the test_round_versions_are_unique unit test being run. Consider registering rounds through register_ruleset (so duplicates fail loudly at import) and handling re-import idempotency with a sentinel, rather than trading a runtime invariant for a test-only one.
| datasets=[{"path": "test.jsonl"}], | ||
| submission_ref=SubmissionReference(model="test", ruleset="does-not-exist"), | ||
| ) | ||
| assert cfg.settings.runtime.scheduler_random_seed == 42 |
There was a problem hiding this comment.
[Claude] low / testing: Untested new branches. (1) The unknown-runtime-fields raise (schema.py:1075) has no test — nothing injects a bogus key (and it's currently unreachable anyway). (2) _auto_register_mlcommons's claimed idempotency is asserted only in a docstring; no test calls it twice to prove setdefault avoids the duplicate error. (3) The SUBMISSION unregistered-ruleset raise is exercised only via direct BenchmarkConfig(...) construction, not via the from-config YAML path (the real user entry point). Add tests for (2) and (3).
| benchmark_rulesets={ | ||
| # Keep the model-singleton keys; deep-copy only the per-model rules so | ||
| # v6.1's mutable leaves (e.g. reported_metrics) can't leak into v5.1. | ||
| model: copy.deepcopy(rules) |
There was a problem hiding this comment.
[Claude] low / design: Deep-copy correctness rides on enum-deepcopy identity. copy.deepcopy(rules) recurses into the inner dict's keys; lookups like benchmark_rulesets[model][opt_prio] keep working only because OptimizationPriority is an Enum and copy.deepcopy(member) is member. If it ever became a plain class without __deepcopy__, every apply_user_config lookup with the module-level singleton would KeyError. Prefer {k: copy.deepcopy(v) for k, v in rules.items()} to preserve keys explicitly. (The deep copy is also purely defensive — nothing currently mutates reported_metrics — so a key-preserving shallow copy would be equally safe and clearer about intent.)
Review Council — Multi-AI Code Review (independent)Reviewed by Codex + Claude | depth: thorough These reviews were run independently of any prior context — Codex in an isolated worktree, Claude fresh — assessing the diff on its own merits (no assumed verification). Codex: no correctness issues found (reviewed against 🟡 Should Fix (medium)
🔵 Consider (low)
Commit hygiene: 3 commits, clean — no squash needed. |
Signed-off-by: arekay-nv <230885705+arekay-nv@users.noreply.github.com>
3 participants
Adds the MLPerf Inference v6.1 RoundRuleset (schedule/sample_index seeds from loadgen/mlperf.conf; per-model latency targets unchanged from v5.1) and makes it CURRENT. The registry now registers every known round by version, so both v5.1 and v6.1 are resolvable.
When a config carries submission_ref, _resolve_and_validate now pins the runtime RNG seeds from the selected ruleset during construction — before the config is dumped to the report dir, so the persisted config.yaml, RuntimeSettings, and report all reflect the seeds that actually ran. The ruleset value wins over any user-supplied seed (mirrors LoadGen locking core seeds from user.conf). Lenient by design: an unregistered ruleset leaves the config unchanged, so placeholder/non-submission configs are unaffected.
Duration/latency overrides and the runtime-path integration (surfacing ruleset latency targets in the report) are deferred to a follow-up.
What does this PR do?
Type of change
Related issues
Testing
Checklist