refactor(benchmark): remove redundant accuracy_datasets from BenchmarkContext - #437
Merged
Merged
Conversation
…kContext `accuracy_datasets: list[Dataset]` was a parallel copy of data already carried by `eval_configs: list[AccuracyConfiguration]` via the `.dataset` field. Remove it and update all callers to use `eval_configs` directly. - `_load_datasets` return type drops the middle `list[Dataset]` element - `_summarize_and_log_metrics` acc_total now uses `ec.num_repeats` (the canonical AccuracyConfiguration field, consistent with accuracy.py) and filters by `dataset_type == ACCURACY` explicitly - All call sites and test mocks updated - Restored three dataset-level sample assertions in override tests that were lost in the migration (now via `eval_configs[0].dataset`) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
leopck
approved these changes
Aug 17, 2026
arekay-nv
added a commit
that referenced
this pull request
Aug 18, 2026
main #437 refactored _load_datasets to return (dataloader, eval_configs). test_accuracy_only_skips_salt_validation is branch-only, so the merge kept its stale 3-tuple unpack — the sole site the merge missed. Adopt the 2-tuple form; perf_loader is still the first element, so the assertion is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
viraatc
added a commit
that referenced
this pull request
Aug 18, 2026
Reviewer feedback on the split granularity (5 new modules felt like too many): collapse to settings.py + workload.py, criterion 'keep together what changes together'. - settings.py absorbs timeouts.py: Timeouts is the settings.timeouts sub-model and changes with the settings block. - workload.py = model_params.py + datasets.py: the request payload and data definition (generation params, submission ref, datasets, accuracy config, generation-override merge helpers). - audit.py folds into schema.py beside the root aggregates its audit: block plugs into. schema.py stays the re-export hub, so no import site outside config/ changes except three tests that imported config.timeouts directly. The regenerate-templates pre-commit hook now matches every config/*.py instead of an enumerated module list, so a consolidated module cannot silently skip template regeneration. Also updates a stale 3-tuple _load_datasets mock left behind by the rebase onto main (#437 changed the return arity to 2).
8 tasks
viraatc
added a commit
that referenced
this pull request
Aug 18, 2026
…ackage Reviewer feedback on the split granularity (5 new sibling modules felt like too many, and config/settings.py was confusable with the pre-existing config/runtime_settings.py): the declared YAML/CLI surface now lives in one namespaced package, config/schema/, three modules total — criterion 'keep together what changes together'. - schema/__init__.py: the former schema.py — root aggregates (BenchmarkConfig/EndpointConfig), cross-field validation, root-level TestType/TestMode, the small audit: block (absorbed from audit.py), and the re-export hub, so every config.schema import site is unchanged. - schema/settings.py: the settings: subtree; absorbs timeouts.py (Timeouts is the settings.timeouts sub-model). - schema/workload.py: request payload + data definition; merges model_params.py + datasets.py (generation params, submission ref, datasets, accuracy config, generation-override merge helpers). config/ top level now separates cleanly for readers: schema/ = what users declare; runtime_settings.py = the resolved run plan; rulesets/ et al. = policy machinery. The regenerate-templates pre-commit hook now matches every config module (including schema/*) instead of an enumerated filename list, so a moved module cannot silently skip template regeneration. Also updates a stale 3-tuple _load_datasets mock left behind by the rebase onto main (#437 changed the return arity to 2).
viraatc
added a commit
that referenced
this pull request
Aug 18, 2026
Reviewer discussion on #409 settled on keeping this MR about the timeout-knob consolidation; the schema.py monolith split (whatever its final shape) moves to a dedicated follow-up MR. schema.py stays one module — the declared YAML/CLI surface, distinct from the resolved run plan in runtime_settings.py — now containing the settings: subtree (incl. the frozen Timeouts at settings.timeouts), the workload definition (model_params:/datasets:), the root aggregates, and the audit: block, in that order with section banners. Kept from the split work, split-independent: - the regenerate-templates pre-commit hook matches every config/*.py instead of an enumerated filename list, so a renamed module cannot silently skip template regeneration. - enums.py stays dissolved: each enum lives beside its owner. - a stale 3-tuple _load_datasets mock left by the rebase onto main (#437 changed the return arity to 2) stays fixed.
viraatc
added a commit
that referenced
this pull request
Aug 18, 2026
Reviewer discussion on #409 settled on keeping this MR about the timeout-knob consolidation; the schema.py monolith split (whatever its final shape) moves to a dedicated follow-up MR. schema.py stays one module — the declared YAML/CLI surface, distinct from the resolved run plan in runtime_settings.py — now containing the settings: subtree (incl. the frozen Timeouts at settings.timeouts), the workload definition (model_params:/datasets:), the root aggregates, and the audit: block, in that order with section banners. Kept from the split work, split-independent: - the regenerate-templates pre-commit hook matches every config/*.py instead of an enumerated filename list, so a renamed module cannot silently skip template regeneration. - enums.py stays dissolved: each enum lives beside its owner. - a stale 3-tuple _load_datasets mock left by the rebase onto main (#437 changed the return arity to 2) stays fixed.
viraatc
added a commit
that referenced
this pull request
Aug 18, 2026
Reviewer discussion on #409 settled on keeping this MR about the timeout-knob consolidation; the schema.py monolith split (whatever its final shape) moves to a dedicated follow-up MR. schema.py stays one module — the declared YAML/CLI surface, distinct from the resolved run plan in runtime_settings.py — now containing the settings: subtree (incl. the frozen Timeouts at settings.timeouts), the workload definition (model_params:/datasets:), the root aggregates, and the audit: block, in that order with section banners. Kept from the split work, split-independent: - the regenerate-templates pre-commit hook matches every config/*.py instead of an enumerated filename list, so a renamed module cannot silently skip template regeneration. - enums.py stays dissolved: each enum lives beside its owner. - a stale 3-tuple _load_datasets mock left by the rebase onto main (#437 changed the return arity to 2) stays fixed.
viraatc
added a commit
that referenced
this pull request
Aug 20, 2026
Reviewer discussion on #409 settled on keeping this MR about the timeout-knob consolidation; the schema.py monolith split (whatever its final shape) moves to a dedicated follow-up MR. schema.py stays one module — the declared YAML/CLI surface, distinct from the resolved run plan in runtime_settings.py — now containing the settings: subtree (incl. the frozen Timeouts at settings.timeouts), the workload definition (model_params:/datasets:), the root aggregates, and the audit: block, in that order with section banners. Kept from the split work, split-independent: - the regenerate-templates pre-commit hook matches every config/*.py instead of an enumerated filename list, so a renamed module cannot silently skip template regeneration. - enums.py stays dissolved: each enum lives beside its owner. - a stale 3-tuple _load_datasets mock left by the rebase onto main (#437 changed the return arity to 2) stays fixed.
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
3 participants
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.
Summary
accuracy_datasets: list[Dataset]fromBenchmarkContext— it was a parallel copy of data already carried byeval_configs: list[AccuracyConfiguration]via the.datasetfield_load_datasetsreturn type drops the middlelist[Dataset]element; all call sites updated_summarize_and_log_metricsacc_totalnow usesec.num_repeats(the canonicalAccuracyConfigurationfield, consistent withaccuracy.py) and filters bydataset_type == ACCURACYexplicitly rather than relying on an implicit invarianteval_configs[0].dataset.load_sample(0)) that were dropped during the migrationTest plan
uv run pytest tests/unit/commands/test_benchmark.py— 178 tests passTestLoadDatasetsGenerationConfigOverrideChat,TestLoadDatasetsGenerationConfigOverrideCompletions) pass with restored sample-level assertions🤖 Generated with Claude Code