feat: env-var consolidation, score aggregation, and ManPage env-var documentation#686
Draft
FileSystemGuy wants to merge 46 commits into
Draft
feat: env-var consolidation, score aggregation, and ManPage env-var documentation#686FileSystemGuy wants to merge 46 commits into
FileSystemGuy wants to merge 46 commits into
Conversation
…BACK_* resolvers - Introduce 5 MLPSTORAGE_*_ENVVAR string constants as single source of truth (D-10). - Add _LEGACY_ENVVAR_MAP for migration-hint lookup (excludes CHECKPOINT_FOLDER per D-08). - Rename _resolve_default_* → _resolve_env_fallback_*, DEFAULT_* → ENV_FALLBACK_*. - Add resolvers/constants for DATA_DIR and CHECKPOINT_FOLDER. - Delete tempdir results-dir fallback and drop unused `import tempfile` (D-13, D-14). - All resolvers return "" when env var unset (SC-3): never None, never a tempdir path.
…DEFAULT_* refs - Delete the 10-line dead-code warn block at main.py:247–256 (D-13); the parse-time loud-error gate in Plan 05-02 replaces its job. - Drop DEFAULT_RESULTS_DIR from main.py's config import; rewrite the stray reference at the reports-subcommand dispatch to getattr(args, 'results_dir', ""). - [Rule 3] Rename common_args.py's DEFAULT_RESULTS_DIR / DEFAULT_SYSTEMNAME references to ENV_FALLBACK_RESULTS_DIR / ENV_FALLBACK_SYSTEMNAME; without this, `import mlpstorage_py.main` (a Task-2 acceptance criterion) raises ImportError against the renamed constants from Task 1.
…cal redefinition - Extend the `from mlpstorage_py.config import ...` block to pull MLPSTORAGE_ORGNAME_ENVVAR and MLPSTORAGE_SYSTEMNAME_ENVVAR from config.py. - Delete the two local string-constant assignments (previously rules/utils.py:21-22). - Preserves the one-way dependency invariant: config → rules.utils (D-11). Downstream callers reading `from mlpstorage_py.rules.utils import MLPSTORAGE_*_ENVVAR` still work because the config import re-exposes the names in this module's namespace.
…TORAGE_* - Rename MLPERF_RESULTS_DIR and MLPERF_SYSTEMNAME rows to their MLPSTORAGE_* counterparts in the run summary Environment section. - Add three sibling rows for MLPSTORAGE_ORGNAME, MLPSTORAGE_DATA_DIR, and MLPSTORAGE_CHECKPOINT_FOLDER so the diagnostic surface reflects the full set of owned env vars. - Order: RESULTS_DIR, SYSTEMNAME, ORGNAME, DATA_DIR, CHECKPOINT_FOLDER, then MPI_RUN_BIN, MPI_EXEC_BIN (unchanged), then existing KVCACHE_* block. - MPI_RUN_BIN, MPI_EXEC_BIN, and KVCACHE_SELECTED_WORKLOADS rows unchanged.
- add_universal_arguments grows a third opt-in kwarg req_checkpoint_folder (default False) that plumbs the _mlps_req_checkpoint_folder marker via parser.set_defaults, mirroring the req_results / req_systemname pattern (D-09). The --checkpoint-folder arg itself stays in checkpointing_args.py so no other benchmark inherits it. - HELP_MESSAGES['systemname'] now references MLPSTORAGE_SYSTEMNAME (not MLPERF_SYSTEMNAME). Marker-plumbing NOTE block updated to name MLPSTORAGE_RESULTS_DIR / MLPSTORAGE_SYSTEMNAME as the env-var-fallback sources. - Docstring documents the new kwarg and points readers at D-09.
- --data-dir now defaults to ENV_FALLBACK_DATA_DIR (from MLPSTORAGE_DATA_DIR
env var if set) so users can seed data location via env alone (ENV-03).
- Post-YAML gate in validate_training_arguments now emits the D-02 verbatim
loud-error line:
"error: --data-dir/-dd is required: pass it on the command line or set
MLPSTORAGE_DATA_DIR"
Gate location (post-YAML, object-mode-only guard) is byte-preserved per
D-07 so --config-file YAML data_dir: still counts.
- D-05 migration hint fires immediately below the error line when the user
has MLPERF_DATA_DIR set but MLPSTORAGE_DATA_DIR unset — actionable at the
exact moment of failure. Hint uses the verbatim D-05 template.
- Adds `import os` for the environ lookup.
…D-04/D-05
- Add _UNIVERSAL_REQUIRED_SPECS table: three rows for --results-dir/-rd,
--systemname/-sn, --checkpoint-folder/-cf (D-09). Training's --data-dir
is intentionally absent — its gate stays post-YAML in training_args.py
(D-07) so --config-file can supply data_dir.
- Emit ONE 'error:' line per missing required universal using the D-02
verbatim template: '--{long}/-{short} is required: pass it on the
command line or set MLPSTORAGE_{NAME}'. Aggregate every missing flag
before exiting once with EXIT_CODE.INVALID_ARGUMENTS (D-01, D-03,
T-05-06). No comma-join, no first-error-then-exit.
- Emit an adjacent 'hint:' line immediately below its associated 'error:'
when the legacy MLPERF_* env var is set AND the new MLPSTORAGE_* is not,
using the D-05 verbatim template: 'hint: MLPERF_{NAME} is set but is no
longer read; rename it to MLPSTORAGE_{NAME}'. Lookup goes through
_LEGACY_ENVVAR_MAP imported from config — no bare MLPERF_* string
literals in cli_parser.py.
- Since _LEGACY_ENVVAR_MAP has no key for MLPSTORAGE_CHECKPOINT_FOLDER
(D-08 — no legacy MLPERF_CHECKPOINT_FOLDER ever existed), the hint
never fires for checkpoint-folder; the exclusion falls out of the map
lookup with no special-case code.
- Rewrite the docstring to reflect the three-universal scope, per-flag
emission model, adjacent-hint contract, and D-01/D-02/D-04/D-05/D-09
references. Drop obsolete mentions of MLPERF_RESULTS_DIR /
MLPERF_SYSTEMNAME as arg-populating defaults.
Behavioral checks (all three markers set, all values empty, env clean):
* 3 'error:' lines, 0 'hint:' lines, exit code 2, verbatim templates.
With MLPERF_SYSTEMNAME and MLPERF_RESULTS_DIR set, MLPSTORAGE_* unset:
* 3 'error:' lines with hints IMMEDIATELY below the results/systemname
errors, and none below checkpoint-folder.
TEST-13 (Plan 05-06) will pin these exact strings; any future paraphrase
fails the phase-wide grep pinned in the acceptance-criteria block of
05-02-PLAN.md.
…for run - --checkpoint-folder in checkpointing_args.py no longer sets argparse-level required=True; the default is ENV_FALLBACK_CHECKPOINT_FOLDER (from MLPSTORAGE_CHECKPOINT_FOLDER env var if set) so the fallback can satisfy the requirement (D-08 / ENV-06). - parser.set_defaults(_mlps_req_checkpoint_folder=True) plumbed locally next to the arg definition inside the `command == "run"` branch so the flag and its post-parse enforcement marker stay co-located (D-09). The loud-error gate lives in cli_parser._check_universal_required_present (Plan 05-02). - HELP_MESSAGES['checkpoint_folder'] in common_args.py extended to name MLPSTORAGE_CHECKPOINT_FOLDER as the env-var fallback. - --checkpoint-folder remains defined ONLY in checkpointing_args.py (no leak to other benchmarks).
…RAGE_/ENV_FALLBACK_ - Rewrite TestDefaultResultsDir → TestEnvFallbackResultsDir; tempdir-fallback test now asserts _resolve_env_fallback_results_dir() returns "" when MLPSTORAGE_RESULTS_DIR is unset (D-13 tempdir retirement). - Rewrite TestDefaultSystemname → TestEnvFallbackSystemname; monkeypatches updated MLPERF_SYSTEMNAME → MLPSTORAGE_SYSTEMNAME. - Drop 'import tempfile' (no remaining consumer in the file). - All 55 tests in test_config.py pass.
- Pins D-02 verbatim error template and D-05 verbatim hint template - TEST-12: MLPERF_SYSTEMNAME alone still fails the ENV-04 loud-error gate - D-04 truth table for the migration hint (all four scenarios) - Covers checkpoint-folder no-legacy-hint case (D-08) - Covers training --data-dir post-YAML gate hint stanza (D-07)
- Fresh-subprocess assertion proves no import cycle exists - Structural grep asserts config.py has no 'from mlpstorage_py.rules' line - Positive-direction assertion documents rules.utils sources env-var constants from config (D-10 single-source-of-truth)
… tests - test_cli.py: rewrite the test_results_dir_required_when_req_results_true helper to patch common_args.ENV_FALLBACK_RESULTS_DIR (was DEFAULT_RESULTS_DIR) and update the docstring to reference MLPSTORAGE_RESULTS_DIR. - test_cli_parser.py: rename every MLPERF_SYSTEMNAME / MLPERF_RESULTS_DIR monkeypatch + assertion + comment to MLPSTORAGE_*, and every DEFAULT_* attribute patch / _resolve_default_* call to ENV_FALLBACK_* / _resolve_env_fallback_*. Update the results-dir-env-var-satisfies test docstring to reflect D-13 (tempdir fallback retired; empty-string fallback now). - test_run_summary.py: rename test_banner_environment_includes_mlperf_systemname → *_mlpstorage_systemname; add sibling test asserting the Environment section contains all five MLPSTORAGE_* rows added by Plan 05-04 (RESULTS_DIR, SYSTEMNAME, ORGNAME, DATA_DIR, CHECKPOINT_FOLDER). - 204 passed / 1 skipped across the three files.
- Pins D-02 verbatim error template for all four required universals: --results-dir, --systemname, --data-dir, --checkpoint-folder - Asserts BOTH the flag string AND the MLPSTORAGE_* env-var string appear on ONE 'error:' line per D-02 - ENV-06 canonical test drives checkpoint-folder end-to-end through parse_arguments with a patched sys.argv - D-01 aggregate-before-exit: three missing universals produce three error lines and one sys.exit(2) call
The tempdir-fallback warning that this file exercised is structurally unreachable after Plan 05-01 deleted the fallback path and the dead-code warn block. D-15 mandates first-class deletion (no skip-marker stub). Also remove the corresponding row from tests/README.md and update the neighboring test_config.py row to reference the new ENV_FALLBACK_RESULTS_DIR / ENV_FALLBACK_SYSTEMNAME constants (D-12).
- 6-run unet3d fixture with anomalous 10x warmup at 20260701_100000 (start field matches 20260701_101500 to fire _process_single_run DLIO id-collision detection at report_generator.py:544-571) — anchors TEST-14 warmup exclusion assertion. - 3-run resnet50 partial fixture — triggers D-24 _INVALID_MSG_TRAINING_COUNT for D-27 rules-strict coverage. - Existing 20250115_143022 fixture byte-unchanged.
- 10-op happy path (20260703_100000) — D-20 canonical shape: read and write throughput lists each have EXACTLY 10 elements. - 7-op partial (20260703_120000) — TEST-15 D-24 _INVALID_MSG_CHECKPOINT_COUNT trigger. - Existing 20250115_150000 fixture untouched.
- milvus/hnsw/run/20260704_100000: recall present in summary.json (D-21 primary in-summary path). - pgvector/ivfflat/run/20260704_120000: summary.json omits recall, sibling recall_stats.json present (D-21 fallback path per vdb_checks.py:462-469). recall_stats shape mirrors calc_recall output from vdb_benchmark/vdbbench/simple_bench.py:140. - Both summaries carry all five _REQUIRED_METRIC_FIELDS.
…pass-through) - kvcache/llama3-8b/run/20260704_140000 with top-level aggregated_* fields and options dict containing profile_a and profile_b sub-dicts (each mirroring kvcache._aggregate_option_results:791-803 shape). - profile_a.aggregated_write_bandwidth_gbps = 0.0 anchors the D-22 pass-through boundary test: reportgen must emit 0.0 verbatim.
- whatif/training/unet3d/run/ with 3 partial training runs (would trigger D-24 _INVALID_MSG_TRAINING_COUNT in a closed/open context; per D-29 the whatif category value causes reportgen to SKIP rules-strict — the row must emit with category='whatif', not category='INVALID'). - metric lists are non-empty because aggregation math still runs on projected metrics per D-29.
…chor)
- multi_orgname/closed/{acme,beta_corp}/results/system-{a,b}/training/unet3d/run/
replicates the canonical CLOSED layout for two distinct orgnames
side-by-side. Used by 06-05/06-06 to verify D-08: one top-level
results.{csv,json} contains rows from BOTH orgs distinguished by
the orgname column.
Note: canonical layout includes a 'results/' path segment that
matches the repo .gitignore's 'results/' output-dir pattern. Files
force-added since the fixture must mirror the production tree shape
that generate_output_location emits (Rule 3 blocking-issue fix:
gitignore rule is intended for benchmark runtime output, not test
fixtures under tests/fixtures/).
- empty_model/training/unet3d/run/ has a valid model+run directory layout but ZERO timestamp subdirectories. Used by 06-06 to verify D-03: reportgen must regenerate results.csv (header row only) and results.json ([]) for empty model dirs, not skip them. - .gitkeep preserves the empty directory under git.
- degenerate/empty_metric/training/unet3d/run/20260707_100000: train_au_percentage=[] triggers D-23 empty-list INVALID downgrade; other metrics stay valid to anchor 'per-metric null / row category INVALID' behavior. - degenerate/nan_metric/training/unet3d/run/20260707_120000: train_au_percentage=[null] documents helper's behavior on NaN (JSON has no NaN literal; null stands in per D-23 discussion). - degenerate/missing_summary/training/unet3d/run/20260707_140000/: no summary.json, just .gitkeep — covers T-06-06 FileNotFoundError graceful-degradation path.
…ants
- Import statistics.fmean + StatisticsError (STACK.md A-01 ADR — no numpy/pandas/scipy)
- Import typing.Set for the forthcoming _aggregate_workload_metrics(runs, warmup_set) signature
- Four module-scope D-24 verbatim message templates with {n}/{key}/{basename} placeholders:
_INVALID_MSG_TRAINING_COUNT, _INVALID_MSG_WARMUP_UNDETECTED,
_INVALID_MSG_CHECKPOINT_COUNT, _INVALID_MSG_EMPTY_METRIC
- Leading DO NOT paraphrase comment (Phase 5 D-02 precedent); § character preserved
Prep for Task 2 which introduces _aggregate_workload_metrics helper. Wiring
into _process_workload_groups deferred to plan 06-04 per phase boundary.
… helpers Introduces the pure aggregation helper on ReportGenerator, placed immediately before _process_workload_groups (the caller in 06-04): - _aggregate_workload_metrics(runs, warmup_set): dispatches on runs[0].benchmark_type across four private helper methods (D-19..D-22). - _aggregate_training (D-19): filters warmup via os.path.abspath(), intra-invocation fmean of each metric list, then inter-invocation fmean of the per-run means. Output key 'train_mean_of_<basename>' strips redundant 'train_' prefix (D-13). - _aggregate_checkpointing (D-20/D-28): warmup_set ignored per docstring; intra-list fmean per invocation; inter-invocation fmean only when len(runs) > 1. Output key 'checkpoint_mean_of_<basename>'. - _aggregate_vdb (D-21): pass-through of throughput/latency/recall fields from summary.json into vdb_* columns; recall_stats.json fallback per submission_checker/checks/vdb_checks.py:462-469; identity columns vdb_engine and vdb_index_type from BenchmarkRun.parameters. - _aggregate_kvcache (D-22/D-16/D-17): pass-through of top-level aggregated_* fields into kvcache_aggregated_* columns; per-option flattening into kvcache_option_<opt>_<metric> columns preserving the source 'aggregated_' word verbatim (D-17 grep-chain). - _load_workload_summary helper: reads <result_dir>/summary.json with OSError/ValueError logged and empty dict returned (mitigates T-06-06 without aborting reportgen). Empty metric lists raise statistics.StatisticsError with the metric key and invocation basename included (D-23) — the caller in 06-04 catches it and downgrades the workload to INVALID using the D-24 _INVALID_MSG_EMPTY_METRIC template. NO 'else 0.0' fallback, NO broad except swallowing StatisticsError. _process_workload_groups and the writer signatures are byte-unchanged (SC-6 preserved). Full unit suite passes with zero regressions.
Rules.md §2.1.28 defines the mechanical file shape produced by reportgen:
bottom-up build from per-model files; one row per workload (no per-row
discriminator column); fixed 6-column prefix (category, orgname, systemname,
benchmark_type, model, accelerator); per-type-grouped columns with train_/
checkpoint_/vdb_/kvcache_ prefixes; alphabetical within each group; trailing
issues column; category ∈ {closed, open, whatif, INVALID}; whatif skips
rules-strict INVALID gates.
Companion .planning/ROADMAP.md + .planning/REQUIREMENTS.md edits (SC-1/SC-5
rewrite; AGG-01..05 extension; Traceability + Coverage bump) are gitignored
per repo policy and land alongside on-disk only.
- Refactor grouping key to per-benchmark-type dispatch (D-05):
- training/checkpointing: (category, orgname, systemname, model, accelerator)
- vdb: (category, orgname, systemname, engine, index_type)
- kv_cache: (category, orgname, systemname, model, performance_profile)
- Add _derive_workload_key + _derive_category/orgname/systemname_from_path
helpers with defensive params handling (D-07).
- Wire _aggregate_workload_metrics call at the former TODO site, wrapped
in a targeted try/except statistics.StatisticsError (D-23 loud-failure).
Inner catch handles the D-24 _INVALID_MSG_EMPTY_METRIC template with
best-effort key/basename extraction; outer broad Exception catch
preserved as pre-aggregation safety net (unchanged).
- Insert rules-strict INVALID gates before aggregation dispatch:
- D-27: training len(runs) != 6 -> _INVALID_MSG_TRAINING_COUNT
- D-26: 6-invocation training with no warmup detected
-> _INVALID_MSG_WARMUP_UNDETECTED
- D-20/D-24: checkpointing metric list len != 10
-> _INVALID_MSG_CHECKPOINT_COUNT
- Gates SKIP for category_str == 'whatif' per D-29.
- Vdb + kvcache SKIP rules-strict entirely (D-22 pass-through).
- Update _print_workload_details to read model/accelerator from
benchmark_run[0] instead of unpacking the workload_key tuple
(decouples printer from D-05 key shape evolution).
- Add module-level 'import statistics' for the targeted except clause.
- Update test_reporting.py::test_groups_by_workload to assert the new
5-tuple key shape (tail (model, accelerator) still stable).
- Update test_reporting.py::test_prints_workload_results to use the
5-tuple key with empty (category, orgname, systemname) leading slots.
- Iterate self.workload_results as single source of truth (was
self.run_results). One aggregated row per workload.
- Add _workload_result_to_row: builds the flat row dict with the
D-10 6-column prefix (category, orgname, systemname, benchmark_type,
model, accelerator), aggregated metric columns from Result.metrics,
and the D-25 trailing 'issues' column joined verbatim by '; '.
- Add _model_group_folder_for_workload: workload-aware wrapper that
synthesizes a single-run proxy Result so the existing
_model_group_folder helper resolves correctly for workload-level
Result objects (their benchmark_run is a list).
- Bottom-up flow (D-02):
per-model rows -> per-model file emission -> concatenate ->
sorted top-level file emission
- Sort top-level all_rows by the 6-column prefix tuple so file order
is deterministic across runs.
- Vdb rows leave model + accelerator empty; kvcache rows have model
populated and accelerator empty (per D-10 / D-14).
- Preserve the 'no runs to write rollups for' warning path.
- SC-6 grep gate preserved (target_dir= appears 8 times: per-model +
top-level + Task-4 empty-emission sites).
- Placeholder _emit_empty_model_dirs + _enumerate_on_disk_model_dirs
added here as part of the bottom-up flow so generate_reports can
call them cleanly (their body is Task 4's D-03 corner emission).
- Update test_reporting.py fixture to populate workload_results in
addition to run_results so the bottom-up flow tests pass under the
new architecture.
- Add ReportGenerator._ordered_fieldnames(rows) implementing the D-11
layout:
[category, orgname, systemname, benchmark_type, model, accelerator]
+ sorted train_*
+ sorted checkpoint_*
+ sorted vdb_*
+ sorted kvcache_*
+ sorted 'other' (defensive)
+ [issues]
- write_csv_file body swaps fieldnames=sorted(fieldnames) for
fieldnames=self._ordered_fieldnames(flattened_results).
- Preserve SC-6 writer signature contract (byte-unchanged):
def write_csv_file(self, results, target_dir: Optional[str] = None)
def write_json_file(self, results, target_dir: Optional[str] = None)
- Preserve csv.DictWriter(..., lineterminator='\n') + writeheader() +
writerows(...) writer shape.
- Empty rows list -> header of prefix + trailing only (D-03 corner:
results.csv = header row only for empty per-model dirs).
- write_json_file body unchanged: JSON preserves dict insertion
order, so per-row key order is set by _workload_result_to_row.
… / SC-2) tests/unit/test_aggregation.py — seven test classes, 21 tests total: - TestTrainingAggregation (TEST-14 / D-19 / AGG-01): 6-run unet3d fixture → mean over 5 real runs only; warmup (~10%) excluded - TestCheckpointingAggregation (TEST-15 / D-20 / D-28 / AGG-02): 10-op intra-list fmean; D-28 warmup_set ignored - TestVdbPassThrough (D-21 / AGG-04): in-summary recall + recall_stats.json fallback path (pgvector/ivfflat) - TestKvCachePassThrough (D-22 / D-16 / D-17 / AGG-05): per-option flattening, D-22 verbatim 0.0 sentinel preservation, D-17 aggregated_ prefix retention - TestInvalidRulesStrict (D-23/D-24/D-26/D-27/D-29): four D-24 verbatim substrings pinned + D-29 whatif SKIPs INVALID gates + StatisticsError propagation - TestColumnOrdering (D-14 / D-18): 6-col prefix, trailing issues, group order train→checkpoint→vdb→kvcache, alphabetical within groups, end-to-end write_csv_file header pin - TestNumpyPandasScipyForbidden (SC-2 grep gate): comment-stripped read of report_generator.py; asserts no numpy/pandas/scipy import + positive pin for 'from statistics import fmean' D-24 verbatim strings pinned twice: via imported _INVALID_MSG_* constants + .format() (structured drift detection on either side) AND as raw literal substrings (Phase 5 D-02 verbatim-pinning precedent — mirrors test_env_var_loud_errors.py). No mocking of _aggregate_workload_metrics itself — tests exercise the real helper against real 06-03 fixtures. No numpy/pandas/scipy in the test file. No skip/xfail markers. Also: tests/fixtures/sample_results/vdb/pgvector/ivfflat/run/20260704_120000/ recall_stats.json — added 'recall: 0.975' key alongside the existing 'recall_at_k' field. Rule 3 (blocking-issue auto-fix): the D-21 fallback code path in report_generator._aggregate_vdb reads 'recall' from recall_stats.json, so the fixture's fallback test needs that key present. Baseline: 2496 passed / 1 skipped → 2517 passed / 1 skipped (+21).
…omplete - tests/unit/test_reportgen_output_shape.py: 5 classes / 9 tests pinning D-10 6-column prefix, D-12 trailing issues, D-25 '; ' join, D-01 defense (no row_type in header/JSON/source), D-29 whatif category, D-08 multi-orgname mixed-row via D-09 per-org per-model files. - tests/integration/test_reportgen_aggregate_end_to_end.py: 4 classes / 4 tests exercising the FULL production pipeline (no writer or aggregator patches) against fixture trees under tmp_path — D-02 bottom-up integrity (top-level rows ⊂ union of per-model rows), D-03 canonical delete-and-rerun (N rows before deletion, N-1 after shutil.rmtree + rerun, deleted workload identity absent), D-03 empty-model corner (header-only CSV + [] JSON), D-04 unrelated-file preservation. Full sweep: 2517 → 2530 passed / 1 skipped (+13, zero regressions). Phase 6 delivers AGG-01/02/03/04/05, TEST-14, TEST-15.
… config.py - _MANPAGE_SYNC_ALLOWLIST frozenset (7 entries): PATH, USER, LOGNAME, MLPERF_SYSTEMNAME, MLPERF_RESULTS_DIR, MLPERF_DATA_DIR, MLPERF_ORGNAME - MANPAGE_ENV_VAR_TIERS dict (25 entries): 7 owned, 4 mpi-borrowed, 5 aws-borrowed, 8 storage-borrowed, 1 internal-write (DLIO_DROP_CACHES_TIMEOUT) - Header comment naming D-05/D-10/D-11 (allowlist rationale) and D-13/D-14 (primary-tier rule); single-source-of-truth sibling of _LEGACY_ENVVAR_MAP - test_no_import_cycles.py still passes (3/3); no rules.* import added
- Replace 4-bullet MLPERF_* environment block (L926-933) with full six-tier
markdown table structure per D-07/D-15
- Owned (7): MLPSTORAGE_RESULTS_DIR/SYSTEMNAME/DATA_DIR/CHECKPOINT_FOLDER/
ORGNAME/CHECKPOINT_URI_SCHEME + KVCACHE_SELECTED_WORKLOADS
- MPI-borrowed (4): MPI_RUN_BIN/MPI_EXEC_BIN/OMPI_COMM_WORLD_RANK/PMI_RANK
- AWS-borrowed (5): AWS_ACCESS_KEY_ID/SECRET/REGION/CA_BUNDLE/ENDPOINT_URL
(no example credential values per T-07-06 mitigation)
- Storage-borrowed (8): BUCKET/STORAGE_LIBRARY/STORAGE_URI_SCHEME/
S3_LOAD_BALANCE_STRATEGY/S3_ENDPOINT_{URIS,TEMPLATE,FILE,ENDPOINT}
- Diagnostic: header-only per plan (0 primary entries in current inventory)
- Internal-write (1 primary): DLIO_DROP_CACHES_TIMEOUT + cross-refs for
AWS_ENDPOINT_URL and MLPSTORAGE_CHECKPOINT_URI_SCHEME per D-13
…RECTORY - Insert ### Aggregate Interpretation as last subsection under ## RESULTS DIRECTORY (after ### Common artifacts, before ## VALIDATOR) per D-19 - First paragraph is D-20 verbatim anti-leaderboard framing (pinned; Plan D substring-asserts this) - Second paragraph enumerates all aggregate column families per D-21: train_mean_of_*, checkpoint_mean_of_*, vdb_* pass-through, kvcache_aggregated_*/kvcache_option_*
…o MLPSTORAGE_ - Drop reportgen from required-list in Universal options --systemname bullet per D-22 (was: run/datagen/configview/reportgen/history rerun) - Drop reportgen from required-list in Systemname resolution section (L150) for consistency (same content, same editorial rule) - Rename $MLPERF_SYSTEMNAME → $MLPSTORAGE_SYSTEMNAME in both locations - Add cross-ref sentence to Reports subsection for reportgen's optional- systemname multi-system-fallback behavior
…erbatim - Change (required) → (optional) on reportgen --systemname subsection per D-23 - Rename $MLPERF_SYSTEMNAME → $MLPSTORAGE_SYSTEMNAME in reportgen block - Append D-23 verbatim multi-system-fallback sentence (pinned, Plan D substring-asserts this): 'When omitted, reportgen operates on the entire results-dir tree and derives systemname per row from the workload dir's path segment (open/<org>/results/<systemname>/... or checkpointing/training/vdb/kvcache analog).'
…-1/SC-6) Rename sites: - L32 SYNOPSIS: MLPERF_SYSTEMNAME → MLPSTORAGE_SYSTEMNAME - L80 DESIGN PHILOSOPHY: MLPERF_ORGNAME → MLPSTORAGE_ORGNAME - L81 DESIGN PHILOSOPHY: $MLPERF_SYSTEMNAME → $MLPSTORAGE_SYSTEMNAME - L264 RESULTS DIRECTORY: $MLPERF_RESULTS_DIR → $MLPSTORAGE_RESULTS_DIR - L274 RESULTS DIRECTORY tree: MLPERF_SYSTEMNAME → MLPSTORAGE_SYSTEMNAME - L476 Universal options --results-dir: $MLPERF_RESULTS_DIR → $MLPSTORAGE_RESULTS_DIR - L966 EXAMPLES export block: MLPERF_SYSTEMNAME → MLPSTORAGE_SYSTEMNAME SC-1: grep -nE 'MLPERF_(SYSTEMNAME|RESULTS_DIR|ORGNAME|DATA_DIR)' ManPage.md = 0 hits SC-6: grep -rnE '...' ManPage.md docs/ = 0 hits (examples/ dir does not exist)
- Four test classes: TestEnvVarInventorySync, TestPerVarTierAssignment, TestVerbatimPinnedStrings, TestMlperfRenameGate - _scan_python_reads: D-01 literal-arg (get/bracket/getenv) + D-02 constant-declarations + indirect-chain scanner for storage_config._resolve_endpoint - _scan_shell_reads: D-03 kv-cache-wrapper.sh getenv scan - _parse_manpage_env_vars: extracts per-tier table rows from ## ENVIRONMENT - Symmetric-difference assertion modulo _MANPAGE_SYNC_ALLOWLIST (SC-3) - SC-4 tier pin checks; D-20 and D-23 verbatim substring assertions - SC-1/SC-6 MLPERF_* rename gate for ManPage.md and docs/ - Two negative-simulation tests prove grep pipeline fires on synthetic inputs - 12 tests, all pass; 0 rules.* imports; 0 skipped tests
- Add S3DLIO_PINNED_VERSION = 'v0.9.106' documentation anchor constant - Add _S3DLIO_HIGH_RISK_ENV_VARS frozenset with 15 HIGH-risk vars (D-03) - Add MANPAGE_STORAGE_BACKEND_ENV_VARS frozenset with 25 entries (D-02/D-09) - Extend MANPAGE_ENV_VAR_TIERS with 3 aws-borrowed + 22 storage-backend entries - Update block comment to 'Seven allowed primary-tier values' including 'storage-backend'
…rowed - Add ### Storage-backend section with 22-row table anchored on s3dlio v0.9.106 - 15 HIGH-risk vars tagged '[high-risk: alters measured benchmark behavior]' - 7 MEDIUM-risk vars tagged '[medium-risk: affects run stability, not throughput measurement]' - Add 3 new rows to ### AWS-borrowed: AWS_SESSION_TOKEN, AWS_DEFAULT_REGION, AWS_S3_ADDRESSING_STYLE - Update ## ENVIRONMENT intro from 'Six tiers' to 'Seven tiers' including Storage-backend - Add Azure/GCS deferred pointer to s3dlio Environment_Variables.md
…canonical tier - Add 'storage-backend' to tier_canonical dict in _parse_manpage_env_vars so S3DLIO_* rows are parsed from ManPage tables (not silently skipped) - Update test_every_tier_value_is_one_of_six_canonical_strings valid_tiers set to include 'storage-backend' (Phase 7.5 D-01) - TestEnvVarInventorySync.test_code_reads_equal_manpage_mentions_modulo_allowlist remains a known failure until Plan 02 adds MANPAGE_STORAGE_BACKEND_ENV_VARS subtraction
- Import _S3DLIO_HIGH_RISK_ENV_VARS from mlpstorage_py.config - Add private method Benchmark._check_storage_backend_env() with D-08 verbatim warning template - Guard on STORAGE_LIBRARY='s3dlio' to avoid false positives in training-only environments (D-07) - Call from _pre_execution_gate() after CAP-03 probe (Phase 7.5 D-07)
… carve-out and TestStorageBackendEnvVars - Import MANPAGE_STORAGE_BACKEND_ENV_VARS from config - Subtract MANPAGE_STORAGE_BACKEND_ENV_VARS from extra_in_docs so storage-backend vars don't fail the symmetric-difference check - Add TestStorageBackendEnvVars class with 3 test methods: backend vars in ManPage, disjoint from allowlist, D-08 verbatim prefix lock
- Add rule 1.2 storageBackendEnvVarDeclaration with D-14 verbatim wording - Requires OPEN category for runs with undeclared storage-backend env vars - Adds Azure/GCS pointer to s3dlio Environment_Variables.md
…raining _model_group_folder returns <model>/run/ for training (Rules.md 2.1.16), but _enumerate_on_disk_model_dirs was returning <model>/. The mismatch caused _emit_empty_model_dirs to treat <model>/ as an unaccounted dir and write a stray results.json there. Now training dirs enumerate to <model>/run/. Also update test_reportgen_output_shape expected paths to match, and fix MLPERF_SYSTEMNAME → MLPSTORAGE_SYSTEMNAME typo in Rules.md.
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
…o main - Conflict in report_generator.py: comment-only; kept HEAD's more detailed Rules.md 2.1.16 cross-reference. - Conflict in test_reportgen_aggregate_end_to_end.py: take main's training path assertions (<model>/run/) to match _enumerate_on_disk_model_dirs fix already in the top commit of this branch (e12e813). - Conflict in test_reportgen_output_shape.py: take main's comment explaining Rules.md 2.1.16 mandate; code paths already correct. - New: add MLPS_CHECKPOINT_MP_START_METHOD (introduced by PR #700 via main) to MANPAGE_ENV_VAR_TIERS and ManPage.md Owned tier to satisfy the automated env-var/ManPage sync test.
Descriptive-only clause with no enforceable checker contract; not present in origin/main. Resolves rules-coverage CI failure (unmapped rule 2.1.28).
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
1 participant
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
This PR delivers Milestone v1.1 (Phases 5–7.5): a complete overhaul of environment variable naming, score aggregation, and submitter-facing documentation.
Phase 5 — Env-var consolidation & loud errors: Hard-rename all
MLPERF_*env vars toMLPSTORAGE_*; addENV_FALLBACK_*resolver functions inconfig.py; replace silent failures with loud parse-time errors that name both the CLI flag and the env var (verbatim D-02 template); drop the tempdir fallback; rename run_summary Environment rows.Phase 6 — Score aggregation: Implement the long-standing
_process_workload_groupsTODO with per-benchmark aggregation math (training: fmean of 5 non-warmup invocations per Rules.md §2.1.17; checkpointing: fmean of 10-op list per §2.1.23; VDB/KVCache: pass-through of pre-computed aggregates). Rewritegenerate_reports()as a bottom-up build emitting per-modelresults.{csv,json}files before the global summary. Fixed 6-column CSV prefix with groupedtrain_*/checkpoint_*/vdb_*/kvcache_*ordering. Add Rules.md §2.1.28aggregateResultsFileclause.Phase 7 — ManPage env-var documentation: Rewrite the
ENVIRONMENTsection ofManPage.mdas six per-tier markdown tables (owned,mpi-borrowed,aws-borrowed,storage-borrowed,diagnostic,internal-write). AddMANPAGE_ENV_VAR_TIERSdict and_MANPAGE_SYNC_ALLOWLISTfrozenset toconfig.py. Addtests/unit/test_env_var_manpage_sync.py— an automated inventory-sync test that enforces the symmetric-difference invariant between env vars read by the code and vars documented in ManPage.Phase 7.5 — s3dlio-side env-var documentation: Add a 7th
storage-backendtier documenting 15 HIGH-risk and 7 MEDIUM-riskS3DLIO_*vars consumed by the s3dlio Rust binary (not Python code), plus 3 missing AWS vars. AddMANPAGE_STORAGE_BACKEND_ENV_VARSand_S3DLIO_HIGH_RISK_ENV_VARSfrozensets andS3DLIO_PINNED_VERSIONconstant toconfig.py. Add_check_storage_backend_env()pre-flight warning inBenchmarkbase class. AddstorageBackendEnvVarDeclarationresult-validity clause to Rules.md §1.2. Extend sync test withTestStorageBackendEnvVars.Test plan
uv run pytest tests/unit -qpasses (2585 passed, 1 skipped verified locally)uv run pytest tests/unit/test_env_var_manpage_sync.py -v— sync test enforces ManPage/code invariantuv run pytest tests/unit/test_aggregation.py -v— aggregation regression contractsuv run pytest tests/unit/test_reporting.py -v— per-model rollup file placementuv run pytest tests/unit/test_reportgen_output_shape.py -v— emitted-file shape contractsmlpstorage training runwith a HIGH-riskS3DLIO_*var set — confirm WARNING emitted to stderr--results-dirwith no env var set — confirm loud error names both--results-dir/-rdandMLPSTORAGE_RESULTS_DIR