diff --git a/ManPage.md b/ManPage.md index 4f834d4d..97ec7efb 100644 --- a/ManPage.md +++ b/ManPage.md @@ -29,7 +29,7 @@ Where: - `` is `datasize`, `datagen`, `run`, or `configview` (subset depending on benchmark) - `` is `file` or `object` — required by `datagen`, `run`, and `configview` for the benchmarks that touch storage - `` is the submitter / organization name pinned to the results-dir by `mlpstorage init`; `[A-Za-z0-9._-]+`, case-sensitive -- `` (for `--systemname`) is the per-run system-under-test identifier; required on every emitting subcommand (`run`, `datagen`, `configview`, `reportgen`, `history`), and may be supplied via the `MLPERF_SYSTEMNAME` environment variable +- `` (for `--systemname`) is the per-run system-under-test identifier; required on every emitting subcommand (`run`, `datagen`, `configview`, `reportgen`, `history`), and may be supplied via the `MLPSTORAGE_SYSTEMNAME` environment variable Before any emitting subcommand can run, the `` must be initialized with `mlpstorage init`. The single bootstrap command `mlpstorage init ` writes a `mlperf-results.yaml` sentinel that pins orgname to the directory; every later non-init command reads it as authoritative. @@ -77,8 +77,8 @@ Mechanisms used: - **Benchmark and model as positionals.** Only models valid for the chosen mode appear as subparsers. A user cannot type `mlpstorage closed training cosmoflow ...` because `cosmoflow` only exists under `whatif`. - **Command as a positional.** `datasize`, `datagen`, `run`, and `configview` are distinct subparsers, so each command sees only the flags relevant to it. `datasize` does not accept storage-access flags; `datagen` and `run` do. - **Storage protocol as a positional.** Commands that touch storage require `file` or `object` as a positional after the command name, making the access path explicit at the call site and visible in command history. -- **Orgname pinned to the results-dir by `mlpstorage init`.** There is no `--orgname` flag on any benchmark subcommand and no `MLPERF_ORGNAME` environment variable consulted by non-init commands. The results-dir is initialized exactly once with `mlpstorage init `, which atomically writes a `mlperf-results.yaml` sentinel. Every later command reads the sentinel; emitting subcommands invoked against an un-initialized results-dir refuse with a `ConfigurationError` directing the submitter to run `init` first. Re-initializing the same directory with the same orgname is idempotent; supplying a different orgname raises `DoubleInitError` rather than silently overwriting. -- **Systemname is per-run.** The `--systemname`/`-sn` flag (defaulting to `$MLPERF_SYSTEMNAME` when set) is required on every emitting subcommand. Because each run names its own system, the same results-dir can host runs from multiple system-under-test configurations without cross-contamination. +- **Orgname pinned to the results-dir by `mlpstorage init`.** There is no `--orgname` flag on any benchmark subcommand and no `MLPSTORAGE_ORGNAME` environment variable consulted by non-init commands. The results-dir is initialized exactly once with `mlpstorage init `, which atomically writes a `mlperf-results.yaml` sentinel. Every later command reads the sentinel; emitting subcommands invoked against an un-initialized results-dir refuse with a `ConfigurationError` directing the submitter to run `init` first. Re-initializing the same directory with the same orgname is idempotent; supplying a different orgname raises `DoubleInitError` rather than silently overwriting. +- **Systemname is per-run.** The `--systemname`/`-sn` flag (defaulting to `$MLPSTORAGE_SYSTEMNAME` when set) is required on every emitting subcommand. Because each run names its own system, the same results-dir can host runs from multiple system-under-test configurations without cross-contamination. - **Mutually exclusive groups.** For example, VectorDB's `--runtime` and `--queries` are wired into an `add_mutually_exclusive_group()`, so only one can be supplied. - **Pinned defaults in closed.** Closed kvcache pins `--gpu-mem-gb`, `--cpu-mem-gb`, `--duration`, `--trials`, `--seed`, `--rag-num-docs`, and several boolean knobs to their rules-mandated values, with no flag exposed to change them. Closed training/checkpointing/vectordb pin `--loops=1`, an empty `--params`, and `--allow-invalid-params=False` as internal defaults (the flags themselves are not registered on the closed parsers). - **Post-parse validators.** What argparse cannot express (for example, "`--num-checkpoints-write` must be 10 or 0 in closed per Rules §4.7.1") is enforced by `validate__arguments()` functions called immediately after parsing. @@ -147,10 +147,10 @@ Run `mlpstorage init ` first. ### Systemname resolution -`--systemname ` / `-sn ` is required on every emitting subcommand (`run`, `datagen`, `configview`, `reportgen`, `history rerun`, etc.). Resolution priority is: +`--systemname ` / `-sn ` is required on every emitting subcommand (`run`, `datagen`, `configview`, `history rerun`, etc.). Resolution priority is: 1. The CLI flag if supplied. -2. The `MLPERF_SYSTEMNAME` environment variable. +2. The `MLPSTORAGE_SYSTEMNAME` environment variable. 3. Otherwise empty string (which fails the required-on-emitting-commands check, surfacing as a parser error). Because systemname is per-run, the same results-dir can host runs from many different systems-under-test. The canonical results path includes both `` (from sentinel) and `` (from CLI/env) so cross-system results never collide. @@ -261,7 +261,7 @@ VectorDB does not use `--data-dir`; vectors are loaded directly into the databas ## RESULTS DIRECTORY (`--results-dir`) -The results directory accumulates every artifact produced by `mlpstorage` as each new invocation of `mlpstorage` executes. The default is `$MLPERF_RESULTS_DIR` if set, otherwise it must be supplied explicitly. Its layout follows the canonical Rules.md §2.1 shape from the moment `mlpstorage init` creates the sentinel: +The results directory accumulates every artifact produced by `mlpstorage` as each new invocation of `mlpstorage` executes. The default is `$MLPSTORAGE_RESULTS_DIR` if set, otherwise it must be supplied explicitly. Its layout follows the canonical Rules.md §2.1 shape from the moment `mlpstorage init` creates the sentinel: ``` / @@ -271,7 +271,7 @@ The results directory accumulates every artifact produced by `mlpstorage` as eac │ ├── systems/ │ │ └── .yaml auto-generated on first `run`; see SYSTEM DESCRIPTION │ └── results/ -│ └── / from --systemname / MLPERF_SYSTEMNAME +│ └── / from --systemname / MLPSTORAGE_SYSTEMNAME │ └── ``` @@ -373,6 +373,12 @@ Every benchmark run writes: - **`results.json`** — aggregated summary across all timestamped run directories, used by `reportgen`. - **Command history** is appended to `/.history/` (consumed by `mlpstorage history`). +### Aggregate Interpretation + +Aggregate columns are informational and NOT a leaderboard-input contract. External ranking pipelines MUST compute their own aggregates from per-invocation summary.json files. + +This framing applies uniformly to `train_mean_of_*`, `checkpoint_mean_of_*`, `vdb_*` pass-through, and `kvcache_aggregated_*` / `kvcache_option_*` columns emitted by `reportgen`. See the Rules.md §2.1.28 mechanical-shape specification for the aggregation math. + ## VALIDATOR `mlpstorage` ships a layered validation system whose ultimate authority is `Rules.md` in the repository root. @@ -467,10 +473,10 @@ The `init` subcommand takes no flags — universal flags such as `--results-dir` ### Universal options (every non-init command) - **`--results-dir `, `-rd `** - Root directory for all written artifacts. Required for any command that writes results. Defaults to `$MLPERF_RESULTS_DIR` if set. Must already be initialized with `mlpstorage init`; commands that consult the orgname-resolution gate refuse to run otherwise. + Root directory for all written artifacts. Required for any command that writes results. Defaults to `$MLPSTORAGE_RESULTS_DIR` if set. Must already be initialized with `mlpstorage init`; commands that consult the orgname-resolution gate refuse to run otherwise. - **`--systemname `, `-sn `** - System-under-test identifier for the current run. Required on every emitting subcommand (`run`, `datagen`, `configview`, `reportgen`, `history rerun`). Defaults to `$MLPERF_SYSTEMNAME`. Each mode (closed/open/whatif) owns its own `.yaml` under the per-mode `systems/` directory, so the same name across modes is fine. + System-under-test identifier for the current run. Required on every emitting subcommand (`run`, `datagen`, `configview`, `history rerun`). Defaults to `$MLPSTORAGE_SYSTEMNAME`. Each mode (closed/open/whatif) owns its own `.yaml` under the per-mode `systems/` directory, so the same name across modes is fine. See the Reports subsection for reportgen's optional-systemname multi-system-fallback behavior. - **`--config-file `, `-c `** YAML file of argument overrides merged in *after* CLI parsing. Useful for keeping repeatable closed-submission knob settings in one place. @@ -843,8 +849,8 @@ mlpstorage reports reportgen --results-dir --systemname - **`--results-dir `, `-rd `** (required) Results tree to summarize. Accepts either a flat benchmark-type root (legacy) or a canonical sentinel-bearing submission root; when the sentinel is detected, `reportgen` scopes to `///results//` and walks only that slice, so a single results-dir hosting runs from multiple systems does not have its runs mashed into one report. -- **`--systemname `, `-sn `** (required) - System-under-test identifier. Under the canonical tree this pins reportgen to a single `results//` slice; under a flat tree it tags the emitted report. Defaults to `$MLPERF_SYSTEMNAME` as everywhere else. +- **`--systemname `, `-sn `** (optional) + System-under-test identifier. Under the canonical tree this pins reportgen to a single `results//` slice; under a flat tree it tags the emitted report. Defaults to `$MLPSTORAGE_SYSTEMNAME` as everywhere else. When omitted, reportgen operates on the entire results-dir tree and derives systemname per row from the workload dir's path segment (open//results//... or checkpointing/training/vdb/kvcache analog). `--output-dir` was removed in PR #617. The rollup outputs must land inside the submission tree so submitters cannot accidentally exclude the summary from what MLCommons reviews. @@ -925,12 +931,99 @@ Reports which Rules.md IDs are referenced by `@rule(rule_id=...)`-decorated chec ## ENVIRONMENT -- **`MLPERF_RESULTS_DIR`** — default value for `--results-dir` when the flag is not supplied. The path must still have been initialized with `mlpstorage init`. -- **`MLPERF_SYSTEMNAME`** — default value for `--systemname` / `-sn` when the flag is not supplied. Emitting subcommands require systemname to be set via flag or env; an empty value is rejected at parse time. -- **`MLPERF_DATA_DIR`** — fallback value for `--data-dir` for some commands. -- **`MPI_RUN_BIN`** — overrides the path used when invoking `mpirun`. - -There is intentionally **no `MLPERF_ORGNAME` environment variable** and no `--orgname` flag on benchmark subcommands. Orgname is sourced exclusively from the `mlperf-results.yaml` sentinel written by `mlpstorage init`. +This section enumerates every environment variable mlpstorage reads or borrows, grouped by ownership tier. Seven tiers: Owned (mlpstorage authors the name), MPI-borrowed / AWS-borrowed / Storage-borrowed (third-party contracts mlpstorage consumes), Storage-backend (s3dlio Rust layer, not Python code), Diagnostic (mlpstorage internal reads surfaced for troubleshooting), and Internal-write (mlpstorage may overwrite user-set values during execution). An automated sync test (`tests/unit/test_env_var_manpage_sync.py`) asserts this section stays symmetric with the actual `os.environ` reads in the codebase. + +### Owned + +| Env var | Read by | Default when unset | Notes | +|---|---|---|---| +| `MLPSTORAGE_RESULTS_DIR` | all emitting subcommands | none — CLI fails at parse time with loud error (see Phase 5 D-02 template) | Required on every emitting subcommand; path must already be initialized with `mlpstorage init`. | +| `MLPSTORAGE_SYSTEMNAME` | all emitting subcommands | none — CLI fails at parse time with loud error (see Phase 5 D-02 template) | Required on every emitting subcommand; per-run system-under-test identifier. | +| `MLPSTORAGE_DATA_DIR` | all emitting subcommands | [not set] | Optional fallback for `--data-dir`; if unset the flag must be supplied explicitly. | +| `MLPSTORAGE_CHECKPOINT_FOLDER` | all emitting subcommands | [not set] | Optional fallback for `--checkpoint-folder`; if unset the flag must be supplied explicitly. | +| `MLPSTORAGE_ORGNAME` | `mlpstorage init` at init time | [not set] | Read only by `init` subcommand; all other commands source orgname from the `mlperf-results.yaml` sentinel. | +| `MLPSTORAGE_CHECKPOINT_URI_SCHEME` | `mlpstorage_py/checkpointing/storage_writers/__init__.py:44` (via `CHECKPOINT_URI_SCHEME_ENV` constant) | [not set] | Selects checkpoint storage backend (`s3`, `file`, etc.). `[internal-write]` — also written by `mlpstorage_py/benchmarks/dlio.py` during checkpointing setup. | +| `KVCACHE_SELECTED_WORKLOADS` | `kv-cache-wrapper.sh` (shell dispatch layer); displayed by `run_summary.py:546` | [not set] | `[shell-wrapper-read]` — filters which kvcache workloads run; unset = run all. Functionally owned by mlpstorage; only the shell wrapper reads it. | +| `MLPS_CHECKPOINT_MP_START_METHOD` | `mlpstorage_py/checkpointing/streaming_checkpoint.py` (`MP_START_METHOD_ENV` constant) | [not set] — backend-aware default used (`fork` for POSIX file, `forkserver` for object-storage) | Overrides the multiprocessing start method for the streaming checkpoint writer subprocess. Valid values: `auto`, `fork`, `forkserver`, `spawn`. `fork` is refused on object-storage paths (deadlocks Tokio runtime, #642); constructor `mp_start_method` arg wins if both are set. | + +### MPI-borrowed + +| Env var | Read by | Default when unset | Notes | +|---|---|---|---| +| `MPI_RUN_BIN` | `mlpstorage_py/utils.py` (MPI launcher resolution) | [not set] | Overrides the `mpirun` binary path; if unset mlpstorage searches `PATH` for `mpirun`. | +| `MPI_EXEC_BIN` | `mlpstorage_py/utils.py` (MPI launcher resolution) | [not set] | Alternative override for `mpiexec`; consulted when `MPI_RUN_BIN` is also unset. | +| `OMPI_COMM_WORLD_RANK` | `mlpstorage_py/cluster_collector.py` (rank detection) | [not set] | OpenMPI-injected rank index; read to identify rank-0 for sidecar writes. | +| `PMI_RANK` | `mlpstorage_py/cluster_collector.py` (rank detection) | [not set] | PMI-injected rank index; fallback when `OMPI_COMM_WORLD_RANK` is absent (e.g. MPICH-derived launchers). | + +### AWS-borrowed + +| Env var | Read by | Default when unset | Notes | +|---|---|---|---| +| `AWS_ACCESS_KEY_ID` | boto3/s3dlio via storage backend init | [not set] | Read by boto3/s3dlio via storage backend init; consumed as-is. | +| `AWS_SECRET_ACCESS_KEY` | boto3/s3dlio via storage backend init | [not set] | Read by boto3/s3dlio via storage backend init; consumed as-is. | +| `AWS_REGION` | `mlpstorage_py/storage_config.py` | `'us-east-1'` | AWS region for S3 endpoint selection; defaults to `us-east-1` when unset. | +| `AWS_CA_BUNDLE` | `mlpstorage_py/storage_config.py` | [not set] | Path to a custom CA bundle for TLS verification; unset uses the system default. | +| `AWS_ENDPOINT_URL` | `mlpstorage_py/storage_config.py`; `mlpstorage_py/checkpointing/storage_writers/s3dlio_writer.py:168` | [not set] | Custom S3-compatible endpoint URL. `[cross-ref → internal-write]` — also written at `s3dlio_writer.py:168` during multi-endpoint selection; see Internal-write cross-refs below. | +| `AWS_SESSION_TOKEN` | boto3/s3dlio via storage backend init | [not set] | Temporary STS session token; consumed by boto3/s3dlio via storage backend init. | +| `AWS_DEFAULT_REGION` | boto3/s3dlio via storage backend init | [not set] | Fallback region when `AWS_REGION` is unset; consumed by boto3/s3dlio. | +| `AWS_S3_ADDRESSING_STYLE` | boto3/s3dlio via storage backend init | [not set] | Controls virtual-hosted vs. path-style S3 addressing; consumed by boto3/s3dlio. | + +### Storage-borrowed + +| Env var | Read by | Default when unset | Notes | +|---|---|---|---| +| `BUCKET` | `mlpstorage_py/storage_config.py` | [not set] | S3 bucket name for object-storage runs; required when `--storage object` is selected. | +| `STORAGE_LIBRARY` | `mlpstorage_py/storage_config.py` | `'s3dlio'` | Storage client library selection; defaults to `s3dlio`. | +| `STORAGE_URI_SCHEME` | `mlpstorage_py/storage_config.py` | `'s3'` | URI scheme for storage paths; defaults to `s3`. | +| `S3_LOAD_BALANCE_STRATEGY` | `mlpstorage_py/storage_config.py` | `'round_robin'` | Load-balance strategy across S3 endpoints; defaults to `round_robin`. | +| `S3_ENDPOINT_URIS` | `mlpstorage_py/checkpointing/storage_writers/s3dlio_writer.py:44` | [not set] | Space-separated list of S3 endpoint URIs; part of the s3dlio endpoint fallback chain per D-08. | +| `S3_ENDPOINT_TEMPLATE` | `mlpstorage_py/checkpointing/storage_writers/s3dlio_writer.py` | [not set] | URI template for S3 endpoint construction; s3dlio endpoint fallback chain per D-08. | +| `S3_ENDPOINT_FILE` | `mlpstorage_py/checkpointing/storage_writers/s3dlio_writer.py` | [not set] | Path to a file containing S3 endpoint URIs; s3dlio endpoint fallback chain per D-08. | +| `S3_ENDPOINT` | `mlpstorage_py/checkpointing/storage_writers/s3dlio_writer.py` | [not set] | Single S3 endpoint URI; lowest-priority entry in the s3dlio endpoint fallback chain per D-08. | + +### Storage-backend + +The following table is anchored on s3dlio v0.9.106. The defaults for `S3DLIO_PUT_VERIFY` and `S3DLIO_MPU_PUT_VERIFY` changed in this release (from `true` to `false`); behavior may differ on earlier versions. For cloud-specific credential and endpoint env vars consumed by s3dlio for Azure or GCS backends, see s3dlio's [Environment_Variables.md](https://github.com/mlcommons/s3dlio/blob/main/docs/Environment_Variables.md). + +| Env var | Read by | Default when unset | Notes | +|---|---|---|---| +| `S3DLIO_SKIP_HEAD` | s3dlio Rust binary (not mlpstorage_py) | `false` | Skips HEAD requests for object existence checks. [high-risk: alters measured benchmark behavior] Enabling this removes per-read existence verification; can significantly increase throughput by reducing API call overhead. | +| `S3DLIO_ENABLE_RANGE_OPTIMIZATION` | s3dlio Rust binary (not mlpstorage_py) | `false` | Enables multi-range read coalescing. [high-risk: alters measured benchmark behavior] Can substantially change read throughput for workloads with many small reads. | +| `S3DLIO_RANGE_THRESHOLD_MB` | s3dlio Rust binary (not mlpstorage_py) | `8` | Minimum size threshold (MB) for range-read coalescing. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_RANGE_CONCURRENCY` | s3dlio Rust binary (not mlpstorage_py) | `4` | Number of concurrent range-read sub-requests. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_CHUNK_SIZE` | s3dlio Rust binary (not mlpstorage_py) | `8388608` | Read/write chunk size in bytes. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_H2C` | s3dlio Rust binary (not mlpstorage_py) | `false` | Enables HTTP/2 cleartext (H2C) transport. [high-risk: alters measured benchmark behavior] Switching transport protocol changes connection multiplexing and can alter throughput substantially. | +| `S3DLIO_H2_ADAPTIVE_WINDOW` | s3dlio Rust binary (not mlpstorage_py) | `false` | Enables HTTP/2 adaptive flow-control window. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_H2_STREAM_WINDOW_MB` | s3dlio Rust binary (not mlpstorage_py) | `1` | HTTP/2 per-stream flow-control window size (MB). [high-risk: alters measured benchmark behavior] | +| `S3DLIO_H2_CONN_WINDOW_MB` | s3dlio Rust binary (not mlpstorage_py) | `1` | HTTP/2 per-connection flow-control window size (MB). [high-risk: alters measured benchmark behavior] | +| `S3DLIO_POOL_MAX_IDLE_PER_HOST` | s3dlio Rust binary (not mlpstorage_py) | `10` | Maximum idle connections per host in the connection pool. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_POOL_IDLE_TIMEOUT_SECS` | s3dlio Rust binary (not mlpstorage_py) | `90` | Time (seconds) before idle connections are evicted. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_PUT_VERIFY` | s3dlio Rust binary (not mlpstorage_py) | `false` | Enables checksum verification on PUT operations. Changed from `true` to `false` in v0.9.106. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_MPU_PUT_VERIFY` | s3dlio Rust binary (not mlpstorage_py) | `false` | Enables checksum verification on multipart PUT operations. Changed from `true` to `false` in v0.9.106. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_MULTIPART_THRESHOLD_MB` | s3dlio Rust binary (not mlpstorage_py) | `64` | Object size threshold (MB) above which multipart upload is used. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_RT_THREADS` | s3dlio Rust binary (not mlpstorage_py) | `1` | Number of Tokio runtime threads. [high-risk: alters measured benchmark behavior] | +| `S3DLIO_MAX_RETRY_ATTEMPTS` | s3dlio Rust binary (not mlpstorage_py) | `3` | Maximum number of retry attempts on transient errors. [medium-risk: affects run stability, not throughput measurement] | +| `S3DLIO_CONNECT_TIMEOUT_SECS` | s3dlio Rust binary (not mlpstorage_py) | `30` | TCP connection timeout in seconds. [medium-risk: affects run stability, not throughput measurement] | +| `S3DLIO_OPERATION_TIMEOUT_SECS` | s3dlio Rust binary (not mlpstorage_py) | `300` | Per-operation timeout in seconds. [medium-risk: affects run stability, not throughput measurement] | +| `S3DLIO_PUT_MAX_RETRIES` | s3dlio Rust binary (not mlpstorage_py) | `3` | Maximum retries for PUT operations. [medium-risk: affects run stability, not throughput measurement] | +| `S3DLIO_PUT_RETRY_DELAY_MS` | s3dlio Rust binary (not mlpstorage_py) | `100` | Delay (ms) between PUT retries. [medium-risk: affects run stability, not throughput measurement] | +| `S3DLIO_MPU_MAX_RETRIES` | s3dlio Rust binary (not mlpstorage_py) | `3` | Maximum retries for multipart upload operations. [medium-risk: affects run stability, not throughput measurement] | +| `S3DLIO_MPU_RETRY_DELAY_S` | s3dlio Rust binary (not mlpstorage_py) | `1` | Delay (seconds) between multipart upload retries. [medium-risk: affects run stability, not throughput measurement] | + +### Diagnostic + +No environment variables are diagnostic-only under the current inventory (2026-07-04). This tier header is preserved so future diagnostic env vars have a home; the sync test in `tests/unit/test_env_var_manpage_sync.py` allows this tier to be empty. + +### Internal-write + +| Env var | Read by | Default when unset | Notes | +|---|---|---|---| +| `DLIO_DROP_CACHES_TIMEOUT` | `mlpstorage_py/benchmarks/dlio.py:629` (write), `:602` (read via `in os.environ`) | [not set] | mlpstorage writes this var before invoking DLIO to communicate the drop-caches timeout; also read to detect whether it has already been set. | + +- **Cross-ref:** `AWS_ENDPOINT_URL` is documented as AWS-borrowed (primary tier); mlpstorage overwrites it at `s3dlio_writer.py:168` during multi-endpoint selection. Not a duplicate `MANPAGE_ENV_VAR_TIERS` entry per D-13. +- **Cross-ref:** `MLPSTORAGE_CHECKPOINT_URI_SCHEME` is documented as Owned (primary tier); mlpstorage writes it at `mlpstorage_py/benchmarks/dlio.py` during checkpointing setup. Not a duplicate `MANPAGE_ENV_VAR_TIERS` entry per D-12/D-13. + +There is intentionally no `MLPSTORAGE_ORGNAME` fallback consulted by non-init commands — orgname is sourced exclusively from the `mlperf-results.yaml` sentinel written by `mlpstorage init`. The row in the Owned table above documents where `mlpstorage init` reads the flag, not a runtime fallback path. ## EXIT STATUS @@ -963,7 +1056,7 @@ mlpstorage init Acme /mnt/results Size, generate, and run UNet3D in closed mode against a POSIX storage target: ``` -export MLPERF_SYSTEMNAME=acme-prod-v1 +export MLPSTORAGE_SYSTEMNAME=acme-prod-v1 mlpstorage closed training unet3d datasize \ --accelerator-type b200 --max-accelerators 8 \ diff --git a/Rules.md b/Rules.md index 1a69e844..e0172ea4 100644 --- a/Rules.md +++ b/Rules.md @@ -54,6 +54,10 @@ The `mlpstorage` tool must be used to run the benchmarks, submitters are not all 1.1. **mlpstorageGeneratesHierarchy** -- The `mlpstorage` command must obtain (somehow) the pathname of the output file directory hierarchy and directly create and/or append to the files within that hierarchy to successively build out the submission folder. We don't want the submitter to manually create anything in that hierarchy except for the SystemDescription.* files (if we can help it). +1.2. **storageBackendEnvVarDeclaration** -- Submitters MUST NOT set any documented s3dlio-side env var (§ENVIRONMENT → Storage-backend in ManPage.md) during the timed run unless the run's configuration file explicitly declares it. Results from runs with undeclared storage-backend env vars MUST be submitted as OPEN category. mlpstorage warns at run start when HIGH-risk vars are detected. + +For cloud-specific credential and endpoint env vars consumed by s3dlio for Azure or GCS backends, see s3dlio's [Environment_Variables.md](https://github.com/mlcommons/s3dlio/blob/main/docs/Environment_Variables.md). + # 2. Core/Common Rules for All Submissions ## 2.1. Core/Common POSIX API Rules @@ -666,7 +670,7 @@ is better), and the **aggregate Storage Throughput in tokens/s**. A run is launched with `mlpstorage kvcache run [OPTIONS]` (the division is the first positional, post-PR #412 modal CLI). The results directory must first be initialised once with `mlpstorage init -`, and every run requires `--systemname` (or the `MLPERF_SYSTEMNAME` +`, and every run requires `--systemname` (or the `MLPSTORAGE_SYSTEMNAME` environment variable). The command executes **all three Options sequentially**, each repeated `trials` times, by prefixing `mpirun` to `mlperf_wrapper.py`. The Option's parameters are built by `mlpstorage` (`_build_option_kvcache_args`) and diff --git a/mlpstorage_py/benchmarks/base.py b/mlpstorage_py/benchmarks/base.py index 911b6d8f..1eac8637 100755 --- a/mlpstorage_py/benchmarks/base.py +++ b/mlpstorage_py/benchmarks/base.py @@ -46,7 +46,13 @@ def _run(self): from functools import wraps -from mlpstorage_py.config import PARAM_VALIDATION, DATETIME_STR, MLPS_DEBUG, EXEC_TYPE +from mlpstorage_py.config import ( + PARAM_VALIDATION, + DATETIME_STR, + MLPS_DEBUG, + EXEC_TYPE, + _S3DLIO_HIGH_RISK_ENV_VARS, +) from mlpstorage_py.errors import ConfigurationError, ErrorCode, FileSystemError from mlpstorage_py.run_directory import ( DEFAULT_COLLISION_BUMP_BUDGET, @@ -991,6 +997,30 @@ def _fs_separation_paths(self) -> Optional[tuple]: f"_fs_separation_paths() for CAP-03" ) + def _check_storage_backend_env(self) -> None: + """Warn when HIGH-risk s3dlio env vars are set (Phase 7.5 D-07, D-08). + + Checks ``os.environ`` for entries in ``_S3DLIO_HIGH_RISK_ENV_VARS`` + and emits a WARNING for each one found, but only when the active + storage library is s3dlio (``STORAGE_LIBRARY == 's3dlio'``). + + Rationale for the STORAGE_LIBRARY guard (D-07): training-only + deployments that never use s3dlio should not see s3dlio warnings. + The guard avoids false positives without requiring per-var allowlists. + + The warning uses the D-08 verbatim template so that automated tests + (test_d08_warning_prefix_in_benchmark_source) can lock the wording. + """ + if os.environ.get('STORAGE_LIBRARY') != 's3dlio': + return + for var_name in sorted(_S3DLIO_HIGH_RISK_ENV_VARS): + if var_name in os.environ: + self.logger.warning( + f"s3dlio env var '{var_name}' is set — this may alter measured benchmark results. " + "Declare it in your config or submit as OPEN category. " + "See ManPage §ENVIRONMENT → Storage-backend for impact." + ) + def _pre_execution_gate(self) -> None: """Run all pre-execution capacity/environment gates (CAP-01 in Slice 3; CAP-02 shared-FS verification is appended in Slice 4 of @@ -1066,6 +1096,8 @@ def _pre_execution_gate(self) -> None: # design decision D-601-1). --skip-fs-separation-gate bypasses the # raise but still writes the sidecar so the validator gets telemetry. self._run_fs_separation_probe() + # Phase 7.5 D-07: warn when HIGH-risk s3dlio env vars are set. + self._check_storage_backend_env() def _run_fs_separation_probe(self) -> None: """Run the CAP-03 FS-separation probe and write the sidecar. diff --git a/mlpstorage_py/cli/checkpointing_args.py b/mlpstorage_py/cli/checkpointing_args.py index 9fcc7f43..f8d9414e 100755 --- a/mlpstorage_py/cli/checkpointing_args.py +++ b/mlpstorage_py/cli/checkpointing_args.py @@ -7,7 +7,10 @@ import sys -from mlpstorage_py.config import DEFAULT_HOSTS, EXEC_TYPE, LLM_MODELS, LLM_MODELS_CLOSED, EXIT_CODE +from mlpstorage_py.config import ( + DEFAULT_HOSTS, EXEC_TYPE, LLM_MODELS, LLM_MODELS_CLOSED, EXIT_CODE, + ENV_FALLBACK_CHECKPOINT_FOLDER, +) from mlpstorage_py.cli.common_args import ( HELP_MESSAGES, add_universal_arguments, @@ -137,14 +140,27 @@ def _add_checkpointing_core_args(parser, command): ), ) - # Checkpoint folder required for run only + # Checkpoint folder required for run only. The argparse-level required=True + # was replaced by post-parse enforcement (Phase 5 D-08 / D-09 / ENV-06) so + # the ENV_FALLBACK_CHECKPOINT_FOLDER default (sourced from + # MLPSTORAGE_CHECKPOINT_FOLDER env var) can satisfy the requirement. The + # loud-error gate lives in cli_parser._check_universal_required_present + # keyed off the _mlps_req_checkpoint_folder marker we set below. The + # --checkpoint-folder argument stays defined ONLY here (never in + # common_args.py) so no other benchmark inherits it. if command == "run": parser.add_argument( '--checkpoint-folder', '-cf', type=str, - required=True, + default=ENV_FALLBACK_CHECKPOINT_FOLDER, help=HELP_MESSAGES['checkpoint_folder'] ) + # Plumb the post-parse-gate marker locally, next to the arg definition, + # so the flag and its enforcement gate stay co-located (D-09). The + # symmetric req_checkpoint_folder kwarg on add_universal_arguments + # exists for defensive future callers; the checkpointing flow uses + # this local set_defaults. + parser.set_defaults(_mlps_req_checkpoint_folder=True) # num-checkpoints-read/write are available in all modes so closed submitters # can split write and read into two invocations (set =0 on one side) with a diff --git a/mlpstorage_py/cli/common_args.py b/mlpstorage_py/cli/common_args.py index caf6572a..4d8492c7 100755 --- a/mlpstorage_py/cli/common_args.py +++ b/mlpstorage_py/cli/common_args.py @@ -12,7 +12,7 @@ from mlpstorage_py.config import ( CHECKPOINT_RANKS_STRINGS, MODELS, ACCELERATORS, ACCELERATORS_CLOSED, DEFAULT_HOSTS, - LLM_MODELS_STRINGS, MPI_CMDS, EXEC_TYPE, DEFAULT_RESULTS_DIR, DEFAULT_SYSTEMNAME, + LLM_MODELS_STRINGS, MPI_CMDS, EXEC_TYPE, ENV_FALLBACK_RESULTS_DIR, ENV_FALLBACK_SYSTEMNAME, VECTOR_DTYPES, DISTRIBUTIONS ) @@ -129,7 +129,10 @@ def add_arguments(self, actions): 'reportgen': "Generate a report from the benchmark results.", # Checkpoint folder is used for training and checkpointing - 'checkpoint_folder': "Location for checkpoint files for training or checkpointing workloads", + 'checkpoint_folder': ( + "Location for checkpoint files for training or checkpointing workloads. " + "Defaults to MLPSTORAGE_CHECKPOINT_FOLDER env var if set." + ), # Checkpointing help messages 'checkpoint_run': "The checkpoint command executes checkpoint saves and restores for a given model.", @@ -186,7 +189,7 @@ def add_arguments(self, actions): # System-under-test name (LAY-04 / D-10) — folder under results/ in canonical layout 'systemname': ( "System-under-test name (folder under results/). " - "Defaults to MLPERF_SYSTEMNAME env var if set." + "Defaults to MLPSTORAGE_SYSTEMNAME env var if set." ), # MPI help messages @@ -219,15 +222,15 @@ def add_arguments(self, actions): } -def add_universal_arguments(parser, req_results, req_systemname=False): +def add_universal_arguments(parser, req_results, req_systemname=False, req_checkpoint_folder=False): """Add arguments common to all benchmarks and commands. Args: parser: Argparse parser to add arguments to. req_results: Whether --results-dir is required. The requirement is enforced POST-PARSE (see ``check_universal_arguments_present``) - so the ``MLPERF_RESULTS_DIR`` env-var-sourced default counts as - satisfying it. Using ``required=True`` here would short-circuit + so the ``MLPSTORAGE_RESULTS_DIR`` env-var-sourced default counts + as satisfying it. Using ``required=True`` here would short-circuit argparse before the env-var default applies — that's the CR-02 bug we fixed. req_systemname: Whether --systemname is required. Defaults to False @@ -235,6 +238,18 @@ def add_universal_arguments(parser, req_results, req_systemname=False): opting in. Emitting commands (datagen/run/configview/datasize/ reportgen/history) MUST opt in per CONTEXT.md D-10 / LAY-04. Same post-parse enforcement model as ``req_results``. + req_checkpoint_folder: Whether --checkpoint-folder is required for + this invocation. Defaults to False. When True, plumbs the + ``_mlps_req_checkpoint_folder`` marker onto the argparse namespace + via ``parser.set_defaults`` so the post-parse gate + (``cli_parser._check_universal_required_present``) can enforce + non-emptiness with the D-02 loud-error line. NOTE: the + ``--checkpoint-folder`` argument itself is registered in + ``mlpstorage_py/cli/checkpointing_args.py`` — NOT here — so no + other benchmark accidentally inherits it (see Phase 5 D-09). This + kwarg exists for symmetry with ``req_results`` / ``req_systemname`` + and for defensive future callers; the checkpointing flow plumbs + the marker locally in its own arg builder. """ standard_args = parser.add_argument_group("Standard Arguments") # NOTE on the env-var fallback (CR-02): argparse's ``required=True`` @@ -245,11 +260,12 @@ def add_universal_arguments(parser, req_results, req_systemname=False): # emitting subcommand. We drop ``required=True`` entirely and instead # stash a marker on the namespace (via an argparse.SUPPRESS-defaulted # action) that ``check_universal_arguments_present`` consumes at the - # post-parse validation layer to enforce non-emptiness. + # post-parse validation layer to enforce non-emptiness. Env-var names: + # MLPSTORAGE_RESULTS_DIR / MLPSTORAGE_SYSTEMNAME (see config.py D-10). standard_args.add_argument( '--results-dir', '-rd', type=str, - default=DEFAULT_RESULTS_DIR, + default=ENV_FALLBACK_RESULTS_DIR, help=HELP_MESSAGES['results_dir'] ) if req_results: @@ -260,19 +276,29 @@ def add_universal_arguments(parser, req_results, req_systemname=False): parser.set_defaults(_mlps_req_results=True) # --systemname: required on emitting commands (Rules.md §2.1.8 systemname - # subdir), optional elsewhere. Default = DEFAULT_SYSTEMNAME = "" unless - # MLPERF_SYSTEMNAME env var is set. Same env-var-fallback bug as above: - # we keep argparse's ``required=False`` and gate emptiness post-parse. + # subdir), optional elsewhere. Default sourced from ENV_FALLBACK_SYSTEMNAME + # = "" unless the MLPSTORAGE_SYSTEMNAME env var is set. Same env-var-fallback + # bug as above: we keep argparse's ``required=False`` and gate emptiness + # post-parse. standard_args.add_argument( '--systemname', '-sn', type=str, required=False, - default=DEFAULT_SYSTEMNAME, + default=ENV_FALLBACK_SYSTEMNAME, help=HELP_MESSAGES['systemname'] ) if req_systemname: parser.set_defaults(_mlps_req_systemname=True) + # --checkpoint-folder marker: per D-09, the `--checkpoint-folder` argument + # is registered in checkpointing_args.py (NOT here). This branch only plumbs + # the ``_mlps_req_checkpoint_folder`` namespace marker for callers that + # prefer symmetric opt-in via ``add_universal_arguments``. The checkpointing + # flow instead plumbs this marker locally next to its arg definition to keep + # the flag and its enforcement gate co-located. + if req_checkpoint_folder: + parser.set_defaults(_mlps_req_checkpoint_folder=True) + standard_args.add_argument( '--config-file', '-c', type=str, diff --git a/mlpstorage_py/cli/training_args.py b/mlpstorage_py/cli/training_args.py index 55a6ba7c..53a27bd6 100755 --- a/mlpstorage_py/cli/training_args.py +++ b/mlpstorage_py/cli/training_args.py @@ -6,11 +6,12 @@ """ import argparse +import os import sys from mlpstorage_py.config import ( MODELS, MODELS_CLOSED, MODELS_OPEN, ACCELERATORS, ACCELERATORS_CLOSED, - DEFAULT_HOSTS, EXEC_TYPE, EXIT_CODE + DEFAULT_HOSTS, EXEC_TYPE, EXIT_CODE, ENV_FALLBACK_DATA_DIR ) from mlpstorage_py.cli.common_args import ( @@ -171,9 +172,11 @@ def _add_training_core_args(parser, command, accel_choices): parser.add_argument( '--data-dir', '-dd', type=str, + default=ENV_FALLBACK_DATA_DIR, help=( "Dataset location. For file storage, this is a filesystem path. " - "For object storage, this is an object key prefix or full object URI." + "For object storage, this is an object key prefix or full object URI. " + "Defaults to MLPSTORAGE_DATA_DIR env var if set." ) ) @@ -293,14 +296,21 @@ def validate_training_arguments(args): # Object-mode --data-dir enforcement runs here (post-YAML) so --config-file # can populate data_dir for object workflows. File-mode enforcement happens - # earlier in cli_parser.parse_arguments via parser.error(). + # earlier in cli_parser.parse_arguments via parser.error(). Message text is + # the D-02 verbatim template (Phase 5 D-02 / D-07) — do NOT paraphrase; it + # is pinned by loud-error tests. if command in ('datagen', 'run') and protocol == 'object' and not getattr(args, 'data_dir', None): print( - f"ERROR: --data-dir is required for training {command} with object storage.\n" - " Specify --data-dir on the command line, or set\n" - " 'data_dir:' in the file passed via --config-file.", + f"error: --data-dir/-dd is required: pass it on the command line or set MLPSTORAGE_DATA_DIR", file=sys.stderr, ) + # D-05 migration hint: fires only when the user set the legacy env var + # but not the new one — actionable at exactly the moment of failure. + if os.environ.get('MLPERF_DATA_DIR') and not os.environ.get('MLPSTORAGE_DATA_DIR'): + print( + f"hint: MLPERF_DATA_DIR is set but is no longer read; rename it to MLPSTORAGE_DATA_DIR", + file=sys.stderr, + ) sys.exit(EXIT_CODE.INVALID_ARGUMENTS) if getattr(args, 'o_direct', False) and protocol != 'file': diff --git a/mlpstorage_py/cli_parser.py b/mlpstorage_py/cli_parser.py index f872ff41..15fee57a 100755 --- a/mlpstorage_py/cli_parser.py +++ b/mlpstorage_py/cli_parser.py @@ -6,12 +6,21 @@ """ import argparse +import os import re import shlex import sys from mlpstorage_py import VERSION -from mlpstorage_py.config import LLM_MODELS, VECTORDB_DEFAULT_RUNTIME, EXIT_CODE +from mlpstorage_py.config import ( + LLM_MODELS, + VECTORDB_DEFAULT_RUNTIME, + EXIT_CODE, + MLPSTORAGE_RESULTS_DIR_ENVVAR, + MLPSTORAGE_SYSTEMNAME_ENVVAR, + MLPSTORAGE_CHECKPOINT_FOLDER_ENVVAR, + _LEGACY_ENVVAR_MAP, +) # Import modular argument builders from cli package from mlpstorage_py.cli import ( @@ -37,6 +46,36 @@ prog_descriptions = PROGRAM_DESCRIPTIONS +# ----------------------------------------------------------------------------- +# _UNIVERSAL_REQUIRED_SPECS — declarative table for the parse-time +# required-universal gate (Plan 05-02 D-09). Each row describes one universal +# CLI flag that a subcommand may opt into as "required-with-env-fallback": +# +# (marker_attr_name, arg_attr_name, envvar_name_constant, +# long_flag, short_flag, name_for_template) +# +# - marker_attr_name: the ``args._mlps_req_*`` attribute set by +# ``add_universal_arguments(..., req_=True)`` on the opt-in subcommand. +# - arg_attr_name: the resolved argparse dest whose value we check for +# truthiness (env-var-sourced defaults populate this if the CLI flag was +# omitted). +# - envvar_name_constant: the ``MLPSTORAGE_*`` env-var-name string constant +# (single source of truth in ``mlpstorage_py.config``); also the lookup +# key into ``_LEGACY_ENVVAR_MAP`` for the D-05 migration hint. +# - long_flag / short_flag / name_for_template: substituted verbatim into +# the D-02 error template and D-05 hint template. +# +# Declaration order pins emission order (results, systemname, +# checkpoint-folder). Training's ``--data-dir`` is intentionally absent — +# per D-07 it gates in ``training_args.py`` after YAML merge. +# ----------------------------------------------------------------------------- +_UNIVERSAL_REQUIRED_SPECS = ( + ("_mlps_req_results", "results_dir", MLPSTORAGE_RESULTS_DIR_ENVVAR, "--results-dir", "-rd", "RESULTS_DIR"), + ("_mlps_req_systemname", "systemname", MLPSTORAGE_SYSTEMNAME_ENVVAR, "--systemname", "-sn", "SYSTEMNAME"), + ("_mlps_req_checkpoint_folder", "checkpoint_folder", MLPSTORAGE_CHECKPOINT_FOLDER_ENVVAR, "--checkpoint-folder", "-cf", "CHECKPOINT_FOLDER"), +) + + def _apply_formatter(parser): """Recursively set MLPStorageHelpFormatter on every parser in the subparser tree.""" parser.formatter_class = MLPStorageHelpFormatter @@ -285,38 +324,77 @@ def validate_args(args): def _check_universal_required_present(args): - """Enforce post-parse non-empty checks for --results-dir / --systemname. - - Subcommands opt in to "required" via ``add_universal_arguments(..., req_results=True)`` - and / or ``req_systemname=True``. That call seeds - ``args._mlps_req_results`` / ``args._mlps_req_systemname`` via - ``parser.set_defaults``. Here we just look at the resolved attribute - values; if the corresponding flag was required but the value is empty - (or None), exit with the same argparse-style error that argparse would - have emitted if ``required=True`` had been used directly. - - The env-var-sourced defaults (``MLPERF_RESULTS_DIR`` / - ``MLPERF_SYSTEMNAME``) populate ``args.results_dir`` / - ``args.systemname`` for free if argparse used the default; this check - is precisely what makes the env-var fallback live again on emitting - subcommands. + """Enforce post-parse non-empty checks for the required universal flags. + + Covers three universals per Plan 05-02 / D-09: + * ``--results-dir / -rd`` (marker: ``_mlps_req_results``) + * ``--systemname / -sn`` (marker: ``_mlps_req_systemname``) + * ``--checkpoint-folder / -cf`` (marker: ``_mlps_req_checkpoint_folder``) + + Training's ``--data-dir`` does NOT flow through this gate — it is + checked in ``training_args.py`` AFTER YAML config merge (D-07), because + a ``--config-file`` may legitimately supply ``data_dir``. + + Subcommands opt in to "required" via + ``add_universal_arguments(..., req_results=True, ...)`` and equivalents, + which set ``args._mlps_req_*`` markers via ``parser.set_defaults``. The + resolved argparse dest (``results_dir`` / ``systemname`` / + ``checkpoint_folder``) may be populated by an ``MLPSTORAGE_*`` env-var + fallback; we treat empty-string and ``None`` as missing. + + Emission model (D-01 / D-02 / D-04 / D-05): + * One ``error:`` line PER missing flag, using the D-02 verbatim + template. + * Immediately BELOW each error line, if the corresponding legacy + ``MLPERF_*`` env var is set AND the new ``MLPSTORAGE_*`` is not, + an adjacent ``hint:`` line using the D-05 verbatim template. + * All missing universals are checked and reported BEFORE + ``sys.exit()`` (D-01 aggregate-before-exit; T-05-06). No first- + error short-circuit. + * Exit code is ``EXIT_CODE.INVALID_ARGUMENTS`` (=2, D-03). + + ``MLPSTORAGE_CHECKPOINT_FOLDER`` has no legacy ``MLPERF_*`` + predecessor (D-08) and is intentionally absent from + ``_LEGACY_ENVVAR_MAP``, so the hint is never emitted for + checkpoint-folder — this falls out of the map lookup without a + special case. """ - errors = [] - if getattr(args, '_mlps_req_results', False): - if not getattr(args, 'results_dir', None): - errors.append( - "--results-dir/-rd is required (or set MLPERF_RESULTS_DIR)" - ) - if getattr(args, '_mlps_req_systemname', False): - if not getattr(args, 'systemname', None): - errors.append( - "--systemname/-sn is required (or set MLPERF_SYSTEMNAME)" - ) - if errors: - # Match argparse's stderr-then-exit-2 convention so existing tests - # that catch SystemExit continue to pass. - msg = "the following arguments are required: " + ", ".join(errors) - print(f"error: {msg}", file=sys.stderr) + pending = [] + for ( + marker_attr, + arg_attr, + envvar_name, + long_flag, + short_flag, + name_for_template, + ) in _UNIVERSAL_REQUIRED_SPECS: + if not getattr(args, marker_attr, False): + continue + if getattr(args, arg_attr, None): + continue + # D-02 verbatim template — kept on one physical line so the + # phase-wide grep for the exact template string finds it. + error_line = f"{long_flag}/{short_flag} is required: pass it on the command line or set MLPSTORAGE_{name_for_template}" + # D-04 / D-05: adjacent migration hint when legacy env is set and + # the new env is not. checkpoint_folder has no legacy pair, so its + # envvar_name is not a key in _LEGACY_ENVVAR_MAP and the hint stays + # None without any special-case code. + hint_line = None + legacy_name = _LEGACY_ENVVAR_MAP.get(envvar_name) + if legacy_name is not None: + legacy_val = os.environ.get(legacy_name, "") + new_val = os.environ.get(envvar_name, "") + if legacy_val and not new_val: + # D-05 verbatim template — single physical line for the + # phase-wide grep to pin the exact wording. + hint_line = f"hint: MLPERF_{name_for_template} is set but is no longer read; rename it to MLPSTORAGE_{name_for_template}" + pending.append((error_line, hint_line)) + + if pending: + for error_line, hint_line in pending: + print(f"error: {error_line}", file=sys.stderr) + if hint_line is not None: + print(hint_line, file=sys.stderr) sys.exit(EXIT_CODE.INVALID_ARGUMENTS) diff --git a/mlpstorage_py/config.py b/mlpstorage_py/config.py index b6e8f873..c9178f75 100755 --- a/mlpstorage_py/config.py +++ b/mlpstorage_py/config.py @@ -2,7 +2,6 @@ import enum import os import pathlib -import tempfile def check_env(setting, default_value=None): @@ -144,40 +143,236 @@ def get_datetime_string(): MAX_NUM_FILES_TRAIN = 128*1024 -def _resolve_default_results_dir() -> str: - """Resolve DEFAULT_RESULTS_DIR from MLPERF_RESULTS_DIR or tempdir. - - Extracted so tests can exercise the env-driven branch without - reloading this module — reload re-creates the PARAM_VALIDATION enum - class, which then fails `in`-checks against any pre-imported copy - held by other modules (notably the rules verifier). - """ - return os.environ.get( - "MLPERF_RESULTS_DIR", - os.path.join(tempfile.gettempdir(), "mlperf_storage_results"), - ) - - -DEFAULT_RESULTS_DIR = _resolve_default_results_dir() - -# DEFAULT_SYSTEMNAME mirrors DEFAULT_RESULTS_DIR (LAY-04): honor the -# MLPERF_SYSTEMNAME env var if set, fall back to an empty string. The -# universal-args layer (add_universal_arguments) decides required-vs-optional -# per subcommand; an empty default plus required=True on emitting commands -# makes "no --systemname and no env var" fail at parse time rather than -# silently producing `///results//...` (T-1-02). -def _resolve_default_systemname() -> str: - """Resolve DEFAULT_SYSTEMNAME from MLPERF_SYSTEMNAME. - - Extracted so tests can exercise the env-driven branch without - reloading this module — reload re-creates the PARAM_VALIDATION enum - class, which then fails `in`-checks against any pre-imported copy - held by other modules (notably the rules verifier). - """ - return os.environ.get("MLPERF_SYSTEMNAME", "") +# ----------------------------------------------------------------------------- +# MLPSTORAGE_* env-var-name string constants — SINGLE SOURCE OF TRUTH (D-10). +# +# Every mlpstorage-owned environment-variable NAME lives here as a string +# constant. Downstream modules (rules/utils.py, cli_parser.py, etc.) MUST +# import these names from this module rather than redefining them locally. +# Import direction is one-way (D-11): config.py MUST NOT import from +# mlpstorage_py.rules.*; doing so would create a cycle at interpreter +# startup. tests/unit/test_no_import_cycles.py locks this invariant. +# ----------------------------------------------------------------------------- +MLPSTORAGE_ORGNAME_ENVVAR = "MLPSTORAGE_ORGNAME" +MLPSTORAGE_SYSTEMNAME_ENVVAR = "MLPSTORAGE_SYSTEMNAME" +MLPSTORAGE_RESULTS_DIR_ENVVAR = "MLPSTORAGE_RESULTS_DIR" +MLPSTORAGE_DATA_DIR_ENVVAR = "MLPSTORAGE_DATA_DIR" +MLPSTORAGE_CHECKPOINT_FOLDER_ENVVAR = "MLPSTORAGE_CHECKPOINT_FOLDER" + +# _LEGACY_ENVVAR_MAP: new MLPSTORAGE_* env-var-name -> legacy MLPERF_* +# predecessor. Consumed by the parse-time migration-hint emitter (Plan 05-02 +# D-04/D-05). MLPSTORAGE_CHECKPOINT_FOLDER_ENVVAR is intentionally absent — +# per D-08 no MLPERF_CHECKPOINT_FOLDER ever existed, so the migration hint +# never fires for that pair. +_LEGACY_ENVVAR_MAP = { + MLPSTORAGE_SYSTEMNAME_ENVVAR: "MLPERF_SYSTEMNAME", + MLPSTORAGE_RESULTS_DIR_ENVVAR: "MLPERF_RESULTS_DIR", + MLPSTORAGE_DATA_DIR_ENVVAR: "MLPERF_DATA_DIR", + MLPSTORAGE_ORGNAME_ENVVAR: "MLPERF_ORGNAME", +} +# ----------------------------------------------------------------------------- +# ManPage sync symbols — SINGLE SOURCE OF TRUTH for the env-var sync test and +# ManPage ENVIRONMENT section authoring (Phase 7 D-05/D-10/D-11). +# +# _MANPAGE_SYNC_ALLOWLIST: env-var names the sync test MUST skip because they +# are either POSIX identity plumbing (USER/LOGNAME/PATH) or legacy MLPERF_* +# migration-detection strings intentionally excluded from the ManPage (D-05). +# Allow-list rationale: mlpstorage documents env vars it OWNS or BORROWS as +# functional contracts — not system identity plumbing or deprecated migration +# sentinels. +# +# MANPAGE_ENV_VAR_TIERS: maps every non-allowlisted env var mlpstorage reads +# to exactly one PRIMARY tier string (D-13/D-14). Seven allowed primary-tier +# values: 'owned', 'mpi-borrowed', 'aws-borrowed', 'storage-borrowed', +# 'storage-backend', 'diagnostic', 'internal-write'. Dual-role vars +# (MLPSTORAGE_CHECKPOINT_URI_SCHEME, AWS_ENDPOINT_URL) carry a single +# primary-tier value; their internal-write cross-ref is ManPage prose only +# per D-13 (no tuple values — D-13 explicitly rejects the tuple form in +# favour of prose cross-refs). +# +# Import direction is one-way (D-11): config.py MUST NOT import from +# mlpstorage_py.rules.*. tests/unit/test_no_import_cycles.py locks this +# invariant. +# ----------------------------------------------------------------------------- +_MANPAGE_SYNC_ALLOWLIST = frozenset({ + 'PATH', # stdlib-convention exempt per D-11 + 'USER', # POSIX identity plumbing per D-10 (read at environment/systemd_ipc.py:104, 156) + 'LOGNAME', # POSIX identity plumbing per D-10 (read at environment/systemd_ipc.py:104, 157) + 'MLPERF_SYSTEMNAME', # migration-detection only per D-05, removal target v1.2 + 'MLPERF_RESULTS_DIR', # migration-detection only per D-05, removal target v1.2 + 'MLPERF_DATA_DIR', # migration-detection only per D-05 (read at cli/training_args.py:309) + 'MLPERF_ORGNAME', # migration-detection only per D-05, removal target v1.2 +}) + +MANPAGE_ENV_VAR_TIERS = { + # -- Owned (8) -- + 'MLPSTORAGE_RESULTS_DIR': 'owned', + 'MLPSTORAGE_SYSTEMNAME': 'owned', + 'MLPSTORAGE_ORGNAME': 'owned', + 'MLPSTORAGE_DATA_DIR': 'owned', + 'MLPSTORAGE_CHECKPOINT_FOLDER': 'owned', + 'MLPSTORAGE_CHECKPOINT_URI_SCHEME': 'owned', # dual-role: primary Owned; internal-write sub-tag in ManPage prose per D-12/D-13. + 'KVCACHE_SELECTED_WORKLOADS': 'owned', # functional shell-wrapper contract per D-09. + 'MLPS_CHECKPOINT_MP_START_METHOD': 'owned', # MP start method override for streaming checkpointing (#682). + + # -- MPI-borrowed (4) -- + 'MPI_RUN_BIN': 'mpi-borrowed', + 'MPI_EXEC_BIN': 'mpi-borrowed', + 'OMPI_COMM_WORLD_RANK': 'mpi-borrowed', # SC-4 tag requirement. + 'PMI_RANK': 'mpi-borrowed', # SC-4 tag requirement. + + # -- AWS-borrowed (5) -- + 'AWS_ACCESS_KEY_ID': 'aws-borrowed', + 'AWS_SECRET_ACCESS_KEY': 'aws-borrowed', + 'AWS_REGION': 'aws-borrowed', + 'AWS_CA_BUNDLE': 'aws-borrowed', + 'AWS_ENDPOINT_URL': 'aws-borrowed', # dual-role: primary AWS-borrowed; internal-write cross-ref in ManPage prose per D-13 (write at s3dlio_writer.py:168). + + # -- Storage-borrowed (8) -- + 'BUCKET': 'storage-borrowed', + 'STORAGE_LIBRARY': 'storage-borrowed', + 'STORAGE_URI_SCHEME': 'storage-borrowed', + 'S3_LOAD_BALANCE_STRATEGY': 'storage-borrowed', + 'S3_ENDPOINT_URIS': 'storage-borrowed', # s3dlio-owned endpoint fallback chain per D-08. + 'S3_ENDPOINT_TEMPLATE': 'storage-borrowed', + 'S3_ENDPOINT_FILE': 'storage-borrowed', + 'S3_ENDPOINT': 'storage-borrowed', + + # -- Internal-write (1 primary) -- + 'DLIO_DROP_CACHES_TIMEOUT': 'internal-write', # write-only site at benchmarks/dlio.py:629; also read via `in os.environ` at dlio.py:602. + + # Note: no 'diagnostic' primary entries in the current inventory. + # The Diagnostic tier header exists in the ManPage (Plan C) but is + # populated by prose only — no var uniquely lives in this tier today. + + # -- AWS-borrowed additions (3) — consumed by boto3/s3dlio, not Python code -- + 'AWS_SESSION_TOKEN': 'aws-borrowed', # D-09: temporary STS session token. + 'AWS_DEFAULT_REGION': 'aws-borrowed', # D-09: fallback region when AWS_REGION is unset. + 'AWS_S3_ADDRESSING_STYLE': 'aws-borrowed', # D-09: virtual-hosted vs. path-style S3. + + # -- Storage-backend (22) — consumed exclusively by s3dlio Rust binary -- + 'S3DLIO_SKIP_HEAD': 'storage-backend', + 'S3DLIO_ENABLE_RANGE_OPTIMIZATION': 'storage-backend', + 'S3DLIO_RANGE_THRESHOLD_MB': 'storage-backend', + 'S3DLIO_RANGE_CONCURRENCY': 'storage-backend', + 'S3DLIO_CHUNK_SIZE': 'storage-backend', + 'S3DLIO_H2C': 'storage-backend', + 'S3DLIO_H2_ADAPTIVE_WINDOW': 'storage-backend', + 'S3DLIO_H2_STREAM_WINDOW_MB': 'storage-backend', + 'S3DLIO_H2_CONN_WINDOW_MB': 'storage-backend', + 'S3DLIO_POOL_MAX_IDLE_PER_HOST': 'storage-backend', + 'S3DLIO_POOL_IDLE_TIMEOUT_SECS': 'storage-backend', + 'S3DLIO_PUT_VERIFY': 'storage-backend', + 'S3DLIO_MPU_PUT_VERIFY': 'storage-backend', + 'S3DLIO_MULTIPART_THRESHOLD_MB': 'storage-backend', + 'S3DLIO_RT_THREADS': 'storage-backend', + 'S3DLIO_MAX_RETRY_ATTEMPTS': 'storage-backend', + 'S3DLIO_CONNECT_TIMEOUT_SECS': 'storage-backend', + 'S3DLIO_OPERATION_TIMEOUT_SECS': 'storage-backend', + 'S3DLIO_PUT_MAX_RETRIES': 'storage-backend', + 'S3DLIO_PUT_RETRY_DELAY_MS': 'storage-backend', + 'S3DLIO_MPU_MAX_RETRIES': 'storage-backend', + 'S3DLIO_MPU_RETRY_DELAY_S': 'storage-backend', +} -DEFAULT_SYSTEMNAME = _resolve_default_systemname() +# Documentation anchor — the s3dlio release this table targets. +S3DLIO_PINNED_VERSION = 'v0.9.106' + +# Phase 7.5 D-03: vars that can swing measured throughput by >=10% or alter protocol. +_S3DLIO_HIGH_RISK_ENV_VARS = frozenset({ + 'S3DLIO_SKIP_HEAD', + 'S3DLIO_ENABLE_RANGE_OPTIMIZATION', + 'S3DLIO_RANGE_THRESHOLD_MB', + 'S3DLIO_RANGE_CONCURRENCY', + 'S3DLIO_CHUNK_SIZE', + 'S3DLIO_H2C', + 'S3DLIO_H2_ADAPTIVE_WINDOW', + 'S3DLIO_H2_STREAM_WINDOW_MB', + 'S3DLIO_H2_CONN_WINDOW_MB', + 'S3DLIO_POOL_MAX_IDLE_PER_HOST', + 'S3DLIO_POOL_IDLE_TIMEOUT_SECS', + 'S3DLIO_PUT_VERIFY', + 'S3DLIO_MPU_PUT_VERIFY', + 'S3DLIO_MULTIPART_THRESHOLD_MB', + 'S3DLIO_RT_THREADS', +}) + +# Phase 7.5 D-02: env vars documented in ManPage but NOT read by +# mlpstorage_py/ Python code (consumed by s3dlio Rust binary or boto3/s3dlio +# backend init). The sync test subtracts this set from extra_in_docs so these +# vars can be documented without triggering the symmetric-difference failure. +MANPAGE_STORAGE_BACKEND_ENV_VARS = frozenset({ + # 15 HIGH-risk S3DLIO_* vars (per D-03) + 'S3DLIO_SKIP_HEAD', + 'S3DLIO_ENABLE_RANGE_OPTIMIZATION', + 'S3DLIO_RANGE_THRESHOLD_MB', + 'S3DLIO_RANGE_CONCURRENCY', + 'S3DLIO_CHUNK_SIZE', + 'S3DLIO_H2C', + 'S3DLIO_H2_ADAPTIVE_WINDOW', + 'S3DLIO_H2_STREAM_WINDOW_MB', + 'S3DLIO_H2_CONN_WINDOW_MB', + 'S3DLIO_POOL_MAX_IDLE_PER_HOST', + 'S3DLIO_POOL_IDLE_TIMEOUT_SECS', + 'S3DLIO_PUT_VERIFY', + 'S3DLIO_MPU_PUT_VERIFY', + 'S3DLIO_MULTIPART_THRESHOLD_MB', + 'S3DLIO_RT_THREADS', + # 7 MEDIUM-risk S3DLIO_* vars (per D-04) + 'S3DLIO_MAX_RETRY_ATTEMPTS', + 'S3DLIO_CONNECT_TIMEOUT_SECS', + 'S3DLIO_OPERATION_TIMEOUT_SECS', + 'S3DLIO_PUT_MAX_RETRIES', + 'S3DLIO_PUT_RETRY_DELAY_MS', + 'S3DLIO_MPU_MAX_RETRIES', + 'S3DLIO_MPU_RETRY_DELAY_S', + # 3 missing AWS vars (per D-09) — consumed by boto3/s3dlio, no Python os.environ.get() + 'AWS_SESSION_TOKEN', + 'AWS_DEFAULT_REGION', + 'AWS_S3_ADDRESSING_STYLE', +}) + + +# ----------------------------------------------------------------------------- +# ENV_FALLBACK_* module-level constants — resolved-at-import-time env-var +# reads for the four universal path/name arguments. Every resolver returns +# the empty string when its env var is unset (SC-3): never None, never a +# tempdir path. The universal-args layer (add_universal_arguments) decides +# required-vs-optional per subcommand; an empty fallback plus the parse-time +# loud-error gate (Plan 05-02) makes "no --flag and no env var" fail at +# parse time rather than silently producing malformed output paths (T-1-02). +# +# Resolvers are extracted from the module-level assignments so tests can +# exercise the env-driven branch without reloading this module — reload +# re-creates the PARAM_VALIDATION enum class, which then fails `in`-checks +# against any pre-imported copy held by other modules (notably the rules +# verifier). See LAY-04 for the same shape mirrored across all four. +# ----------------------------------------------------------------------------- +def _resolve_env_fallback_results_dir() -> str: + """Return MLPSTORAGE_RESULTS_DIR or empty string (never a tempdir path).""" + return os.environ.get(MLPSTORAGE_RESULTS_DIR_ENVVAR, "") + + +def _resolve_env_fallback_systemname() -> str: + """Return MLPSTORAGE_SYSTEMNAME or empty string.""" + return os.environ.get(MLPSTORAGE_SYSTEMNAME_ENVVAR, "") + + +def _resolve_env_fallback_data_dir() -> str: + """Return MLPSTORAGE_DATA_DIR or empty string.""" + return os.environ.get(MLPSTORAGE_DATA_DIR_ENVVAR, "") + + +def _resolve_env_fallback_checkpoint_folder() -> str: + """Return MLPSTORAGE_CHECKPOINT_FOLDER or empty string.""" + return os.environ.get(MLPSTORAGE_CHECKPOINT_FOLDER_ENVVAR, "") + + +ENV_FALLBACK_RESULTS_DIR = _resolve_env_fallback_results_dir() +ENV_FALLBACK_SYSTEMNAME = _resolve_env_fallback_systemname() +ENV_FALLBACK_DATA_DIR = _resolve_env_fallback_data_dir() +ENV_FALLBACK_CHECKPOINT_FOLDER = _resolve_env_fallback_checkpoint_folder() import enum diff --git a/mlpstorage_py/main.py b/mlpstorage_py/main.py index 7bf04ebc..8177cdfd 100755 --- a/mlpstorage_py/main.py +++ b/mlpstorage_py/main.py @@ -15,7 +15,7 @@ import traceback from mlpstorage_py.cli_parser import parse_arguments, validate_args, update_args -from mlpstorage_py.config import HISTFILE, DATETIME_STR, EXIT_CODE, DEFAULT_RESULTS_DIR, get_datetime_string, HYDRA_OUTPUT_SUBDIR +from mlpstorage_py.config import HISTFILE, DATETIME_STR, EXIT_CODE, get_datetime_string, HYDRA_OUTPUT_SUBDIR from mlpstorage_py.debug import debugger_hook, MLPS_DEBUG from mlpstorage_py.history import HistoryTracker from mlpstorage_py.mlps_logging import setup_logging, apply_logging_options @@ -244,17 +244,6 @@ def run_benchmark(args, run_datetime): benchmark = benchmark_class(args, run_datetime=run_datetime, logger=logger) - # Warn if the user is relying on the temp-dir default for results. - # Results stored in /tmp (or equivalent) are wiped on reboot. - _results_dir = getattr(args, 'results_dir', DEFAULT_RESULTS_DIR) - if _results_dir == DEFAULT_RESULTS_DIR and not os.environ.get('MLPERF_RESULTS_DIR'): - logger.warning( - f"Results directory not specified. Writing results to the system temp directory: " - f"{DEFAULT_RESULTS_DIR}. These results will NOT persist across a reboot. " - f"Use --results-dir or set the MLPERF_RESULTS_DIR environment variable " - f"to save results permanently." - ) - ret_code = EXIT_CODE.SUCCESS try: @@ -435,7 +424,7 @@ def _main_impl(): # for the reports subcommand. Keeping the import here lets validate / # rules-coverage / version run on a base install. from mlpstorage_py.report_generator import ReportGenerator - results_dir = args.results_dir if hasattr(args, 'results_dir') else DEFAULT_RESULTS_DIR + results_dir = getattr(args, 'results_dir', "") report_generator = ReportGenerator(results_dir, args, logger=logger) return report_generator.generate_reports() diff --git a/mlpstorage_py/report_generator.py b/mlpstorage_py/report_generator.py index 4eabc358..61a18e44 100755 --- a/mlpstorage_py/report_generator.py +++ b/mlpstorage_py/report_generator.py @@ -614,8 +614,10 @@ def _enumerate_on_disk_model_dirs(self) -> List[str]: if index_dir.is_dir(): found.add(str(index_dir.resolve())) elif bt_dir.name == 'training': - # training: per-model rollup lives at /run/ - # (Rules.md 2.1.16). Only add run/ if it exists. + # Rules.md 2.1.16: model-group folder is + # /run/, not / — must match + # _model_group_folder so _emit_empty_model_dirs + # doesn't write a stray file at /. run_dir = model_dir / 'run' if run_dir.is_dir(): found.add(str(run_dir.resolve())) diff --git a/mlpstorage_py/rules/utils.py b/mlpstorage_py/rules/utils.py index 3af09e50..a3877dac 100755 --- a/mlpstorage_py/rules/utils.py +++ b/mlpstorage_py/rules/utils.py @@ -10,16 +10,25 @@ import sys from typing import Tuple, List, Optional -from mlpstorage_py.config import BENCHMARK_TYPES, DATETIME_STR +from mlpstorage_py.config import ( + BENCHMARK_TYPES, + DATETIME_STR, + MLPSTORAGE_ORGNAME_ENVVAR, + MLPSTORAGE_SYSTEMNAME_ENVVAR, +) from mlpstorage_py.errors import ConfigurationError, ErrorCode -# Env-var names used by the CLI dispatch layer to source orgname/systemname. -# generate_output_location itself does NOT read these; values are threaded in -# via benchmark.args (populated upstream by main._main_impl()'s sentinel- -# resolution gate). The names are exported here as a single source of truth -# for the env-var spelling. -MLPSTORAGE_ORGNAME_ENVVAR = "MLPSTORAGE_ORGNAME" -MLPSTORAGE_SYSTEMNAME_ENVVAR = "MLPSTORAGE_SYSTEMNAME" +# Env-var names used by the CLI dispatch layer to source orgname/systemname +# are imported from mlpstorage_py.config (D-10): config.py is the single +# source of truth for the env-var spelling. Import direction is one-way +# (D-11) — config.py MUST NOT import from mlpstorage_py.rules. Downstream +# callers that historically read `from mlpstorage_py.rules.utils import +# MLPSTORAGE_*_ENVVAR` continue to work because these names are re-exported +# by the config import above. +# +# generate_output_location itself does NOT read these; values are threaded +# in via benchmark.args (populated upstream by main._main_impl()'s +# sentinel-resolution gate). # Each path segment appended to results_dir by generate_output_location must # match this — POSIX-safe alphanumeric plus '.', '_', '-' — and must not be diff --git a/mlpstorage_py/run_summary.py b/mlpstorage_py/run_summary.py index 9701277a..6706b46c 100644 --- a/mlpstorage_py/run_summary.py +++ b/mlpstorage_py/run_summary.py @@ -530,10 +530,13 @@ def print_run_summary(args) -> None: # Always-visible environment section. lines.append("") lines.append("--- Environment ---") - lines.append(_row("MLPERF_RESULTS_DIR:", os.environ.get('MLPERF_RESULTS_DIR', '[not set]'))) - lines.append(_row("MLPERF_SYSTEMNAME:", os.environ.get('MLPERF_SYSTEMNAME', '[not set]'))) - lines.append(_row("MPI_RUN_BIN:", os.environ.get('MPI_RUN_BIN', '[not set]'))) - lines.append(_row("MPI_EXEC_BIN:", os.environ.get('MPI_EXEC_BIN', '[not set]'))) + lines.append(_row("MLPSTORAGE_RESULTS_DIR:", os.environ.get('MLPSTORAGE_RESULTS_DIR', '[not set]'))) + lines.append(_row("MLPSTORAGE_SYSTEMNAME:", os.environ.get('MLPSTORAGE_SYSTEMNAME', '[not set]'))) + lines.append(_row("MLPSTORAGE_ORGNAME:", os.environ.get('MLPSTORAGE_ORGNAME', '[not set]'))) + lines.append(_row("MLPSTORAGE_DATA_DIR:", os.environ.get('MLPSTORAGE_DATA_DIR', '[not set]'))) + lines.append(_row("MLPSTORAGE_CHECKPOINT_FOLDER:", os.environ.get('MLPSTORAGE_CHECKPOINT_FOLDER', '[not set]'))) + lines.append(_row("MPI_RUN_BIN:", os.environ.get('MPI_RUN_BIN', '[not set]'))) + lines.append(_row("MPI_EXEC_BIN:", os.environ.get('MPI_EXEC_BIN', '[not set]'))) # KVCACHE_SELECTED_WORKLOADS is read by kv-cache-wrapper.sh and filters the # workload set. Surface it for kvcache so reviewers know whether the full # suite or a subset ran. diff --git a/tests/README.md b/tests/README.md index 616721c7..961dbc86 100644 --- a/tests/README.md +++ b/tests/README.md @@ -255,9 +255,8 @@ pytest tests/unit/test_benchmarks_kvcache.py -v | `test_cli_kvcache.py` | CLI argument parsing — KV cache model and cache configuration | | `test_cli_vectordb.py` | CLI argument parsing — VectorDB run/datagen subcommands | | `test_cluster_collector.py` | Cluster metric collection | -| `test_config.py` | Config module, env var handling, `DEFAULT_RESULTS_DIR` env-var override | +| `test_config.py` | Config module, env var handling, `ENV_FALLBACK_RESULTS_DIR` / `ENV_FALLBACK_SYSTEMNAME` env-var override | | `test_dlio_object_storage.py` | `DLIOBenchmark._apply_object_storage_params()` — `.env` loading, param injection, error cases | -| `test_main_warnings.py` | `run_benchmark()` tempdir warning — fires/suppresses correctly | | `test_dependency_check.py` | Dependency checking logic | | `test_environment.py` | Environment detection and validation | | `test_history.py` | `HistoryTracker` — run history file management | diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 21bda349..67698cd1 100755 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -125,15 +125,15 @@ def test_results_dir_required_when_req_results_true(self, parser, monkeypatch): Post-CR-02 fix: the requirement is enforced at the ``validate_args`` (post-parse) layer rather than via argparse ``required=True``, so the env-var-sourced default can satisfy it. - With ``MLPERF_RESULTS_DIR`` unset AND ``DEFAULT_RESULTS_DIR`` also - emptied (to simulate the "no env var, no fallback" worst case), + With ``MLPSTORAGE_RESULTS_DIR`` unset AND ``ENV_FALLBACK_RESULTS_DIR`` + also emptied (to simulate the "no env var, no fallback" worst case), the validator must error out via SystemExit. """ from mlpstorage_py.cli_parser import _check_universal_required_present - # Force DEFAULT_RESULTS_DIR to '' so the resolved value is empty + # Force ENV_FALLBACK_RESULTS_DIR to '' so the resolved value is empty # even though argparse no longer demands the CLI flag. import mlpstorage_py.cli.common_args as common_args_mod - monkeypatch.setattr(common_args_mod, 'DEFAULT_RESULTS_DIR', '') + monkeypatch.setattr(common_args_mod, 'ENV_FALLBACK_RESULTS_DIR', '') add_universal_arguments(parser, req_results=True) args = parser.parse_args([]) # no error from argparse itself now assert getattr(args, '_mlps_req_results', False) is True, ( diff --git a/tests/unit/test_cli_parser.py b/tests/unit/test_cli_parser.py index db779e7c..eface108 100755 --- a/tests/unit/test_cli_parser.py +++ b/tests/unit/test_cli_parser.py @@ -392,11 +392,11 @@ def test_no_file_object_consolidation_needed(self): # ===================================================================== -# 6. --systemname / MLPERF_SYSTEMNAME plumbing (LAY-04, D-10) +# 6. --systemname / MLPSTORAGE_SYSTEMNAME plumbing (LAY-04, D-10) # ===================================================================== class TestSystemname: - """Tests for the --systemname flag and MLPERF_SYSTEMNAME env-var plumbing. + """Tests for the --systemname flag and MLPSTORAGE_SYSTEMNAME env-var plumbing. Per CONTEXT.md D-10, --systemname is required on every emitting subcommand: training {datagen, run, configview, datasize}, checkpointing {datagen, run, @@ -462,7 +462,7 @@ class TestSystemname: ) def test_systemname_on_emitting_commands(self, label, argv_tail, monkeypatch): """Every emitting subcommand accepts --systemname and binds it to args.systemname.""" - monkeypatch.delenv('MLPERF_SYSTEMNAME', raising=False) + monkeypatch.delenv('MLPSTORAGE_SYSTEMNAME', raising=False) full = ['mlpstorage'] + argv_tail + ['--systemname', 'sys-v1'] with patch('sys.argv', full): args = parse_arguments() @@ -481,10 +481,11 @@ def test_empty_systemname_errors(self, label, argv_tail, monkeypatch): Post CR-02 fix: ``--systemname`` is no longer ``required=True`` at the argparse layer (that would silently neuter the - ``MLPERF_SYSTEMNAME`` env-var fallback per D-10). Instead, the post- - parse validator checks that the resolved value is non-empty; with - neither the CLI flag nor the env var supplied, ``DEFAULT_SYSTEMNAME`` - resolves to ``""`` and the validator errors out. + ``MLPSTORAGE_SYSTEMNAME`` env-var fallback per D-10). Instead, the + post-parse validator checks that the resolved value is non-empty; + with neither the CLI flag nor the env var supplied, + ``ENV_FALLBACK_SYSTEMNAME`` resolves to ``""`` and the validator + errors out. ``reports reportgen`` is intentionally excluded: reportgen makes ``--systemname`` OPTIONAL because omitting it aggregates a global @@ -499,20 +500,20 @@ def test_empty_systemname_errors(self, label, argv_tail, monkeypatch): "no-systemname aggregation contract." ) # Ensure env var is unset so the default falls back to '' (empty). - monkeypatch.delenv('MLPERF_SYSTEMNAME', raising=False) + monkeypatch.delenv('MLPSTORAGE_SYSTEMNAME', raising=False) full = ['mlpstorage'] + argv_tail # no --systemname with patch('sys.argv', full): with pytest.raises(SystemExit): parse_arguments() - # ---- CR-02: MLPERF_SYSTEMNAME / MLPERF_RESULTS_DIR env-var fallback ---- + # ---- CR-02: MLPSTORAGE_SYSTEMNAME / MLPSTORAGE_RESULTS_DIR env-var fallback ---- # # The reviewer flagged that ``required=True`` + ``default=`` is # contradictory in argparse — ``required=True`` checks the CLI tokens, # not the resolved value, so the env-var defaults were dead on every # emitting subcommand. These tests pin the post-fix behavior: when - # ``MLPERF_SYSTEMNAME`` / ``MLPERF_RESULTS_DIR`` is set, the CLI flag - # is no longer mandatory. + # ``MLPSTORAGE_SYSTEMNAME`` / ``MLPSTORAGE_RESULTS_DIR`` is set, the CLI + # flag is no longer mandatory. @pytest.mark.parametrize( "label,argv_tail", @@ -522,25 +523,26 @@ def test_empty_systemname_errors(self, label, argv_tail, monkeypatch): def test_systemname_env_var_satisfies_requirement( self, label, argv_tail, monkeypatch, ): - """If MLPERF_SYSTEMNAME is set, ``--systemname`` may be omitted on the CLI. + """If MLPSTORAGE_SYSTEMNAME is set, ``--systemname`` may be omitted on the CLI. Pre-fix this raised SystemExit because argparse's ``required=True`` ignored the env-var-sourced default. Post-fix, the resolved - ``DEFAULT_SYSTEMNAME`` (sourced from the env var) satisfies the - requirement and the parsed namespace carries that value. + ``ENV_FALLBACK_SYSTEMNAME`` (sourced from the env var) satisfies + the requirement and the parsed namespace carries that value. """ - monkeypatch.setenv('MLPERF_SYSTEMNAME', 'env-sys-v1') - # Patch DEFAULT_SYSTEMNAME in place — do NOT reload mlpstorage_py.config. - # Reloading config re-mints PARAM_VALIDATION (and other enums), breaking - # `enum_instance in [enum_class.MEMBER, ...]` checks in any module that - # already imported them (notably mlpstorage_py.rules.*). Downstream CLI - # builders captured DEFAULT_SYSTEMNAME by name at import time, so reload - # them so they pick up the patched value. + monkeypatch.setenv('MLPSTORAGE_SYSTEMNAME', 'env-sys-v1') + # Patch ENV_FALLBACK_SYSTEMNAME in place — do NOT reload + # mlpstorage_py.config. Reloading config re-mints PARAM_VALIDATION + # (and other enums), breaking `enum_instance in [enum_class.MEMBER, + # ...]` checks in any module that already imported them (notably + # mlpstorage_py.rules.*). Downstream CLI builders captured + # ENV_FALLBACK_SYSTEMNAME by name at import time, so reload them so + # they pick up the patched value. import importlib import mlpstorage_py.config as cfg_mod import mlpstorage_py.cli.common_args as common_args_mod - saved_systemname = cfg_mod.DEFAULT_SYSTEMNAME - cfg_mod.DEFAULT_SYSTEMNAME = 'env-sys-v1' + saved_systemname = cfg_mod.ENV_FALLBACK_SYSTEMNAME + cfg_mod.ENV_FALLBACK_SYSTEMNAME = 'env-sys-v1' importlib.reload(common_args_mod) import mlpstorage_py.cli as cli_mod importlib.reload(cli_mod) @@ -551,37 +553,37 @@ def test_systemname_env_var_satisfies_requirement( with patch('sys.argv', full): args = cli_parser_mod.parse_arguments() assert getattr(args, 'systemname', None) == 'env-sys-v1', ( - f"{label}: MLPERF_SYSTEMNAME env var must satisfy --systemname " + f"{label}: MLPSTORAGE_SYSTEMNAME env var must satisfy --systemname " f"requirement; got {getattr(args, 'systemname', None)!r}" ) finally: - monkeypatch.delenv('MLPERF_SYSTEMNAME', raising=False) - cfg_mod.DEFAULT_SYSTEMNAME = saved_systemname + monkeypatch.delenv('MLPSTORAGE_SYSTEMNAME', raising=False) + cfg_mod.ENV_FALLBACK_SYSTEMNAME = saved_systemname importlib.reload(common_args_mod) importlib.reload(cli_mod) importlib.reload(cli_parser_mod) def test_results_dir_env_var_satisfies_requirement(self, monkeypatch): - """If MLPERF_RESULTS_DIR is set, ``--results-dir`` may be omitted on the CLI. + """If MLPSTORAGE_RESULTS_DIR is set, ``--results-dir`` may be omitted on the CLI. - DEFAULT_RESULTS_DIR has a non-empty tempdir fallback even without - the env var, but the contractual D-10 promise is that - MLPERF_RESULTS_DIR is honored as a default on every emitting - subcommand. Pre-fix, ``required=True`` ignored the env var entirely. + Post D-13, ``ENV_FALLBACK_RESULTS_DIR`` returns ``""`` when the env + var is unset (the tempdir fallback was retired). The contractual + D-10 promise remains: ``MLPSTORAGE_RESULTS_DIR`` is honored as a + default on every emitting subcommand. """ - monkeypatch.setenv('MLPERF_RESULTS_DIR', '/env/results') - monkeypatch.setenv('MLPERF_SYSTEMNAME', 'env-sys') # so systemname check passes - # Patch DEFAULT_RESULTS_DIR + DEFAULT_SYSTEMNAME in place — do NOT - # reload mlpstorage_py.config (see sibling test for the enum-identity - # rationale). Reload the downstream CLI builders so the patched values - # propagate into argparse defaults. + monkeypatch.setenv('MLPSTORAGE_RESULTS_DIR', '/env/results') + monkeypatch.setenv('MLPSTORAGE_SYSTEMNAME', 'env-sys') # so systemname check passes + # Patch ENV_FALLBACK_RESULTS_DIR + ENV_FALLBACK_SYSTEMNAME in place + # — do NOT reload mlpstorage_py.config (see sibling test for the + # enum-identity rationale). Reload the downstream CLI builders so + # the patched values propagate into argparse defaults. import importlib import mlpstorage_py.config as cfg_mod import mlpstorage_py.cli.common_args as common_args_mod - saved_results_dir = cfg_mod.DEFAULT_RESULTS_DIR - saved_systemname = cfg_mod.DEFAULT_SYSTEMNAME - cfg_mod.DEFAULT_RESULTS_DIR = '/env/results' - cfg_mod.DEFAULT_SYSTEMNAME = 'env-sys' + saved_results_dir = cfg_mod.ENV_FALLBACK_RESULTS_DIR + saved_systemname = cfg_mod.ENV_FALLBACK_SYSTEMNAME + cfg_mod.ENV_FALLBACK_RESULTS_DIR = '/env/results' + cfg_mod.ENV_FALLBACK_SYSTEMNAME = 'env-sys' importlib.reload(common_args_mod) import mlpstorage_py.cli as cli_mod importlib.reload(cli_mod) @@ -598,21 +600,21 @@ def test_results_dir_env_var_satisfies_requirement(self, monkeypatch): with patch('sys.argv', argv): args = cli_parser_mod.parse_arguments() assert args.results_dir == '/env/results', ( - f"MLPERF_RESULTS_DIR env var must satisfy --results-dir " + f"MLPSTORAGE_RESULTS_DIR env var must satisfy --results-dir " f"requirement; got {args.results_dir!r}" ) finally: - monkeypatch.delenv('MLPERF_RESULTS_DIR', raising=False) - monkeypatch.delenv('MLPERF_SYSTEMNAME', raising=False) - cfg_mod.DEFAULT_RESULTS_DIR = saved_results_dir - cfg_mod.DEFAULT_SYSTEMNAME = saved_systemname + monkeypatch.delenv('MLPSTORAGE_RESULTS_DIR', raising=False) + monkeypatch.delenv('MLPSTORAGE_SYSTEMNAME', raising=False) + cfg_mod.ENV_FALLBACK_RESULTS_DIR = saved_results_dir + cfg_mod.ENV_FALLBACK_SYSTEMNAME = saved_systemname importlib.reload(common_args_mod) importlib.reload(cli_mod) importlib.reload(cli_parser_mod) def test_lockfile_does_not_require_systemname(self, monkeypatch): """Pure utility subcommands (lockfile) parse without --systemname.""" - monkeypatch.delenv('MLPERF_SYSTEMNAME', raising=False) + monkeypatch.delenv('MLPSTORAGE_SYSTEMNAME', raising=False) with patch('sys.argv', ['mlpstorage', 'lockfile', 'generate', '-o', '/tmp/x', '--results-dir', '/r']): args = parse_arguments() @@ -620,13 +622,13 @@ def test_lockfile_does_not_require_systemname(self, monkeypatch): def test_init_does_not_require_systemname(self, monkeypatch): """`mlpstorage init` parses without --systemname (init is bootstrap, not emitting).""" - monkeypatch.delenv('MLPERF_SYSTEMNAME', raising=False) + monkeypatch.delenv('MLPSTORAGE_SYSTEMNAME', raising=False) with patch('sys.argv', ['mlpstorage', 'init', 'Acme', '/tmp/r']): args = parse_arguments() assert args.mode == 'init' def test_systemname_flag_default_reflects_env_var(self, monkeypatch): - """The argparse default for --systemname reflects DEFAULT_SYSTEMNAME (which reads MLPERF_SYSTEMNAME). + """The argparse default for --systemname reflects ENV_FALLBACK_SYSTEMNAME (which reads MLPSTORAGE_SYSTEMNAME). Per CONTEXT.md D-10 / plan Task 1, --systemname is `required=True` on emitting commands — argparse demands the flag on CLI even when the @@ -635,9 +637,9 @@ def test_systemname_flag_default_reflects_env_var(self, monkeypatch): config-module constant; tests of the empty-systemname raise live in the slice-3 / slice-4 generate_output_location path, not the parser. """ - monkeypatch.setenv('MLPERF_SYSTEMNAME', 'env-sys') + monkeypatch.setenv('MLPSTORAGE_SYSTEMNAME', 'env-sys') # Call the env-resolver helper directly — reloading mlpstorage_py.config # re-mints PARAM_VALIDATION and corrupts enum-identity in downstream - # tests (see _resolve_default_systemname docstring in config.py). + # tests (see _resolve_env_fallback_systemname docstring in config.py). import mlpstorage_py.config as cfg_mod - assert cfg_mod._resolve_default_systemname() == 'env-sys' + assert cfg_mod._resolve_env_fallback_systemname() == 'env-sys' diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 71b528fa..3f095ac5 100755 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -5,11 +5,10 @@ - Environment variable handling (check_env) - Datetime string generation - Enum values and constants -- DEFAULT_RESULTS_DIR env-var override +- ENV_FALLBACK_RESULTS_DIR / ENV_FALLBACK_SYSTEMNAME env-var override """ import os -import tempfile import pytest from mlpstorage_py.config import ( @@ -305,68 +304,68 @@ def test_mpi_value(self): assert EXEC_TYPE.MPI.value == 'mpi' -class TestDefaultResultsDir: - """Tests for the DEFAULT_RESULTS_DIR constant. +class TestEnvFallbackResultsDir: + """Tests for the ENV_FALLBACK_RESULTS_DIR constant. - DEFAULT_RESULTS_DIR is set at module import time using: - os.environ.get('MLPERF_RESULTS_DIR', os.path.join(tempfile.gettempdir(), ...)) + ENV_FALLBACK_RESULTS_DIR is set at module import time using: + os.environ.get(MLPSTORAGE_RESULTS_DIR_ENVVAR, "") - Tests verify that the env-var override and the tempdir fallback both work. + Tests verify that the env-var override and the empty-string fallback both + work (Phase 5 D-13 retired the tempdir fallback — resolver returns "" when + the env var is unset). """ - def test_is_a_non_empty_string(self): - """DEFAULT_RESULTS_DIR is a non-empty string.""" - from mlpstorage_py.config import DEFAULT_RESULTS_DIR - assert isinstance(DEFAULT_RESULTS_DIR, str) - assert len(DEFAULT_RESULTS_DIR) > 0 + def test_is_a_string(self): + """ENV_FALLBACK_RESULTS_DIR is a string (empty or env-sourced).""" + from mlpstorage_py.config import ENV_FALLBACK_RESULTS_DIR + assert isinstance(ENV_FALLBACK_RESULTS_DIR, str) def test_matches_current_environment(self): - """DEFAULT_RESULTS_DIR reflects MLPERF_RESULTS_DIR if set, else the tempdir path.""" - from mlpstorage_py.config import DEFAULT_RESULTS_DIR - mlperf_env = os.environ.get('MLPERF_RESULTS_DIR') - if mlperf_env: - assert DEFAULT_RESULTS_DIR == mlperf_env + """ENV_FALLBACK_RESULTS_DIR reflects MLPSTORAGE_RESULTS_DIR if set, else empty string.""" + from mlpstorage_py.config import ENV_FALLBACK_RESULTS_DIR + mlps_env = os.environ.get('MLPSTORAGE_RESULTS_DIR') + if mlps_env: + assert ENV_FALLBACK_RESULTS_DIR == mlps_env else: - expected = os.path.join(tempfile.gettempdir(), 'mlperf_storage_results') - assert DEFAULT_RESULTS_DIR == expected + assert ENV_FALLBACK_RESULTS_DIR == "" - def test_env_var_overrides_tempdir_default(self, monkeypatch): - """When MLPERF_RESULTS_DIR is set, the resolver returns that value. + def test_env_var_overrides_default(self, monkeypatch): + """When MLPSTORAGE_RESULTS_DIR is set, the resolver returns that value. Calls the helper directly rather than reloading mlpstorage_py.config: reload would rebuild the PARAM_VALIDATION enum class, breaking identity comparisons in modules that already imported the original. """ - from mlpstorage_py.config import _resolve_default_results_dir - monkeypatch.setenv('MLPERF_RESULTS_DIR', '/custom/mlperf/results') - assert _resolve_default_results_dir() == '/custom/mlperf/results' + from mlpstorage_py.config import _resolve_env_fallback_results_dir + monkeypatch.setenv('MLPSTORAGE_RESULTS_DIR', '/custom/mlperf/results') + assert _resolve_env_fallback_results_dir() == '/custom/mlperf/results' - def test_falls_back_to_tempdir_when_env_not_set(self, monkeypatch): - """When MLPERF_RESULTS_DIR is absent, the resolver uses tempdir.""" - from mlpstorage_py.config import _resolve_default_results_dir - monkeypatch.delenv('MLPERF_RESULTS_DIR', raising=False) - expected = os.path.join(tempfile.gettempdir(), 'mlperf_storage_results') - assert _resolve_default_results_dir() == expected + def test_returns_empty_string_when_env_not_set(self, monkeypatch): + """When MLPSTORAGE_RESULTS_DIR is absent, the resolver returns empty string (D-13).""" + from mlpstorage_py.config import _resolve_env_fallback_results_dir + monkeypatch.delenv('MLPSTORAGE_RESULTS_DIR', raising=False) + assert _resolve_env_fallback_results_dir() == "" -class TestDefaultSystemname: - """Tests for the DEFAULT_SYSTEMNAME constant (LAY-04). +class TestEnvFallbackSystemname: + """Tests for the ENV_FALLBACK_SYSTEMNAME constant (LAY-04, D-12). - DEFAULT_SYSTEMNAME mirrors the DEFAULT_RESULTS_DIR pattern: reads the - MLPERF_SYSTEMNAME env var, falling back to an empty string when unset. + ENV_FALLBACK_SYSTEMNAME mirrors the ENV_FALLBACK_RESULTS_DIR pattern: reads + the MLPSTORAGE_SYSTEMNAME env var, falling back to an empty string when + unset. """ - def test_default_systemname_env_var(self, monkeypatch): - """DEFAULT_SYSTEMNAME reflects MLPERF_SYSTEMNAME env var when set, empty otherwise.""" + def test_env_fallback_systemname_env_var(self, monkeypatch): + """ENV_FALLBACK_SYSTEMNAME reflects MLPSTORAGE_SYSTEMNAME env var when set, empty otherwise.""" import mlpstorage_py.config as cfg_mod # Call the helper directly — reloading mlpstorage_py.config re-mints # PARAM_VALIDATION (and other enums), breaking enum-identity in # already-imported modules like mlpstorage_py.rules.* and corrupting # downstream integration tests. - monkeypatch.setenv('MLPERF_SYSTEMNAME', 'sys-v1') - assert cfg_mod._resolve_default_systemname() == 'sys-v1' + monkeypatch.setenv('MLPSTORAGE_SYSTEMNAME', 'sys-v1') + assert cfg_mod._resolve_env_fallback_systemname() == 'sys-v1' - monkeypatch.delenv('MLPERF_SYSTEMNAME', raising=False) - assert cfg_mod._resolve_default_systemname() == '' + monkeypatch.delenv('MLPSTORAGE_SYSTEMNAME', raising=False) + assert cfg_mod._resolve_env_fallback_systemname() == '' diff --git a/tests/unit/test_env_var_loud_errors.py b/tests/unit/test_env_var_loud_errors.py new file mode 100644 index 00000000..b2a6c40d --- /dev/null +++ b/tests/unit/test_env_var_loud_errors.py @@ -0,0 +1,280 @@ +""" +Loud-error contract tests for Phase 5 required universals (TEST-13, ENV-06, D-19). + +This file is the primary defender of the D-02 verbatim error template. When a +required universal is missing at post-parse time, mlpstorage MUST exit 2 with +a single ``error:`` line that names BOTH the CLI flag (long/short) AND the +corresponding ``MLPSTORAGE_*`` env var. TEST-13 pins this for the three +cli_parser-gate universals; ENV-06 pins it for ``--checkpoint-folder``. + +Covers four universals: + 1. ``--results-dir/-rd`` ↔ ``MLPSTORAGE_RESULTS_DIR`` (TEST-13) + 2. ``--systemname/-sn`` ↔ ``MLPSTORAGE_SYSTEMNAME`` (TEST-13) + 3. ``--data-dir/-dd`` ↔ ``MLPSTORAGE_DATA_DIR`` (TEST-13, training post-YAML gate) + 4. ``--checkpoint-folder/-cf`` ↔ ``MLPSTORAGE_CHECKPOINT_FOLDER`` (ENV-06) + +Contract invariants asserted in every test: + - D-01: One ``error:`` line per missing flag; multiple universals aggregate + BEFORE ``sys.exit`` (no first-error short-circuit). + - D-02: Verbatim template + ``"--{long}/-{short} is required: pass it on the command line or set MLPSTORAGE_{NAME}"`` + — pinned by substring match so future paraphrases fail loudly. + - D-03: Exit code is ``EXIT_CODE.INVALID_ARGUMENTS`` (= 2). + +Design notes: + - The three cli_parser gates are unit-tested in-process via + ``_check_universal_required_present`` with a hand-built Namespace. This is + the fastest and most focused surface — no subparser wiring noise. + - The data-dir gate lives in ``training_args.validate_training_arguments`` + (D-07: post-YAML). Tested via a direct call with a stubbed Namespace. + - The checkpoint-folder gate is exercised end-to-end through + ``parse_arguments`` (patching ``sys.argv``) because the + ``_mlps_req_checkpoint_folder=True`` marker is set inside the + ``checkpointing_args`` subparser build — reproducing that plumbing + manually would just duplicate the source under test. +""" + +from __future__ import annotations + +import argparse +import sys +from unittest.mock import patch + +import pytest + +from mlpstorage_py.cli_parser import ( + _check_universal_required_present, + parse_arguments, +) +from mlpstorage_py.cli.training_args import validate_training_arguments + + +# ---------------------------------------------------------------------------- # +# Helpers # +# ---------------------------------------------------------------------------- # + +# Env vars this file exercises — cleared before every test so ambient shell +# state does not leak in. +_ALL_ENV_NAMES = ( + "MLPERF_SYSTEMNAME", + "MLPERF_RESULTS_DIR", + "MLPERF_DATA_DIR", + "MLPSTORAGE_SYSTEMNAME", + "MLPSTORAGE_RESULTS_DIR", + "MLPSTORAGE_DATA_DIR", + "MLPSTORAGE_CHECKPOINT_FOLDER", +) + + +def _clean_env(monkeypatch): + for name in _ALL_ENV_NAMES: + monkeypatch.delenv(name, raising=False) + + +def _make_ns( + *, + req_results=False, + req_systemname=False, + req_checkpoint_folder=False, + results_dir="", + systemname="", + checkpoint_folder="", + mode="closed", +): + """Build a Namespace shaped for ``_check_universal_required_present``.""" + return argparse.Namespace( + mode=mode, + _mlps_req_results=req_results, + _mlps_req_systemname=req_systemname, + _mlps_req_checkpoint_folder=req_checkpoint_folder, + results_dir=results_dir, + systemname=systemname, + checkpoint_folder=checkpoint_folder, + ) + + +def _one_line_contains_both(stderr: str, needle_a: str, needle_b: str) -> bool: + """True iff at least one line in ``stderr`` contains BOTH substrings. + + D-02 is a single-line contract: the flag string and the env-var string + must appear on the SAME physical line so users grepping for either one + see the complete actionable message. + """ + return any(needle_a in line and needle_b in line for line in stderr.splitlines()) + + +# ---------------------------------------------------------------------------- # +# TEST-13: --results-dir / MLPSTORAGE_RESULTS_DIR # +# ---------------------------------------------------------------------------- # + +class TestResultsDirLoudError: + """D-02 verbatim template + single-line contract for the results-dir gate.""" + + def test_missing_results_dir_emits_verbatim_d02_template( + self, monkeypatch, capsys + ): + _clean_env(monkeypatch) + ns = _make_ns(req_results=True, results_dir="") + + with pytest.raises(SystemExit) as excinfo: + _check_universal_required_present(ns) + # D-03: exit code 2. + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + # D-02 verbatim template — full string, pinned. + assert ( + "error: --results-dir/-rd is required: pass it on the command line or set MLPSTORAGE_RESULTS_DIR" + in err + ) + # Single-line invariant: flag AND env var on the SAME line. + assert _one_line_contains_both( + err, "--results-dir/-rd", "MLPSTORAGE_RESULTS_DIR" + ) + + +# ---------------------------------------------------------------------------- # +# TEST-13: --systemname / MLPSTORAGE_SYSTEMNAME # +# ---------------------------------------------------------------------------- # + +class TestSystemnameLoudError: + """D-02 verbatim template + single-line contract for the systemname gate.""" + + def test_missing_systemname_emits_verbatim_d02_template( + self, monkeypatch, capsys + ): + _clean_env(monkeypatch) + ns = _make_ns(req_systemname=True, systemname="") + + with pytest.raises(SystemExit) as excinfo: + _check_universal_required_present(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + assert ( + "error: --systemname/-sn is required: pass it on the command line or set MLPSTORAGE_SYSTEMNAME" + in err + ) + assert _one_line_contains_both(err, "--systemname/-sn", "MLPSTORAGE_SYSTEMNAME") + + +# ---------------------------------------------------------------------------- # +# TEST-13: --data-dir / MLPSTORAGE_DATA_DIR (post-YAML training gate, D-07) # +# ---------------------------------------------------------------------------- # + +class TestDataDirLoudError: + """D-02 verbatim template + single-line contract for the training --data-dir + gate. The gate lives in ``validate_training_arguments`` and runs AFTER + YAML config merge (D-07) so ``--config-file`` can supply ``data_dir``.""" + + def test_missing_data_dir_emits_verbatim_d02_template( + self, monkeypatch, capsys + ): + _clean_env(monkeypatch) + + # Object protocol + run command is the D-07 gate trigger. + ns = argparse.Namespace( + command="run", + data_access_protocol="object", + data_dir=None, + o_direct=False, + ) + + with pytest.raises(SystemExit) as excinfo: + validate_training_arguments(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + assert ( + "error: --data-dir/-dd is required: pass it on the command line or set MLPSTORAGE_DATA_DIR" + in err + ) + assert _one_line_contains_both(err, "--data-dir/-dd", "MLPSTORAGE_DATA_DIR") + + +# ---------------------------------------------------------------------------- # +# ENV-06: --checkpoint-folder / MLPSTORAGE_CHECKPOINT_FOLDER # +# ---------------------------------------------------------------------------- # + +class TestCheckpointFolderLoudError: + """D-02 verbatim template + single-line contract for the checkpoint-folder + gate (ENV-06). Exercised end-to-end through ``parse_arguments`` because + the ``_mlps_req_checkpoint_folder`` marker is set inside the checkpointing + subparser build (checkpointing_args.py per D-08 / D-09).""" + + def test_missing_checkpoint_folder_emits_verbatim_d02_template( + self, monkeypatch, capsys + ): + _clean_env(monkeypatch) + + # Bare 'checkpointing run' with every OTHER required flag supplied but + # NOT --checkpoint-folder. --results-dir and --systemname are supplied + # so ONLY the checkpoint-folder gate can fire. + argv = [ + "mlpstorage", "closed", "checkpointing", "run", + "-cm", "1024", + "-m", "llama3-8b", + "-np", "8", + "-rd", "/tmp", + "-sn", "sys-v1", + "file", + ] + + with patch("sys.argv", argv): + with pytest.raises(SystemExit) as excinfo: + parse_arguments() + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + assert ( + "error: --checkpoint-folder/-cf is required: pass it on the command line or set MLPSTORAGE_CHECKPOINT_FOLDER" + in err + ) + assert _one_line_contains_both( + err, "--checkpoint-folder/-cf", "MLPSTORAGE_CHECKPOINT_FOLDER" + ) + + +# ---------------------------------------------------------------------------- # +# D-01: aggregate-before-exit — multiple missing universals produce multiple # +# error lines, and sys.exit is called ONCE at the end. # +# ---------------------------------------------------------------------------- # + +class TestMultipleMissingUniversalsAggregate: + """D-01: no first-error short-circuit. All missing universals reported.""" + + def test_three_missing_universals_produce_three_error_lines( + self, monkeypatch, capsys + ): + _clean_env(monkeypatch) + # All three cli_parser-gate universals required and empty. + ns = _make_ns( + req_results=True, + req_systemname=True, + req_checkpoint_folder=True, + results_dir="", + systemname="", + checkpoint_folder="", + ) + + with pytest.raises(SystemExit) as excinfo: + _check_universal_required_present(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + # Three error lines — aggregate emission (D-01). + assert err.count("error:") == 3 + + # Each of the three D-02 verbatim templates appears. + assert ( + "--results-dir/-rd is required: pass it on the command line or set MLPSTORAGE_RESULTS_DIR" + in err + ) + assert ( + "--systemname/-sn is required: pass it on the command line or set MLPSTORAGE_SYSTEMNAME" + in err + ) + assert ( + "--checkpoint-folder/-cf is required: pass it on the command line or set MLPSTORAGE_CHECKPOINT_FOLDER" + in err + ) diff --git a/tests/unit/test_env_var_manpage_sync.py b/tests/unit/test_env_var_manpage_sync.py new file mode 100644 index 00000000..d78a25aa --- /dev/null +++ b/tests/unit/test_env_var_manpage_sync.py @@ -0,0 +1,636 @@ +""" +Phase 7 DOC-05 sync test — env-var inventory gate. + +Purpose +------- +Assert that every environment variable mlpstorage reads in Python source +(``mlpstorage_py/**/*.py``) and the kv-cache shell wrapper is documented in +``ManPage.md``'s ``## ENVIRONMENT`` section, and vice-versa. An automated +symmetric-difference check makes future doc-drift a CI failure. + +Decisions enforced +------------------ + D-01 Three literal-arg grep patterns: ``os.environ.get()``, + ``os.environ[]``, ``os.getenv()``. + D-02 Constant-based reads caught via ``_ENV(VAR)?\\s*=\\s*['"]...['"]`` + module-level regex and an indirect-chain scanner for files that use + ``os.environ.get(var)`` with a loop variable (storage_config.py). + D-03 Shell-wrapper scan: ``os.getenv("KVCACHE_SELECTED_WORKLOADS")`` + literal-arg in ``kv-cache-wrapper.sh``. + D-04 Docstring examples are structurally excluded because the D-01 + regexes don't anchor to ``>>> `` lines; the bracket regex catches + ``os.environ['S3_ENDPOINT_URIS']`` in a docstring, but since that + name IS documented, this is a false-positive in the safe direction. + +Four test-class purposes (D-15/D-20/pattern-map): + TestEnvVarInventorySync — symmetric-difference + phantom-read sanity + TestPerVarTierAssignment — MANPAGE_ENV_VAR_TIERS vs ManPage tier headers + TestVerbatimPinnedStrings — D-20 anti-leaderboard and D-23 fallback strings + TestMlperfRenameGate — SC-1 / SC-6: no legacy MLPERF_* names in docs +""" + +from __future__ import annotations + +import re +import pathlib +import pytest + +from mlpstorage_py.config import ( + _MANPAGE_SYNC_ALLOWLIST, + MANPAGE_ENV_VAR_TIERS, + MANPAGE_STORAGE_BACKEND_ENV_VARS, +) + + +# --------------------------------------------------------------------------- +# Path constants (mirror test_no_import_cycles.py:38-42) +# --------------------------------------------------------------------------- + +_TESTS_UNIT_DIR = pathlib.Path(__file__).resolve().parent +_REPO_ROOT = _TESTS_UNIT_DIR.parent.parent +_MANPAGE = _REPO_ROOT / "ManPage.md" +_MLPSTORAGE_PY = _REPO_ROOT / "mlpstorage_py" +_KVCACHE_WRAPPER = _REPO_ROOT / "kv_cache_benchmark" / "utils" / "kv-cache-wrapper.sh" + + +# --------------------------------------------------------------------------- +# D-20 verbatim pinned strings (MUST NOT be paraphrased — Phase 5/6 doctrine) +# --------------------------------------------------------------------------- + +_D20_ANTI_LEADERBOARD_S1 = ( + "Aggregate columns are informational and NOT a leaderboard-input contract." +) +_D20_ANTI_LEADERBOARD_S2 = ( + "External ranking pipelines MUST compute their own aggregates from " + "per-invocation summary.json files." +) + +# D-23: multi-system-fallback sentence (verbatim from Phase 6 D-07 / Plan 07-03) +_D23_MULTI_SYSTEM_FALLBACK = ( + "When omitted, reportgen operates on the entire results-dir tree and " + "derives systemname per row from the workload dir's path segment " + "(open//results//... or checkpointing/training/vdb/kvcache analog)." +) + + +# --------------------------------------------------------------------------- +# Regex patterns (D-01 / D-02) +# --------------------------------------------------------------------------- + +# D-01: three literal-arg read patterns +_ENV_GET_RE = re.compile( + r"""os\.environ\.get\(\s*['"]([A-Z_][A-Z0-9_]*)['"]""" +) +_ENV_BRACKET_RE = re.compile( + r"""os\.environ\[\s*['"]([A-Z_][A-Z0-9_]*)['"]\s*\]""" +) +_ENV_GETENV_RE = re.compile( + r"""os\.getenv\(\s*['"]([A-Z_][A-Z0-9_]*)['"]""" +) + +# D-02: module-level _ENV / _ENVVAR constant declarations +_ENV_CONST_RE = re.compile( + r"""_ENV(?:VAR)?\s*=\s*['"]([A-Z_][A-Z0-9_]*)['"]""", + re.MULTILINE, +) + +# D-02 (indirect): detect files using os.environ.get(var) with a loop variable; +# if found, also scan for list-item string literals that are env-var names. +_ENV_INDIRECT_RE = re.compile(r"""os\.environ\.get\(var\b""") +_ENV_CHAIN_ITEM_RE = re.compile( + r"""^\s+['"]([A-Z][A-Z0-9_]{2,})['"]\s*,?\s*$""", + re.MULTILINE, +) + +# D-03: narrow shell-wrapper scan (os.getenv literal inside .sh embedded Python) +_KVCACHE_SHELL_RE = re.compile( + r"""os\.getenv\(\s*['"]([A-Z_][A-Z0-9_]*)['"]""" +) + + +# --------------------------------------------------------------------------- +# Module-level helper functions +# --------------------------------------------------------------------------- + +def _scan_python_reads(root: pathlib.Path) -> set[str]: + """Walk ``root/**/*.py`` and collect every env-var name read literally. + + Applies D-01 (three literal-arg patterns) and D-02 (module-level constant + declarations + indirect-chain scanner) per the Phase 7 DOC-05 contract. + + Files containing ``# sync-test-scan-skip`` on their first line are skipped + (escape hatch; no current files use this). + + The indirect-chain scanner (D-02 extension) activates when a file uses the + ``os.environ.get(var)`` loop pattern (like ``storage_config._resolve_endpoint``). + In that case, every string literal that looks like an env-var name appearing + as a list element is also included. This catches ``S3_ENDPOINT_TEMPLATE``, + ``S3_ENDPOINT_FILE``, and ``S3_ENDPOINT`` which are read via an iterated chain + rather than a direct literal-arg call. + + Raises: + FileNotFoundError: if ``root`` does not exist (propagated, never swallowed). + """ + reads: set[str] = set() + for py_file in root.rglob("*.py"): + text = py_file.read_text(errors="replace") + # Check for skip marker on first line + first_line = text.split("\n", 1)[0] + if "# sync-test-scan-skip" in first_line: + continue + + # D-01 literal-arg patterns + for m in _ENV_GET_RE.finditer(text): + reads.add(m.group(1)) + for m in _ENV_BRACKET_RE.finditer(text): + reads.add(m.group(1)) + for m in _ENV_GETENV_RE.finditer(text): + reads.add(m.group(1)) + + # D-02 module-level constant declarations + for m in _ENV_CONST_RE.finditer(text): + reads.add(m.group(1)) + + # D-02 indirect-chain scanner + if _ENV_INDIRECT_RE.search(text): + for m in _ENV_CHAIN_ITEM_RE.finditer(text): + candidate = m.group(1) + # Only admit uppercase-only names (no mixed-case false positives) + if candidate == candidate.upper(): + reads.add(candidate) + + return reads + + +def _scan_shell_reads(wrapper_path: pathlib.Path) -> set[str]: + """Scan ``wrapper_path`` for ``os.getenv()`` env-var reads (D-03). + + The kv-cache-wrapper.sh embeds a Python script that calls + ``os.getenv("KVCACHE_SELECTED_WORKLOADS")``. This function applies the same + ``_KVCACHE_SHELL_RE`` pattern as D-03 describes. + + Raises: + FileNotFoundError: if the wrapper does not exist. Per the loud-failure + doctrine, NEVER caught here — if the wrapper moves, the test fails loudly. + """ + text = wrapper_path.read_text(errors="replace") + reads: set[str] = set() + for m in _KVCACHE_SHELL_RE.finditer(text): + reads.add(m.group(1)) + return reads + + +def _parse_manpage_env_vars(manpage_path: pathlib.Path) -> dict[str, str]: + """Parse ``ManPage.md``'s ``## ENVIRONMENT`` section and return a mapping. + + Returns: + Dict mapping each env-var name (backtick-fenced in the first column of + a tier table) to its tier-header text (lowercased, with ``-borrowed`` + preserved). Examples:: + + 'MLPSTORAGE_RESULTS_DIR' -> 'owned' + 'OMPI_COMM_WORLD_RANK' -> 'mpi-borrowed' + 'DLIO_DROP_CACHES_TIMEOUT' -> 'internal-write' + + Only table-row env-var-column entries are parsed; cross-ref bullet lines + under ``Internal-write`` are **not** added to this dict because they + reference vars whose primary tier is in another section (D-13). + + Raises: + ValueError: if ``## ENVIRONMENT`` section is absent. + """ + text = manpage_path.read_text() + + # Extract the ENVIRONMENT section body + env_split = text.split("## ENVIRONMENT", 1) + if len(env_split) < 2: + raise ValueError( + "ManPage.md is missing '## ENVIRONMENT' section — " + "Phase 7 Plan C (07-03) may not have run yet." + ) + env_body = env_split[1] + + # Trim at the next top-level ## heading + tail_split = re.split(r"\n## ", env_body, maxsplit=1) + env_body = tail_split[0] + + # Split body on ### sub-headers to get per-tier chunks + # First chunk is the intro prose (before the first ###) + tier_chunks = re.split(r"\n### ", env_body) + + tier_map: dict[str, str] = {} + tier_canonical = { + "owned": "owned", + "mpi-borrowed": "mpi-borrowed", + "aws-borrowed": "aws-borrowed", + "storage-borrowed": "storage-borrowed", + "storage-backend": "storage-backend", # Phase 7.5 D-01: s3dlio Rust-layer vars + "diagnostic": "diagnostic", + "internal-write": "internal-write", + } + + for chunk in tier_chunks[1:]: # skip intro prose + lines = chunk.split("\n") + tier_header_raw = lines[0].strip() + tier_key = tier_header_raw.lower() + + if tier_key not in tier_canonical: + continue + canonical_tier = tier_canonical[tier_key] + + # Walk table rows: pattern is `| \`VAR_NAME\` | ... |` + row_re = re.compile(r"^\|\s*`([A-Z_][A-Z0-9_]*)`\s*\|") + for line in lines[1:]: + m = row_re.match(line) + if m: + var_name = m.group(1) + tier_map[var_name] = canonical_tier + + return tier_map + + +# =========================================================================== +# Test Class 1: Inventory sync (SC-3 / D-01 / D-02 / D-03 / D-04) +# =========================================================================== + + +class TestEnvVarInventorySync: + """SC-3: code reads vs ManPage docs-mentions must be symmetric (modulo allowlist). + + Enforces D-01 / D-02 / D-03 / D-04 and provides negative-simulation + methods that prove the grep pipeline fires on synthetic inputs. + """ + + def test_code_reads_equal_manpage_mentions_modulo_allowlist(self): + """Symmetric-difference between code reads and ManPage mentions must be empty. + + Failure message shows BOTH the missing set (in code, not in ManPage) and + the extra set (in ManPage, not in code) so a developer can fix the drift + without re-instrumenting. + """ + code_reads = _scan_python_reads(_MLPSTORAGE_PY) | _scan_shell_reads(_KVCACHE_WRAPPER) + docs_mentions = set(_parse_manpage_env_vars(_MANPAGE).keys()) + + missing_in_docs = code_reads - _MANPAGE_SYNC_ALLOWLIST - docs_mentions + extra_in_docs = docs_mentions - _MANPAGE_SYNC_ALLOWLIST - MANPAGE_STORAGE_BACKEND_ENV_VARS - code_reads + + assert missing_in_docs == set() and extra_in_docs == set(), ( + "Env-var inventory out of sync (SC-3 / DOC-05).\n\n" + f" In code but NOT in ManPage (missing from docs):\n" + f" {sorted(missing_in_docs)}\n\n" + f" In ManPage but NOT in code (extra in docs):\n" + f" {sorted(extra_in_docs)}\n\n" + "To fix:\n" + " - For vars in 'missing from docs': add them to ManPage.md's " + "ENVIRONMENT section under the appropriate tier AND add them to " + "MANPAGE_ENV_VAR_TIERS in mlpstorage_py/config.py.\n" + " - For vars in 'extra in docs': either add the env-var read to " + "the codebase, remove it from the ManPage tier table, or add it to " + "_MANPAGE_SYNC_ALLOWLIST in config.py with a rationale comment." + ) + + def test_allowlist_names_are_never_documented(self): + """Allowlist names (POSIX identity and legacy MLPERF_*) must not appear in ManPage tables. + + Verifies _MANPAGE_SYNC_ALLOWLIST's purpose: these names are exempt + because they are NOT functional contracts (they are either POSIX identity + plumbing or deprecated migration-detection sentinels). If an allowlist + name leaks into a ManPage tier table, this test fails. + """ + docs_mentions = set(_parse_manpage_env_vars(_MANPAGE).keys()) + leaked = _MANPAGE_SYNC_ALLOWLIST & docs_mentions + assert leaked == set(), ( + "Allowlist names found in ManPage tier tables — " + "these names must not be documented (per D-05/D-10):\n" + f" {sorted(leaked)}\n\n" + "Remove them from the ManPage ENVIRONMENT tier tables. If they " + "need to stay in ManPage prose (e.g. a 'formerly known as' note), " + "that is fine — only table rows are checked here." + ) + + def test_scan_detects_phantom_read(self): + """Negative simulation: the three D-01 regexes catch synthetic env-var reads. + + Constructs a synthetic text snippet with each of the three literal-arg + patterns and asserts each regex matches the phantom name 'PHANTOM_ENV_VAR_XYZ'. + Proves the grep pipeline fires on real code; a future refactor that + accidentally breaks the regex (e.g., adds mandatory triple-quotes) will + fail here LOUDLY rather than silently passing on an empty match set. + """ + synthetic = ( + "value1 = os.environ.get('PHANTOM_ENV_VAR_XYZ', 'default')\n" + "value2 = os.environ['PHANTOM_ENV_VAR_XYZ']\n" + "value3 = os.getenv('PHANTOM_ENV_VAR_XYZ')\n" + ) + found: set[str] = set() + for m in _ENV_GET_RE.finditer(synthetic): + found.add(m.group(1)) + for m in _ENV_BRACKET_RE.finditer(synthetic): + found.add(m.group(1)) + for m in _ENV_GETENV_RE.finditer(synthetic): + found.add(m.group(1)) + + assert "PHANTOM_ENV_VAR_XYZ" in found, ( + "D-01 regex pipeline did not detect 'PHANTOM_ENV_VAR_XYZ' in the " + "synthetic test string — one or more of the three regexes is broken.\n" + f" Matched names: {sorted(found)}" + ) + + def test_scan_detects_phantom_constant_declaration(self): + """Negative simulation: D-02 _ENV_CONST_RE catches synthetic constant declarations. + + Constructs a synthetic text snippet with the module-level constant + pattern and asserts _ENV_CONST_RE matches 'PHANTOM_CONST_NAME'. + """ + synthetic = "PHANTOM_ENV = 'PHANTOM_CONST_NAME'\n" + found: set[str] = set() + for m in _ENV_CONST_RE.finditer(synthetic): + found.add(m.group(1)) + + assert "PHANTOM_CONST_NAME" in found, ( + "D-02 _ENV_CONST_RE did not detect 'PHANTOM_CONST_NAME' in the " + "synthetic test string — the constant-declaration regex is broken.\n" + f" Matched names: {sorted(found)}" + ) + + +# =========================================================================== +# Test Class 2: Per-var tier assignment (SC-4 / D-12 / D-13 / D-14) +# =========================================================================== + + +class TestPerVarTierAssignment: + """SC-4: every env var's declared primary tier matches its ManPage sub-header. + + Enforces MANPAGE_ENV_VAR_TIERS consistency with the ManPage table layout, + pins specific SC-4 vars (OMPI_COMM_WORLD_RANK / PMI_RANK as mpi-borrowed; + MLPSTORAGE_CHECKPOINT_URI_SCHEME as owned), and validates tier-value spelling. + """ + + def test_every_manpage_env_var_matches_declared_tier(self): + """Each key in MANPAGE_ENV_VAR_TIERS must appear under the matching ManPage sub-header. + + Parses the ManPage tier tables and compares each var's discovered tier + against the declared tier in MANPAGE_ENV_VAR_TIERS. Failure message + identifies the mismatched var with expected and actual values. + """ + manpage_tiers = _parse_manpage_env_vars(_MANPAGE) + mismatches = [] + for var_name, declared_tier in MANPAGE_ENV_VAR_TIERS.items(): + actual_tier = manpage_tiers.get(var_name) + if actual_tier is None: + mismatches.append( + f" {var_name!r}: declared={declared_tier!r}, " + f"actual=NOT FOUND in ManPage tables" + ) + elif actual_tier != declared_tier: + mismatches.append( + f" {var_name!r}: declared={declared_tier!r}, actual={actual_tier!r}" + ) + + assert not mismatches, ( + "MANPAGE_ENV_VAR_TIERS tier assignments disagree with ManPage sub-headers " + "(SC-4 / D-12 / D-13):\n\n" + + "\n".join(mismatches) + + "\n\n" + "Fix by updating either MANPAGE_ENV_VAR_TIERS in mlpstorage_py/config.py " + "or the ManPage tier table (or both)." + ) + + def test_every_tier_value_is_one_of_six_canonical_strings(self): + """MANPAGE_ENV_VAR_TIERS values must be one of the seven canonical tier strings (Phase 7.5 D-01). + + The seven valid values are: + 'owned', 'mpi-borrowed', 'aws-borrowed', 'storage-borrowed', + 'storage-backend', 'diagnostic', 'internal-write' + + Any spelling variation (extra whitespace, wrong capitalisation, unknown tier) + fails this test. + """ + valid_tiers = { + "owned", + "mpi-borrowed", + "aws-borrowed", + "storage-borrowed", + "storage-backend", # Phase 7.5 D-01: s3dlio Rust-layer vars + "diagnostic", + "internal-write", + } + bad_values = { + v for v in MANPAGE_ENV_VAR_TIERS.values() if v not in valid_tiers + } + assert not bad_values, ( + f"MANPAGE_ENV_VAR_TIERS contains invalid tier value(s): {sorted(bad_values)}\n" + f"Valid tier values: {sorted(valid_tiers)}\n" + "Fix by correcting the tier string in mlpstorage_py/config.py." + ) + + def test_sc4_specific_tier_pins(self): + """SC-4 spot-checks: specific vars must have specific tier assignments. + + Pins the three vars explicitly called out in SC-4 and D-12: + - MLPSTORAGE_CHECKPOINT_URI_SCHEME: 'owned' (per D-12; dual-role via + internal-write sub-tag in ManPage prose only, NOT a second tier value) + - OMPI_COMM_WORLD_RANK: 'mpi-borrowed' (SC-4 OpenMPI rank injection) + - PMI_RANK: 'mpi-borrowed' (SC-4 PMI/MPICH fallback rank injection) + """ + assert MANPAGE_ENV_VAR_TIERS.get("MLPSTORAGE_CHECKPOINT_URI_SCHEME") == "owned", ( + "MANPAGE_ENV_VAR_TIERS['MLPSTORAGE_CHECKPOINT_URI_SCHEME'] must be 'owned' " + "(SC-4 / D-12). The internal-write sub-tag is ManPage prose only — " + "not a second MANPAGE_ENV_VAR_TIERS entry." + ) + assert MANPAGE_ENV_VAR_TIERS.get("OMPI_COMM_WORLD_RANK") == "mpi-borrowed", ( + "MANPAGE_ENV_VAR_TIERS['OMPI_COMM_WORLD_RANK'] must be 'mpi-borrowed' (SC-4)." + ) + assert MANPAGE_ENV_VAR_TIERS.get("PMI_RANK") == "mpi-borrowed", ( + "MANPAGE_ENV_VAR_TIERS['PMI_RANK'] must be 'mpi-borrowed' (SC-4)." + ) + + +# =========================================================================== +# Test Class 3: Verbatim pinned strings (D-20 / D-23) +# =========================================================================== + + +class TestVerbatimPinnedStrings: + """D-20 / D-23: specific prose sentences must be present verbatim in ManPage.md. + + Continues the Phase 5 D-02 / Phase 6 D-24 verbatim-pinning doctrine: + user-pinned wording changes are CI failures, not merge conflicts. + """ + + def test_d20_anti_leaderboard_sentence_1_present(self): + """ManPage.md must contain the D-20 first anti-leaderboard sentence verbatim.""" + text = _MANPAGE.read_text() + assert _D20_ANTI_LEADERBOARD_S1 in text, ( + f"D-20 first anti-leaderboard sentence missing from ManPage.md.\n" + f"Expected substring:\n {_D20_ANTI_LEADERBOARD_S1!r}\n\n" + "Fix: add this exact sentence (no paraphrase) to the " + "'### Aggregate Interpretation' subsection under '## RESULTS DIRECTORY'." + ) + + def test_d20_anti_leaderboard_sentence_2_present(self): + """ManPage.md must contain the D-20 second anti-leaderboard sentence verbatim.""" + text = _MANPAGE.read_text() + assert _D20_ANTI_LEADERBOARD_S2 in text, ( + f"D-20 second anti-leaderboard sentence missing from ManPage.md.\n" + f"Expected substring:\n {_D20_ANTI_LEADERBOARD_S2!r}\n\n" + "Fix: add this exact sentence (no paraphrase) to the " + "'### Aggregate Interpretation' subsection under '## RESULTS DIRECTORY'." + ) + + def test_d23_multi_system_fallback_present(self): + """ManPage.md must contain the D-23 multi-system-fallback sentence verbatim. + + Pins the reportgen ``--systemname`` optional-fallback behaviour documented + in Phase 6 D-07 and the 07-03 ManPage rewrite. + """ + text = _MANPAGE.read_text() + assert _D23_MULTI_SYSTEM_FALLBACK in text, ( + f"D-23 multi-system-fallback sentence missing from ManPage.md.\n" + f"Expected substring:\n {_D23_MULTI_SYSTEM_FALLBACK!r}\n\n" + "Fix: add this exact sentence (no paraphrase) to the reportgen " + "'--systemname' subsection under '### Reports' (approximately line 852)." + ) + + +# =========================================================================== +# Test Class 4: MLPERF_* rename gate (SC-1 / SC-6) +# =========================================================================== + + +class TestMlperfRenameGate: + """SC-1 / SC-6: no legacy MLPERF_* env-var names in ManPage.md or docs/. + + SC-1 is the executable form of Phase 5's grep-visible ADR enforcement: + ``grep -nE "MLPERF_(SYSTEMNAME|RESULTS_DIR|ORGNAME|DATA_DIR)" ManPage.md`` = 0 hits. + SC-6 extends the gate to ``docs/``. + + Note: the string ``MLPERF_DATA_DIR`` IS in ``mlpstorage_py/`` source code + (at ``cli/training_args.py``) for migration-hint detection — it is NOT + expected to be zero across the codebase; only the ManPage and docs/ are + tested here. + """ + + _LEGACY_PATTERN = re.compile( + r"MLPERF_(SYSTEMNAME|RESULTS_DIR|ORGNAME|DATA_DIR)" + ) + + def test_no_legacy_env_var_names_in_manpage(self): + """ManPage.md must contain zero hits for MLPERF_(SYSTEMNAME|RESULTS_DIR|ORGNAME|DATA_DIR). + + This is the SC-1 executable form. A match means the Phase 7 rename + pass (Plan 07-03 D-25) missed a line. Failure message shows the + offending line number and surrounding context. + """ + text = _MANPAGE.read_text() + lines = text.splitlines() + offenders = [] + for lineno, line in enumerate(lines, start=1): + if self._LEGACY_PATTERN.search(line): + offenders.append(f" L{lineno}: {line.rstrip()}") + + assert not offenders, ( + "ManPage.md contains legacy MLPERF_* env-var names (SC-1 violated).\n" + "Offending lines:\n" + + "\n".join(offenders) + + "\n\nFix: rename to the corresponding MLPSTORAGE_* equivalent " + "(see Phase 7 Plan 07-03 D-25 for the rename map)." + ) + + def test_no_legacy_env_var_names_in_docs_dir(self): + """docs/*.md must contain zero hits for MLPERF_(SYSTEMNAME|RESULTS_DIR|ORGNAME|DATA_DIR). + + Extends SC-1 to the ``docs/`` directory (SC-6). Skips cleanly if the + directory does not yet exist (not-yet-created case, not a failure). + The ``examples/`` directory is intentionally not scanned (it does not + exist in this repo; SC-6 scope adjustment recorded in Plan 07-03 SUMMARY). + """ + docs_dir = _REPO_ROOT / "docs" + if not docs_dir.exists(): + pytest.skip( + "docs/ directory does not exist — SC-6 gate is a no-op until " + "docs/ is created." + ) + + offenders = [] + for md_file in docs_dir.rglob("*.md"): + text = md_file.read_text(errors="replace") + lines = text.splitlines() + for lineno, line in enumerate(lines, start=1): + if self._LEGACY_PATTERN.search(line): + rel = md_file.relative_to(_REPO_ROOT) + offenders.append(f" {rel}:L{lineno}: {line.rstrip()}") + + assert not offenders, ( + "docs/*.md contains legacy MLPERF_* env-var names (SC-6 violated).\n" + "Offending lines:\n" + + "\n".join(offenders) + + "\n\nFix: rename to MLPSTORAGE_* equivalents throughout docs/." + ) + + +# =========================================================================== +# Test Class 5: Storage-backend env var carve-out (Phase 7.5 D-02, D-08) +# =========================================================================== + + +class TestStorageBackendEnvVars: + """Phase 7.5 D-02: MANPAGE_STORAGE_BACKEND_ENV_VARS carve-out assertions. + + Verifies that: + 1. Every entry in MANPAGE_STORAGE_BACKEND_ENV_VARS appears in ManPage.md + under the tier declared in MANPAGE_ENV_VAR_TIERS. + 2. MANPAGE_STORAGE_BACKEND_ENV_VARS and _MANPAGE_SYNC_ALLOWLIST are disjoint + (backend vars are DOCUMENTED; allowlist vars are NOT). + 3. The D-08 verbatim warning prefix appears in benchmark source. + """ + + def test_backend_vars_appear_in_manpage(self): + """Every entry in MANPAGE_STORAGE_BACKEND_ENV_VARS is in ManPage under its declared tier.""" + manpage_tiers = _parse_manpage_env_vars(_MANPAGE) + missing = [] + wrong_tier = [] + for var_name in MANPAGE_STORAGE_BACKEND_ENV_VARS: + declared_tier = MANPAGE_ENV_VAR_TIERS.get(var_name) + actual_tier = manpage_tiers.get(var_name) + if actual_tier is None: + missing.append(var_name) + elif declared_tier is not None and actual_tier != declared_tier: + wrong_tier.append( + f" {var_name!r}: expected={declared_tier!r}, got={actual_tier!r}" + ) + assert not missing and not wrong_tier, ( + "MANPAGE_STORAGE_BACKEND_ENV_VARS integrity failure (Phase 7.5 D-02):\n" + + (f"\nNot found in ManPage:\n {sorted(missing)}" if missing else "") + + ("\nWrong tier in ManPage:\n" + "\n".join(wrong_tier) if wrong_tier else "") + ) + + def test_backend_vars_disjoint_from_allowlist(self): + """MANPAGE_STORAGE_BACKEND_ENV_VARS must not overlap with _MANPAGE_SYNC_ALLOWLIST. + + Backend vars are DOCUMENTED in ManPage tier tables (they have a tier entry). + Allowlist vars are NOT documented (they are intentionally excluded). + Overlap means a var is simultaneously 'documented but not read by Python' + AND 'exempt from documentation' — a contradiction. + """ + overlap = MANPAGE_STORAGE_BACKEND_ENV_VARS & _MANPAGE_SYNC_ALLOWLIST + assert not overlap, ( + "MANPAGE_STORAGE_BACKEND_ENV_VARS and _MANPAGE_SYNC_ALLOWLIST overlap " + f"(Phase 7.5 D-02/D-05 disjoint invariant violated):\n {sorted(overlap)}\n\n" + "Backend vars are documented; allowlist vars are not. They must be disjoint." + ) + + def test_d08_warning_prefix_in_benchmark_source(self): + """D-08 verbatim warning prefix must appear in mlpstorage_py/benchmarks/base.py. + + Locks the warn-and-continue check added by Phase 7.5 Plan 02 Task 1. + If the method is removed or the string is paraphrased, this test fails. + """ + base_src = (_MLPSTORAGE_PY / "benchmarks" / "base.py").read_text() + assert "s3dlio env var '" in base_src, ( + "D-08 verbatim warning prefix \"s3dlio env var '\" not found in " + "mlpstorage_py/benchmarks/base.py.\n\n" + "Fix: ensure _check_storage_backend_env() in base.py uses the verbatim " + "template from Phase 7.5 D-08 (do not paraphrase)." + ) diff --git a/tests/unit/test_env_var_migration.py b/tests/unit/test_env_var_migration.py new file mode 100644 index 00000000..2a2b82d2 --- /dev/null +++ b/tests/unit/test_env_var_migration.py @@ -0,0 +1,347 @@ +""" +Env-var migration regression tests (Phase 5 D-17, TEST-12). + +This file is the negative-assertion contract for the MLPERF_* → MLPSTORAGE_* +rename. It guarantees: + + (a) TEST-12 canonical regression — setting MLPERF_SYSTEMNAME alone does + NOT satisfy the ENV-04 loud-error gate. If a future PR re-adds a + back-compat shim reading MLPERF_*, this test fails loudly. + (b) D-05 verbatim migration hint text — the exact string + ``"hint: MLPERF_ is set but is no longer read; rename it to + MLPSTORAGE_"`` appears on stderr immediately below the D-02 + error line in the correct D-04 conditions. + (c) D-04 negative condition — the hint does NOT fire when the CLI flag + was passed even with MLPERF_* set (no error → nothing to hint about). + (d) D-04 negative condition — the hint does NOT fire when both MLPERF_* + AND MLPSTORAGE_* are set (user has already migrated). + +Verbatim template strings pinned here: + D-02: "is required: pass it on the command line or set MLPSTORAGE_" + D-05: "is set but is no longer read; rename it to MLPSTORAGE_" + +Legacy MLPERF_* names appear ONLY in this file (per D-16 carveout) — they are +the negative-assertion payload. +""" + +from __future__ import annotations + +import argparse +import os +import sys + +import pytest + +from mlpstorage_py.cli_parser import _check_universal_required_present +from mlpstorage_py.cli.training_args import validate_training_arguments + + +# ---------------------------------------------------------------------------- # +# Helpers # +# ---------------------------------------------------------------------------- # + +# Legacy env-var names the phase must NOT read. Kept as constants so future +# grep-audits that flag MLPERF_* in tests can allowlist THIS FILE cleanly. +_LEGACY_SYSTEMNAME = "MLPERF_SYSTEMNAME" +_LEGACY_RESULTS_DIR = "MLPERF_RESULTS_DIR" +_LEGACY_DATA_DIR = "MLPERF_DATA_DIR" +# NOTE: intentionally NO _LEGACY_CHECKPOINT_FOLDER — per D-08 there was never +# a MLPERF_CHECKPOINT_FOLDER; its absence from _LEGACY_ENVVAR_MAP is asserted +# by the hint-negative test below. + +_NEW_SYSTEMNAME = "MLPSTORAGE_SYSTEMNAME" +_NEW_RESULTS_DIR = "MLPSTORAGE_RESULTS_DIR" +_NEW_DATA_DIR = "MLPSTORAGE_DATA_DIR" +_NEW_CHECKPOINT_FOLDER = "MLPSTORAGE_CHECKPOINT_FOLDER" + +# All Phase-5-relevant env vars — cleared at the start of every test so no +# ambient shell state leaks in. +_ALL_ENV_NAMES = ( + _LEGACY_SYSTEMNAME, + _LEGACY_RESULTS_DIR, + _LEGACY_DATA_DIR, + _NEW_SYSTEMNAME, + _NEW_RESULTS_DIR, + _NEW_DATA_DIR, + _NEW_CHECKPOINT_FOLDER, +) + + +def _clean_env(monkeypatch): + """Clear every MLPERF_* and MLPSTORAGE_* var this file exercises.""" + for name in _ALL_ENV_NAMES: + monkeypatch.delenv(name, raising=False) + + +def _make_ns( + *, + req_results=False, + req_systemname=False, + req_checkpoint_folder=False, + results_dir="", + systemname="", + checkpoint_folder="", + mode="closed", +): + """Build a minimal argparse.Namespace shaped like a post-parse args object. + + Only the attributes ``_check_universal_required_present`` inspects are + populated — the function is a pure predicate over these markers/values. + """ + return argparse.Namespace( + mode=mode, + _mlps_req_results=req_results, + _mlps_req_systemname=req_systemname, + _mlps_req_checkpoint_folder=req_checkpoint_folder, + results_dir=results_dir, + systemname=systemname, + checkpoint_folder=checkpoint_folder, + ) + + +# ---------------------------------------------------------------------------- # +# TEST-12: MLPERF_* alone does NOT satisfy the ENV-04 gate. # +# ---------------------------------------------------------------------------- # + +class TestEnvVarMigrationCliParserGate: + """Cover the systemname / results-dir / checkpoint-folder universals whose + loud-error gate runs in ``cli_parser._check_universal_required_present``.""" + + # ------------------------------------------------------------------ (a) -- + def test_mlperf_systemname_alone_fails_env04_gate(self, monkeypatch, capsys): + """TEST-12: MLPERF_SYSTEMNAME set, MLPSTORAGE_SYSTEMNAME unset → still fails. + + Also asserts that BOTH the D-02 verbatim error line AND the D-05 + verbatim migration hint appear on stderr. + """ + _clean_env(monkeypatch) + monkeypatch.setenv(_LEGACY_SYSTEMNAME, "legacy-sys") + + ns = _make_ns(req_systemname=True, systemname="") + + with pytest.raises(SystemExit) as excinfo: + _check_universal_required_present(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + # D-02 verbatim template — this is the whole point of TEST-12. + assert ( + "--systemname/-sn is required: pass it on the command line or set MLPSTORAGE_SYSTEMNAME" + in err + ) + # D-05 verbatim template — the migration hint MUST fire here. + assert ( + "hint: MLPERF_SYSTEMNAME is set but is no longer read; rename it to MLPSTORAGE_SYSTEMNAME" + in err + ) + + # ------------------------------------------------------------------ (b) -- + def test_hint_fires_only_when_mlperf_set_and_mlpstorage_unset( + self, monkeypatch, capsys + ): + """D-04 truth table for the hint, scenario A: legacy set, new unset → hint fires.""" + _clean_env(monkeypatch) + monkeypatch.setenv(_LEGACY_SYSTEMNAME, "legacy-sys") + # MLPSTORAGE_SYSTEMNAME intentionally UNSET. + + ns = _make_ns(req_systemname=True, systemname="") + + with pytest.raises(SystemExit): + _check_universal_required_present(ns) + + err = capsys.readouterr().err + assert ( + "hint: MLPERF_SYSTEMNAME is set but is no longer read; rename it to MLPSTORAGE_SYSTEMNAME" + in err + ) + + def test_hint_does_not_fire_when_both_mlperf_and_mlpstorage_set( + self, monkeypatch, capsys + ): + """D-04 truth table for the hint, scenario B: both set, arg populated → gate passes, no output. + + When MLPSTORAGE_SYSTEMNAME is set the resolver would have populated + args.systemname, so the gate does not trigger and no error/hint is + emitted. Simulate the post-parse state where the resolver has done its + job. + """ + _clean_env(monkeypatch) + monkeypatch.setenv(_LEGACY_SYSTEMNAME, "legacy-sys") + monkeypatch.setenv(_NEW_SYSTEMNAME, "new-sys") + + # env-var default already flowed into the Namespace value. + ns = _make_ns(req_systemname=True, systemname="new-sys") + + # No SystemExit: the gate does not trigger. + _check_universal_required_present(ns) + + err = capsys.readouterr().err + # No error → no hint. Assert both are absent. + assert "error:" not in err + assert "hint:" not in err + + def test_no_hint_when_neither_env_var_is_set(self, monkeypatch, capsys): + """D-04 truth table for the hint, scenario C: neither env set → error fires, hint does NOT.""" + _clean_env(monkeypatch) + + ns = _make_ns(req_systemname=True, systemname="") + + with pytest.raises(SystemExit) as excinfo: + _check_universal_required_present(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + # Error line MUST appear (D-02). + assert ( + "--systemname/-sn is required: pass it on the command line or set MLPSTORAGE_SYSTEMNAME" + in err + ) + # Hint MUST NOT appear — no legacy state to migrate from. + assert "hint:" not in err + + # ------------------------------------------------------------------ (c) -- + def test_hint_does_not_fire_when_cli_flag_was_passed(self, monkeypatch, capsys): + """D-04: hint only appears adjacent to an error line — a satisfied gate emits nothing.""" + _clean_env(monkeypatch) + monkeypatch.setenv(_LEGACY_SYSTEMNAME, "legacy-sys") + + # The user passed --systemname on the CLI; the arg is populated. + ns = _make_ns(req_systemname=True, systemname="user-supplied-via-flag") + + # Gate does not trigger. + _check_universal_required_present(ns) + + err = capsys.readouterr().err + # Silence is the correct behavior: no error, therefore no hint. + assert "error:" not in err + assert "hint:" not in err + + # ------------------------------------------------------------------ (d) -- + def test_results_dir_migration_hint_verbatim(self, monkeypatch, capsys): + """D-05 verbatim template for the results-dir migration hint.""" + _clean_env(monkeypatch) + monkeypatch.setenv(_LEGACY_RESULTS_DIR, "/legacy/results") + + ns = _make_ns(req_results=True, results_dir="") + + with pytest.raises(SystemExit) as excinfo: + _check_universal_required_present(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + assert ( + "--results-dir/-rd is required: pass it on the command line or set MLPSTORAGE_RESULTS_DIR" + in err + ) + assert ( + "hint: MLPERF_RESULTS_DIR is set but is no longer read; rename it to MLPSTORAGE_RESULTS_DIR" + in err + ) + + def test_checkpoint_folder_has_no_migration_hint(self, monkeypatch, capsys): + """D-08: checkpoint-folder has no legacy MLPERF_* predecessor — hint never fires. + + Even if the user somehow has a MLPERF_CHECKPOINT_FOLDER set (which was + never read by any historical mlpstorage), the hint must NOT appear — + the entry is absent from ``_LEGACY_ENVVAR_MAP`` on purpose. The D-02 + error line still fires for the missing flag. + """ + _clean_env(monkeypatch) + # Simulate a user who imagined the legacy name — mlpstorage must not + # emit a rename hint, because the "legacy" name was never valid. + monkeypatch.setenv("MLPERF_CHECKPOINT_FOLDER", "/legacy/ckpt") + + ns = _make_ns(req_checkpoint_folder=True, checkpoint_folder="") + + with pytest.raises(SystemExit) as excinfo: + _check_universal_required_present(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + # D-02 error MUST still appear. + assert ( + "--checkpoint-folder/-cf is required: pass it on the command line or set MLPSTORAGE_CHECKPOINT_FOLDER" + in err + ) + # But NO migration hint — checkpoint-folder is a fresh env var. + assert "hint:" not in err + + +# ---------------------------------------------------------------------------- # +# Data-dir gate lives in training_args.validate_training_arguments (D-07) # +# — post-YAML, so it is exercised separately from the cli_parser gate above. # +# ---------------------------------------------------------------------------- # + +class TestEnvVarMigrationTrainingGate: + """Cover the training --data-dir gate. Runs post-YAML in + ``validate_training_arguments``; the migration-hint stanza there mirrors + the D-04/D-05 rules from the cli_parser gate.""" + + def test_data_dir_migration_hint_fires_verbatim(self, monkeypatch, capsys): + """D-05 verbatim template for the data-dir migration hint.""" + _clean_env(monkeypatch) + monkeypatch.setenv(_LEGACY_DATA_DIR, "/legacy/data") + + # Object protocol + run command triggers the post-YAML gate at + # training_args.py:302 when data_dir is falsy. + ns = argparse.Namespace( + command="run", + data_access_protocol="object", + data_dir=None, + o_direct=False, + ) + + with pytest.raises(SystemExit) as excinfo: + validate_training_arguments(ns) + assert excinfo.value.code == 2 + + err = capsys.readouterr().err + # D-02 verbatim template. + assert ( + "--data-dir/-dd is required: pass it on the command line or set MLPSTORAGE_DATA_DIR" + in err + ) + # D-05 verbatim template. + assert ( + "hint: MLPERF_DATA_DIR is set but is no longer read; rename it to MLPSTORAGE_DATA_DIR" + in err + ) + + def test_data_dir_hint_absent_when_only_mlpstorage_set(self, monkeypatch, capsys): + """D-04: no legacy set → no hint, and if data_dir is populated, no error either.""" + _clean_env(monkeypatch) + monkeypatch.setenv(_NEW_DATA_DIR, "/new/data") + + # Simulate the env-var default having populated args.data_dir already. + ns = argparse.Namespace( + command="run", + data_access_protocol="object", + data_dir="/new/data", + o_direct=False, + ) + + # Gate passes; nothing printed. + validate_training_arguments(ns) + + err = capsys.readouterr().err + assert "error:" not in err + assert "hint:" not in err + + def test_data_dir_hint_absent_when_flag_passed(self, monkeypatch, capsys): + """D-04: user passed --data-dir on the CLI → no error → no hint.""" + _clean_env(monkeypatch) + monkeypatch.setenv(_LEGACY_DATA_DIR, "/legacy/data") + + ns = argparse.Namespace( + command="run", + data_access_protocol="object", + data_dir="/cli/supplied/data", + o_direct=False, + ) + + # Gate passes; nothing printed. + validate_training_arguments(ns) + + err = capsys.readouterr().err + assert "error:" not in err + assert "hint:" not in err diff --git a/tests/unit/test_main_warnings.py b/tests/unit/test_main_warnings.py deleted file mode 100644 index 37130610..00000000 --- a/tests/unit/test_main_warnings.py +++ /dev/null @@ -1,144 +0,0 @@ -""" -Tests for warning/info messages emitted by mlpstorage_py.main.run_benchmark(). - -Changes under test: - - A warning is logged when results_dir defaults to the system temp directory - and MLPERF_RESULTS_DIR is not set in the environment. - - No warning when the user explicitly passes --results-dir. - - No warning when MLPERF_RESULTS_DIR is set (the default already reflects the - env var, so the user has expressed a preference). -""" - -import os -import tempfile -from argparse import Namespace -from unittest.mock import MagicMock, patch - -import pytest - -from mlpstorage_py.config import EXIT_CODE - - -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -def _make_args(results_dir=None): - """Return a minimal Namespace accepted by run_benchmark().""" - from mlpstorage_py.config import DEFAULT_RESULTS_DIR - return Namespace( - benchmark='training', - results_dir=results_dir if results_dir is not None else DEFAULT_RESULTS_DIR, - verify_lockfile=None, # skip lockfile validation branch - skip_validation=True, # skip environment validation branch - dry_run=False, - ) - - -def _mock_benchmark(): - """Return a mock benchmark whose run() returns SUCCESS.""" - b = MagicMock() - b.run.return_value = EXIT_CODE.SUCCESS - return b - - -# --------------------------------------------------------------------------- -# Tests -# --------------------------------------------------------------------------- - -class TestResultsDirWarning: - """run_benchmark() warns when results land in the system temp directory.""" - - @patch('mlpstorage_py.benchmarks.TrainingBenchmark') - @patch('mlpstorage_py.main.logger') - def test_warning_emitted_when_using_tempdir_default( - self, mock_logger, mock_training_cls, monkeypatch - ): - """Warning fires when results_dir == DEFAULT_RESULTS_DIR and env var unset.""" - from mlpstorage_py.main import run_benchmark - from mlpstorage_py.config import DEFAULT_RESULTS_DIR - - monkeypatch.delenv('MLPERF_RESULTS_DIR', raising=False) - mock_training_cls.return_value = _mock_benchmark() - - args = _make_args(DEFAULT_RESULTS_DIR) - run_benchmark(args, '20260427_120000') - - # At least one warning call should mention the temp directory - assert mock_logger.warning.called, "Expected logger.warning to be called" - warning_text = ' '.join( - str(c) for c in mock_logger.warning.call_args_list - ).lower() - assert 'temp' in warning_text or 'tmp' in warning_text, ( - f"Expected temp-dir mention in warning, got: {warning_text}" - ) - - @patch('mlpstorage_py.benchmarks.TrainingBenchmark') - @patch('mlpstorage_py.main.logger') - def test_warning_mentions_results_dir_flag( - self, mock_logger, mock_training_cls, monkeypatch - ): - """Warning text tells the user about --results-dir and MLPERF_RESULTS_DIR.""" - from mlpstorage_py.main import run_benchmark - from mlpstorage_py.config import DEFAULT_RESULTS_DIR - - monkeypatch.delenv('MLPERF_RESULTS_DIR', raising=False) - mock_training_cls.return_value = _mock_benchmark() - - run_benchmark(_make_args(DEFAULT_RESULTS_DIR), '20260427_120000') - - warning_text = ' '.join( - str(c) for c in mock_logger.warning.call_args_list - ) - assert 'results-dir' in warning_text or '--results-dir' in warning_text, ( - "Warning should tell users about the --results-dir flag" - ) - assert 'MLPERF_RESULTS_DIR' in warning_text, ( - "Warning should tell users about the MLPERF_RESULTS_DIR env var" - ) - - @patch('mlpstorage_py.benchmarks.TrainingBenchmark') - @patch('mlpstorage_py.main.logger') - def test_no_tempdir_warning_when_results_dir_explicitly_set( - self, mock_logger, mock_training_cls, monkeypatch - ): - """No tempdir warning when the user supplies an explicit results directory.""" - from mlpstorage_py.main import run_benchmark - - monkeypatch.delenv('MLPERF_RESULTS_DIR', raising=False) - mock_training_cls.return_value = _mock_benchmark() - - run_benchmark(_make_args('/explicit/results/path'), '20260427_120000') - - # Inspect all warning calls — none should be about the temp directory - for call in mock_logger.warning.call_args_list: - text = str(call).lower() - assert 'temp directory' not in text and 'mlperf_results_dir' not in text, ( - f"Unexpected tempdir warning when results_dir was explicit: {call}" - ) - - @patch('mlpstorage_py.benchmarks.TrainingBenchmark') - @patch('mlpstorage_py.main.logger') - def test_no_tempdir_warning_when_mlperf_results_dir_env_set( - self, mock_logger, mock_training_cls, monkeypatch - ): - """No tempdir warning when MLPERF_RESULTS_DIR is set in the environment. - - Even if results_dir happens to equal the DEFAULT_RESULTS_DIR constant that - was baked in at import time, the runtime env-var check prevents the warning. - """ - from mlpstorage_py.main import run_benchmark - from mlpstorage_py.config import DEFAULT_RESULTS_DIR - - # Set the env var at runtime — the warning condition checks os.environ live - monkeypatch.setenv('MLPERF_RESULTS_DIR', '/env/results') - mock_training_cls.return_value = _mock_benchmark() - - # Pass the old DEFAULT_RESULTS_DIR value; the env-var check still suppresses warning - run_benchmark(_make_args(DEFAULT_RESULTS_DIR), '20260427_120000') - - for call in mock_logger.warning.call_args_list: - text = str(call).lower() - assert 'temp directory' not in text, ( - f"Unexpected tempdir warning when MLPERF_RESULTS_DIR was set: {call}" - ) diff --git a/tests/unit/test_no_import_cycles.py b/tests/unit/test_no_import_cycles.py new file mode 100644 index 00000000..16e980c8 --- /dev/null +++ b/tests/unit/test_no_import_cycles.py @@ -0,0 +1,131 @@ +""" +Import-direction invariant tests (Phase 5 D-11 / D-18). + +After Phase 5, ``mlpstorage_py.config`` owns all ``MLPSTORAGE_*_ENVVAR`` +string constants (D-10) and ``mlpstorage_py.rules.utils`` imports them from +``config`` — never the other direction. This one-way dependency prevents +circular imports and lets ``config`` stay a leaf module that any layer can +import without dragging the rules subsystem in. + +This file guards two invariants: + + (a) A fresh Python interpreter can execute + ``import mlpstorage_py.config; import mlpstorage_py.rules.utils`` + with exit code 0 — proving no cycle exists at import time. A fresh + subprocess is essential; an in-process check lies because both + modules are cached in ``sys.modules`` by the time pytest runs. + + (b) Structural grep — ``mlpstorage_py/config.py`` contains no + ``from mlpstorage_py.rules`` or ``import mlpstorage_py.rules`` line. + This catches accidental reintroduction of the reverse dependency in + code review by making the direction violation loudly visible as a + test failure rather than a runtime surprise. + +If either invariant breaks in a future PR, one of these tests fails with a +message that names the invariant, so the reviewer sees the direction +violation immediately. +""" + +from __future__ import annotations + +import pathlib +import subprocess +import sys + + +# Path to the project root — walk up from this test file to the repo root, +# then to ``mlpstorage_py/config.py``. This test file lives at +# ``tests/unit/test_no_import_cycles.py``, so root is parent x 2. +_TESTS_UNIT_DIR = pathlib.Path(__file__).resolve().parent +_REPO_ROOT = _TESTS_UNIT_DIR.parent.parent +_CONFIG_PY = _REPO_ROOT / "mlpstorage_py" / "config.py" +_RULES_UTILS_PY = _REPO_ROOT / "mlpstorage_py" / "rules" / "utils.py" + + +class TestNoImportCycles: + """Guard the config → rules.utils one-way dependency direction (D-11).""" + + def test_config_then_rules_utils_imports_cleanly_in_fresh_interpreter(self): + """A fresh subprocess must be able to import config THEN rules.utils. + + In-process checks are unreliable because both modules are already + cached in ``sys.modules`` by the time pytest runs — a cycle would + not resurface. Spawning a fresh interpreter forces the import + machinery to walk the full dependency graph from scratch. + """ + result = subprocess.run( + [ + sys.executable, + "-c", + "import mlpstorage_py.config; import mlpstorage_py.rules.utils", + ], + capture_output=True, + text=True, + timeout=30, + ) + + # Include stderr in the failure message so the diagnosis is + # immediate — the reviewer sees the ImportError / stack trace + # without needing to re-run manually. + assert result.returncode == 0, ( + "Fresh-interpreter import of config then rules.utils failed " + "(D-11 violated).\n" + f"stdout: {result.stdout!r}\n" + f"stderr: {result.stderr!r}" + ) + # Belt-and-suspenders: even if the subprocess somehow returned 0 + # with an ImportError logged, catch that too. + assert "ImportError" not in result.stderr, ( + f"ImportError surfaced in stderr: {result.stderr!r}" + ) + assert "circular" not in result.stderr.lower(), ( + f"'circular' appeared in stderr: {result.stderr!r}" + ) + + def test_config_does_not_import_from_rules(self): + """Structural check: ``config.py`` must not import from ``rules.*``. + + Grep-style assertion over the file text. Catches accidental + reintroduction of the reverse dependency in code review by making + the direction violation loudly visible as a test failure. This + assertion is what makes D-11 durable across future refactors — + the runtime import check above catches the symptom; this catches + the cause. + """ + text = _CONFIG_PY.read_text() + + # Both syntactic forms are prohibited. + assert "from mlpstorage_py.rules" not in text, ( + "mlpstorage_py/config.py contains 'from mlpstorage_py.rules' — " + "one-way dependency direction violated (D-11). config MUST NOT " + "import from rules; rules imports from config." + ) + assert "import mlpstorage_py.rules" not in text, ( + "mlpstorage_py/config.py contains 'import mlpstorage_py.rules' — " + "one-way dependency direction violated (D-11). config MUST NOT " + "import from rules; rules imports from config." + ) + + def test_rules_utils_imports_env_var_constants_from_config(self): + """Positive-direction assertion: rules.utils sources env-var names from config. + + Documents the intended direction (rules → config) and pins the + specific constants moved in Plan 05-01 per D-10. If a future + refactor tries to re-inline the env-var-name string literals in + ``rules/utils.py``, this test surfaces the duplication. + """ + text = _RULES_UTILS_PY.read_text() + + assert "from mlpstorage_py.config import" in text, ( + "mlpstorage_py/rules/utils.py must import from " + "mlpstorage_py.config (positive D-11 direction)." + ) + # The two constants moved in Plan 05-01 per D-10. + assert "MLPSTORAGE_ORGNAME_ENVVAR" in text, ( + "rules/utils.py does not reference MLPSTORAGE_ORGNAME_ENVVAR — " + "the D-10 single-source-of-truth import may have regressed." + ) + assert "MLPSTORAGE_SYSTEMNAME_ENVVAR" in text, ( + "rules/utils.py does not reference MLPSTORAGE_SYSTEMNAME_ENVVAR — " + "the D-10 single-source-of-truth import may have regressed." + ) diff --git a/tests/unit/test_run_summary.py b/tests/unit/test_run_summary.py index 2b0e0481..fb27d87b 100644 --- a/tests/unit/test_run_summary.py +++ b/tests/unit/test_run_summary.py @@ -266,28 +266,54 @@ def test_banner_shows_systemname(self, mock_logger): ) @patch('mlpstorage_py.run_summary.logger') - def test_banner_environment_includes_mlperf_systemname(self, mock_logger, - monkeypatch): - """Environment section lists the MLPERF_SYSTEMNAME env var alongside - the existing MLPERF_RESULTS_DIR row. + def test_banner_environment_includes_mlpstorage_systemname(self, mock_logger, + monkeypatch): + """Environment section lists the MLPSTORAGE_SYSTEMNAME env var alongside + the existing MLPSTORAGE_RESULTS_DIR row. """ from mlpstorage_py.run_summary import print_run_summary - monkeypatch.setenv('MLPERF_SYSTEMNAME', 'env-sys-v1') + monkeypatch.setenv('MLPSTORAGE_SYSTEMNAME', 'env-sys-v1') args = _make_args(orgname='Acme', systemname='env-sys-v1', mode='closed', results_dir='/r') print_run_summary(args) output = _joined_status_calls(mock_logger) - assert 'MLPERF_SYSTEMNAME' in output, ( - f"Expected MLPERF_SYSTEMNAME env-var row in Environment section; " + assert 'MLPSTORAGE_SYSTEMNAME' in output, ( + f"Expected MLPSTORAGE_SYSTEMNAME env-var row in Environment section; " f"got: {output!r}" ) assert 'env-sys-v1' in output, ( f"Expected env-var value 'env-sys-v1' in output; got: {output!r}" ) + @patch('mlpstorage_py.run_summary.logger') + def test_banner_environment_includes_all_mlpstorage_rows(self, mock_logger, + monkeypatch): + """Environment section contains all five MLPSTORAGE_* env-var rows + added by Plan 05-04 (RESULTS_DIR, SYSTEMNAME, ORGNAME, DATA_DIR, + CHECKPOINT_FOLDER). + """ + from mlpstorage_py.run_summary import print_run_summary + + args = _make_args(orgname='Acme', systemname='sys-v1', + mode='closed', results_dir='/r') + print_run_summary(args) + + output = _joined_status_calls(mock_logger) + for row_label in ( + 'MLPSTORAGE_RESULTS_DIR', + 'MLPSTORAGE_SYSTEMNAME', + 'MLPSTORAGE_ORGNAME', + 'MLPSTORAGE_DATA_DIR', + 'MLPSTORAGE_CHECKPOINT_FOLDER', + ): + assert row_label in output, ( + f"Expected {row_label} row in Environment section; " + f"got: {output!r}" + ) + class TestOutputOnlyDenylist: """Output-only knobs (quiet/debug/verbose/stream_log_level) never appear as their own rows."""