From 2addb926662e9e3318141ac4d6227b09d6588f12 Mon Sep 17 00:00:00 2001 From: Elias Bermudez <6505145+debermudez@users.noreply.github.com> Date: Mon, 31 Aug 2026 12:22:42 -0700 Subject: [PATCH] fix(config): stop silently dropping CLI flags passed with --config (#1280) Signed-off-by: Elias Bermudez --- .cursor/rules/python.mdc | 4 + .github/copilot-instructions.md | 4 + AGENTS.md | 4 + CLAUDE.md | 4 + docs/cli-options.md | 4 +- docs/dev/global-invariants.md | 125 ++++ .../config/flags/_config_flag_routing.py | 491 +++++++++++++ src/aiperf/config/flags/_converter_dataset.py | 439 +++++++++-- .../config/flags/_converter_profiling.py | 39 +- src/aiperf/config/flags/_converter_runtime.py | 9 +- .../config/flags/_converter_telemetry.py | 33 +- src/aiperf/config/flags/_converter_warmup.py | 46 +- src/aiperf/config/flags/_section_fields.py | 2 + src/aiperf/config/flags/cli_config.py | 7 +- src/aiperf/config/flags/resolver.py | 636 +++++++++++----- src/aiperf/config/loader/parsing.py | 24 +- ...st_random_pool_batch_size_yaml_override.py | 114 ++- .../test_random_seed_envelope_override.py | 75 ++ .../common/config/test_config_validators.py | 29 + tests/unit/config/test_config_flag_routing.py | 317 ++++++++ .../config/test_config_input_overrides.py | 694 ++++++++++++++++++ .../config/test_config_override_invariants.py | 554 ++++++++++++++ .../config/test_config_phase_overrides.py | 354 +++++++++ .../config/test_config_telemetry_overrides.py | 251 +++++++ tests/unit/config/test_trace_flag_routing.py | 36 +- 25 files changed, 3969 insertions(+), 326 deletions(-) create mode 100644 src/aiperf/config/flags/_config_flag_routing.py create mode 100644 tests/unit/cli_runner/test_random_seed_envelope_override.py create mode 100644 tests/unit/config/test_config_flag_routing.py create mode 100644 tests/unit/config/test_config_input_overrides.py create mode 100644 tests/unit/config/test_config_override_invariants.py create mode 100644 tests/unit/config/test_config_phase_overrides.py create mode 100644 tests/unit/config/test_config_telemetry_overrides.py diff --git a/.cursor/rules/python.mdc b/.cursor/rules/python.mdc index bdf5cce17f..d532653a40 100644 --- a/.cursor/rules/python.mdc +++ b/.cursor/rules/python.mdc @@ -51,6 +51,10 @@ Python 3.11+ async AI benchmarking tool for measuring LLM inference server perfo Numeric metric values crossing a serialization boundary or feeding a numerical algorithm must be finite or explicitly `None`. Use `aiperf.common.finite` (`FiniteFloat`, `scrub_non_finite`, `nan_safe_mean`/`std`, `is_finite_value`). Mechanical CI invariants in `tests/unit/property/test_finite_invariants.py` reject new violations and ratchet existing debt to zero via baseline files. See [`docs/dev/patterns.md`](docs/dev/patterns.md) § "NaN/Inf Discipline Pattern" and [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) for the full contract. +## CLI Flag Routing Under `--config` + +A CLI flag passed alongside `--config` MUST either change the resolved config or raise an error naming the flag — never be silently ignored, which for a benchmarking tool means publishing numbers that do not match what was asked for. `CLIConfig` is the source of truth, structurally: `resolve_config` receives a `CLIConfig` and a path and nothing else, so a flag reaches `AIPerfConfig` only by being a field on it. Every field must be classified in `src/aiperf/config/flags/_config_flag_routing.py` as `ROUTED_UNDER_CONFIG`, `UNROUTED_UNDER_CONFIG`, `EXEMPT_FROM_CONFIG_ROUTING`, or one of the conditional sets (`COMPANION_ROUTED`, `MAGIC_LIST_ONLY_UNDER_CONFIG`). Adding a flag without classifying it fails `test_every_cli_config_field_is_classified`; claiming a flag is routed when nothing routes it fails `test_routed_field_never_silently_no_ops`. Prefer routing the flag (check whether a builder already exists) over listing it as unrouted. See [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) § "CLI flag routing under `--config`". + ## Build and Test Commands ```bash diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 35afb07d47..a14e7de782 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -46,6 +46,10 @@ Python 3.11+ async AI benchmarking tool for measuring LLM inference server perfo Numeric metric values crossing a serialization boundary or feeding a numerical algorithm must be finite or explicitly `None`. Use `aiperf.common.finite` (`FiniteFloat`, `scrub_non_finite`, `nan_safe_mean`/`std`, `is_finite_value`). Mechanical CI invariants in `tests/unit/property/test_finite_invariants.py` reject new violations and ratchet existing debt to zero via baseline files. See [`docs/dev/patterns.md`](docs/dev/patterns.md) § "NaN/Inf Discipline Pattern" and [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) for the full contract. +## CLI Flag Routing Under `--config` + +A CLI flag passed alongside `--config` MUST either change the resolved config or raise an error naming the flag — never be silently ignored, which for a benchmarking tool means publishing numbers that do not match what was asked for. `CLIConfig` is the source of truth, structurally: `resolve_config` receives a `CLIConfig` and a path and nothing else, so a flag reaches `AIPerfConfig` only by being a field on it. Every field must be classified in `src/aiperf/config/flags/_config_flag_routing.py` as `ROUTED_UNDER_CONFIG`, `UNROUTED_UNDER_CONFIG`, `EXEMPT_FROM_CONFIG_ROUTING`, or one of the conditional sets (`COMPANION_ROUTED`, `MAGIC_LIST_ONLY_UNDER_CONFIG`). Adding a flag without classifying it fails `test_every_cli_config_field_is_classified`; claiming a flag is routed when nothing routes it fails `test_routed_field_never_silently_no_ops`. Prefer routing the flag (check whether a builder already exists) over listing it as unrouted. See [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) § "CLI flag routing under `--config`". + ## Build and Test Commands ```bash diff --git a/AGENTS.md b/AGENTS.md index 59643c536e..ed011257f0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,10 @@ Python 3.11+ async AI benchmarking tool for measuring LLM inference server perfo Numeric metric values crossing a serialization boundary or feeding a numerical algorithm must be finite or explicitly `None`. Use `aiperf.common.finite` (`FiniteFloat`, `scrub_non_finite`, `nan_safe_mean`/`std`, `is_finite_value`). Mechanical CI invariants in `tests/unit/property/test_finite_invariants.py` reject new violations and ratchet existing debt to zero via baseline files. See [`docs/dev/patterns.md`](docs/dev/patterns.md) § "NaN/Inf Discipline Pattern" and [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) for the full contract. +## CLI Flag Routing Under `--config` + +A CLI flag passed alongside `--config` MUST either change the resolved config or raise an error naming the flag — never be silently ignored, which for a benchmarking tool means publishing numbers that do not match what was asked for. `CLIConfig` is the source of truth, structurally: `resolve_config` receives a `CLIConfig` and a path and nothing else, so a flag reaches `AIPerfConfig` only by being a field on it. Every field must be classified in `src/aiperf/config/flags/_config_flag_routing.py` as `ROUTED_UNDER_CONFIG`, `UNROUTED_UNDER_CONFIG`, `EXEMPT_FROM_CONFIG_ROUTING`, or one of the conditional sets (`COMPANION_ROUTED`, `MAGIC_LIST_ONLY_UNDER_CONFIG`). Adding a flag without classifying it fails `test_every_cli_config_field_is_classified`; claiming a flag is routed when nothing routes it fails `test_routed_field_never_silently_no_ops`. Prefer routing the flag (check whether a builder already exists) over listing it as unrouted. See [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) § "CLI flag routing under `--config`". + ## Build and Test Commands ```bash diff --git a/CLAUDE.md b/CLAUDE.md index d8f9360b7b..923a42389b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,6 +45,10 @@ Python 3.11+ async AI benchmarking tool for measuring LLM inference server perfo Numeric metric values crossing a serialization boundary or feeding a numerical algorithm must be finite or explicitly `None`. Use `aiperf.common.finite` (`FiniteFloat`, `scrub_non_finite`, `nan_safe_mean`/`std`, `is_finite_value`). Mechanical CI invariants in `tests/unit/property/test_finite_invariants.py` reject new violations and ratchet existing debt to zero via baseline files. See [`docs/dev/patterns.md`](docs/dev/patterns.md) § "NaN/Inf Discipline Pattern" and [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) for the full contract. +## CLI Flag Routing Under `--config` + +A CLI flag passed alongside `--config` MUST either change the resolved config or raise an error naming the flag — never be silently ignored, which for a benchmarking tool means publishing numbers that do not match what was asked for. `CLIConfig` is the source of truth, structurally: `resolve_config` receives a `CLIConfig` and a path and nothing else, so a flag reaches `AIPerfConfig` only by being a field on it. Every field must be classified in `src/aiperf/config/flags/_config_flag_routing.py` as `ROUTED_UNDER_CONFIG`, `UNROUTED_UNDER_CONFIG`, `EXEMPT_FROM_CONFIG_ROUTING`, or one of the conditional sets (`COMPANION_ROUTED`, `MAGIC_LIST_ONLY_UNDER_CONFIG`). Adding a flag without classifying it fails `test_every_cli_config_field_is_classified`; claiming a flag is routed when nothing routes it fails `test_routed_field_never_silently_no_ops`. Prefer routing the flag (check whether a builder already exists) over listing it as unrouted. See [`docs/dev/global-invariants.md`](docs/dev/global-invariants.md) § "CLI flag routing under `--config`". + ## Build and Test Commands ```bash diff --git a/docs/cli-options.md b/docs/cli-options.md index 7d015a0e4f..1bd03ebe5a 100644 --- a/docs/cli-options.md +++ b/docs/cli-options.md @@ -577,7 +577,7 @@ Fraction of trace sessions to keep for replay, sampled whole-session to preserve #### `-f`, `--config` `` -Path to a YAML configuration file. CLI flags override values from the config file. +Path to a YAML configuration file. CLI flags override values from the config file. A flag that cannot be applied on top of a config file is rejected by name rather than ignored, so a run never silently differs from what was asked for. ### Fixed Schedule @@ -2168,7 +2168,7 @@ Fraction of trace sessions to keep for replay, sampled whole-session to preserve #### `-f`, `--config` `` -Path to a YAML configuration file. CLI flags override values from the config file. +Path to a YAML configuration file. CLI flags override values from the config file. A flag that cannot be applied on top of a config file is rejected by name rather than ignored, so a run never silently differs from what was asked for. ### Fixed Schedule diff --git a/docs/dev/global-invariants.md b/docs/dev/global-invariants.md index 0b33af7f49..005653fe3a 100644 --- a/docs/dev/global-invariants.md +++ b/docs/dev/global-invariants.md @@ -152,6 +152,131 @@ When you fix a field, just delete its line from the baseline; the test starts enforcing the constraint on that field on the next CI run. +## CLI flag routing under `--config` + +`resolve_config` merges a YAML config file with explicitly-set CLI +flags. Historically it only knew how to route a subset of `CLIConfig` +into the merged dict, and **every other flag was discarded without a +word** — `aiperf profile -f base.yaml --random-seed 42` ran with a +different seed than the user asked for and said nothing. For a +benchmarking tool that silently corrupts published numbers. + +The invariant now in force: + +> A CLI flag passed alongside `--config` either changes the resolved +> config, or raises an error naming the flag. It is never silently +> ignored. + +### `CLIConfig` is the source of truth + +The guarantee is scoped to what `resolve_config` can see, and its +signature is the boundary: + +```python +def resolve_config(cli_config: CLIConfig, config_file: Path | None = None) -> AIPerfConfig +``` + +Both entry points — `aiperf profile` and `aiperf service` — call it with +nothing but their `CLIConfig` and a path. So a CLI option can influence +the resolved `AIPerfConfig` **only** by being a field on `CLIConfig`, +and that is a structural property of the call, not a convention someone +has to remember. It is what lets the classification test enumerate +`CLIConfig.model_fields` and claim completeness: there is no second +source of flags for it to miss. + +Two consequences worth stating plainly: + +- **Command-level parameters are out of scope, by construction.** + `aiperf service` declares `--service-id`, `--health-host`, + `--health-port` and its `service_type` argument directly on the + command function rather than on `CLIConfig`. They are service-instance + plumbing, never passed to `resolve_config`, and so cannot be dropped + by this mechanism — they never enter it. Anything that *is* benchmark + configuration belongs on `CLIConfig`, precisely so this guarantee + covers it. +- **The assumption is falsifiable, and this is how it would break.** + Giving `resolve_config` another parameter that carries user intent, or + having it read flags from module state or the environment, puts values + into the resolved config that no test enumerates. If you need to do + that, extend the classification to cover the new source in the same + breath — otherwise the suite will keep reporting a completeness it no + longer has. + +Every field on `CLIConfig` must be classified in +[`_config_flag_routing.py`](https://github.com/ai-dynamo/aiperf/tree/main/src/aiperf/config/flags/_config_flag_routing.py): + +| Set | Meaning | +| --- | --- | +| `ROUTED_UNDER_CONFIG` | Reaches `AIPerfConfig`. Derived from the resolver's own routing tables where possible, so it cannot drift from them. | +| `UNROUTED_UNDER_CONFIG` | Known not to route. Raises `ConfigurationError` naming the flag. | +| `EXEMPT_FROM_CONFIG_ROUTING` | Not benchmark config at all (`--config` itself). Each entry needs a stated reason. | +| `COMPANION_ROUTED` | Routes only alongside another flag (`--model-selection-strategy` needs `--model-names`). Rejected when the companion is absent. | +| `MAGIC_LIST_ONLY_UNDER_CONFIG` | Routes in list form only (`--isl 128 256` becomes a sweep parameter; scalar `--isl 128` goes to the dataset). Decided per value. | + +### Why the guarantee holds + +Four layers, each covering the previous one's blind spot: + +1. **The universe is derived, not maintained.** Every set is + subtracted from `frozenset(CLIConfig.model_fields)`. You cannot add + a CLI option without adding a field, so nothing is checked against + a list someone has to remember to update. +2. **Runtime is default-deny.** `reject_unrouted_cli_flags` computes + `model_fields_set - ROUTED - EXEMPT` and raises on the remainder — + "is this known-good?", not "is this known-bad?". It gates on + `model_fields_set`, never truthiness, so `--prompt-batch-size 0` + counts as set. +3. **Classification is mandatory.** + `test_every_cli_config_field_is_classified` fails when a field + belongs to none of the sets, so a new flag fails CI at authoring + time. +4. **The classification is verified, not trusted.** Layer 3 only + proves someone applied a label. + `test_routed_field_never_silently_no_ops` drives every routed field + against both a synthetic and a file dataset and asserts it changes + the config or raises. This is what catches whole-section + classifications (`OUTPUT`/`TOKENIZER`/`ACCURACY`/`SWEEPING` are + marked routed wholesale, so a new member is auto-classified and + layer 3 passes vacuously). + +Two further invariants guard the merge itself: +`test_override_emits_no_key_the_user_did_not_set` runs `build_dataset` +twice with different values for one field — any emitted key identical +across both runs is a materialized default that would overwrite a YAML +value the user never mentioned. `test_routed_dataset_field_actually_changes_the_resolved_config` +covers the dataset block specifically. + +### Adding a new CLI flag + +1. Add the field to `CLIConfig` as usual. CI now fails with + `CLI fields are unclassified for the --config path: [...]`. +2. Decide what the flag should do under `--config`: + - **Route it** (preferred). Dataset-shaping fields flow through + `build_dataset` automatically. Otherwise wire it into + `build_cli_overrides` — check first whether a builder already + exists, as `build_mlflow`/`build_otel`/`build_network_latency` + did while going uncalled. + - **Or list it in `UNROUTED_UNDER_CONFIG`**, so users get an error + naming the flag rather than a wrong benchmark. +3. If the invariant suite cannot generate a value from the + annotation (`typing.Any`, free-form strings), add one to + `FIELD_PROBE_VALUES` or list the field in `UNDRIVABLE_FIELDS` with + a reason. Do not let it report as skipped — a skip is invisible in + CI, which is exactly how a coverage hole hides. + +### Known gaps + +`--sweep-type` and `--disable-auto-fixed-schedule` are unrouted and +loud: the first needs a sweep block to attach to, the second is +consumed during phase construction, which this path does not rebuild. + +The flags in `SWEEP_FIELDS_NOT_ROUTED` (`--concurrency-min/max/steps`, +`--isl-*`/`--osl-*`, the `*-sla-ms` filters, `--parameter-sweep-*`) +resolve cleanly and change nothing, so they are rejected. Verified +individually — notably `--ttft-sla-ms` does **not** take effect even +alongside `--search-recipe` and `--streaming`. Routing them is +follow-up work; until then the failure is loud. + ## Extending the suite ### Adding a new mechanical invariant diff --git a/src/aiperf/config/flags/_config_flag_routing.py b/src/aiperf/config/flags/_config_flag_routing.py new file mode 100644 index 0000000000..f650a82f96 --- /dev/null +++ b/src/aiperf/config/flags/_config_flag_routing.py @@ -0,0 +1,491 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Which CLI flags actually reach ``AIPerfConfig`` when ``--config`` is used. + +``resolve_config`` merges a YAML base with explicitly-set CLI flags, but it +only knows how to route a subset of ``CLIConfig`` into the merged dict. Any +flag outside that subset used to be discarded without a word: a user running +``aiperf profile -f base.yaml --random-seed 42`` got a different seed than +they asked for and no diagnostic. For a benchmarking tool that silently +corrupts published numbers. + +``CLIConfig`` is the source of truth, and that is structural rather than a +convention: ``resolve_config`` takes a ``CLIConfig`` and a path, and both +``aiperf profile`` and ``aiperf service`` call it with nothing else. A flag +can therefore reach ``AIPerfConfig`` only by being a field on ``CLIConfig``, +which is what lets the classification test enumerate ``model_fields`` and +claim completeness. Give ``resolve_config`` another source of user intent +and that claim quietly stops holding -- extend the classification in the +same change. (``aiperf service``'s own ``--service-id`` / ``--health-*`` +parameters live on the command, never reach this path, and are out of scope +by construction.) + +This module makes the boundary explicit and enforceable: + +``ROUTED_UNDER_CONFIG`` + Fields the resolver genuinely routes. Derived from the same maps the + resolver and converters use, so it cannot drift from them. + +``UNROUTED_UNDER_CONFIG`` + Fields known to be dropped. Written out by hand on purpose -- a computed + complement would make ``test_every_section_field_is_classified`` vacuous. + A newly-added CLI flag lands in neither set and fails that test at + authoring time, which is the whole point. + +:func:`reject_unrouted_cli_flags` turns the drop into a ``ConfigurationError`` +naming the offending flags. Repairing the routing (so entries move from +``UNROUTED`` to ``ROUTED``) is separate work; until then users get a loud +error instead of a wrong benchmark. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from aiperf.config.flags._section_fields import ( + ACCURACY_FIELDS, + ENDPOINT_FIELDS, + INPUT_FIELDS, + LOADGEN_FIELDS, + OUTPUT_FIELDS, + SWEEPING_FIELDS, + TOKENIZER_FIELDS, +) + +if TYPE_CHECKING: + from aiperf.config.flags import CLIConfig + +ALL_SECTION_FIELDS: frozenset[str] = ( + ACCURACY_FIELDS + | ENDPOINT_FIELDS + | INPUT_FIELDS + | LOADGEN_FIELDS + | OUTPUT_FIELDS + | SWEEPING_FIELDS + | TOKENIZER_FIELDS +) + + +# Fields that are not benchmark configuration at all, so "routed under +# --config" does not apply to them. Each needs a reason: an unexplained +# exemption is a silent drop with extra steps. +EXEMPT_FROM_CONFIG_ROUTING: frozenset[str] = frozenset( + { + # The --config flag itself; it selects the file being merged. + "config_file", + # Alias input consumed during CLIConfig validation rather than routed + # onto AIPerfConfig; it populates its canonical sibling, which is + # itself classified. + "stream", + } +) + +# Fields outside every section frozenset that nonetheless reach AIPerfConfig, +# verified by resolving with the flag set and diffing against the no-flag +# baseline. The section sets are opt-in and incomplete; this records what is +# genuinely routed rather than letting absence imply anything. +_ROUTED_OUTSIDE_SECTIONS: frozenset[str] = frozenset( + { + # service runtime / logging, via build_logging_runtime + "api_host", + "api_port", + "extra_verbose", + "log_level", + "record_processor_service_count", + "stats_interval", + "ui_type", + "verbose", + "workers_max", + "zmq_dual_bind", + "zmq_ipc_path", + "zmq_tcp_host", + # telemetry collectors already wired into build_cli_overrides + "gpu_telemetry", + "no_gpu_telemetry", + "server_metrics", + "no_server_metrics", + "server_metrics_formats", + # wandb, via build_wandb (the non-project flags are guarded loudly + # rather than dropped when --wandb-project is absent) + "wandb_project", + "wandb_entity", + "wandb_run_name", + "wandb_tags", + # agentic phase fields, via _apply_agentic_replay_fields + "agentic_cache_warmup_duration", + "agentic_warmup_grace_period", + # publishing targets, via build_mlflow / build_otel / + # build_network_latency -- the builders the CLI-only converter has + # always used, now called by build_cli_overrides too + "mlflow_artifact_globs", + "mlflow_experiment", + "mlflow_parent_run_id", + "mlflow_run_name", + "mlflow_tags", + "mlflow_tracking_uri", + "otel_resource_attributes", + "otel_url", + "gen_ai_provider", + "network_latency_automatic", + "network_latency_mean", + "network_latency_ping_interval", + # scenario lock, via _apply_scenario_overrides + "scenario", + "unsafe_override", + # inter-turn delay cap, carried onto trace datasets by build_dataset + "inter_turn_delay_cap_seconds", + # verbatim system prompt, carried onto the dataset by + # _VERBATIM_DATASET_FIELDS (added on main by #1268) + "system_prompt", + "system_prompt_file", + # baseten_trace dataset knobs carried by _VERBATIM_DATASET_FIELDS; + # against another format they raise rather than drop + "force_min_tokens", + "max_idle_gap_cap_seconds", + "omit_kv_hints", + "open_loop_replay", + "open_loop_strict", + "replay_speedup", + "trace_session_sample_ratio", + } +) + + +# SWEEPING members that resolve cleanly under --config and change nothing, +# each verified by resolving with the flag and diffing the result. Notably +# --ttft-sla-ms does NOT take effect even alongside --search-recipe plus +# --streaming, contrary to the example in resolver's module docstring. +SWEEP_FIELDS_NOT_ROUTED: frozenset[str] = frozenset( + { + "concurrency_max", + "concurrency_min", + "concurrency_steps", + "convergence_mode", + "convergence_stat", + "degradation_metric_tag", + "degradation_stat", + "e2e_sla_ms", + "isl_max", + "isl_min", + "isl_steps", + "itl_sla_ms", + "osl_max", + "osl_min", + "osl_steps", + "parameter_sweep_cooldown_seconds", + "parameter_sweep_mode", + "parameter_sweep_same_seed", + "search_style", + "sweep_variants", + "tpot_sla_ms", + "ttft_sla_ms", + } +) + + +# Flags that only route when a companion flag is also present, verified by +# resolving with and without the companion. Without it they resolve cleanly +# and change nothing, so they must be rejected rather than quietly ignored. +COMPANION_ROUTED: dict[str, frozenset[str]] = { + # _apply_endpoint_overrides writes `models.strategy` only inside the + # `model_names` branch. + "model_selection_strategy": frozenset({"model_names"}), + # _apply_phase_loadgen_overrides consults arrival_pattern only when + # rewriting a phase for --request-rate-series. + "arrival_pattern": frozenset({"request_rate_series"}), + # EndpointConfig._validate_per_chunk_usage requires server token counts, + # streaming, and a chat endpoint; on its own the flag can only be set to + # its default. + "per_chunk_usage": frozenset({"use_server_token_count", "streaming"}), +} + + +# Flags that would replace the dataset the config file declared rather than +# shape it: its source file, its public-dataset identity, its format. The YAML +# owns those, and build_dataset drops the corresponding keys in override mode, +# so the flags must keep erroring instead of appearing to work. +DATASET_SOURCE_FIELDS: frozenset[str] = frozenset( + { + # Switch the dataset TYPE, not just a value inside it. The variants + # barely overlap -- FileDataset carries a dozen fields PublicDataset + # rejects -- so accepting these would silently invalidate whole groups + # of keys the user wrote in the config file. + "public_dataset", + "hf_weka_dataset", + } +) + +# Source flags that change a value WITHIN the declared type: the trace file +# and the loader that reads it. Overriding these is the "same benchmark, +# different trace" loop, and is no more surprising than --model overriding +# models.items. build_dataset applies them only when the config file declares +# type: file; against any other type they raise, because that would be a type +# switch wearing a different hat. +DATASET_SOURCE_IN_TYPE_FIELDS: frozenset[str] = frozenset( + {"input_file", "custom_dataset_type"} +) + +# INPUT_FIELDS members that build_dataset does not carry: they land on the +# slos block or the profiling phase instead. They ARE routed (see +# _apply_phase_loadgen_overrides and the goodput handling in +# build_cli_overrides); this set exists to keep them out of the dataset +# reconciliation, which would otherwise report them as inert because +# build_dataset legitimately emits nothing for them. +_INPUT_NOT_ON_DATASET: frozenset[str] = frozenset( + { + # --goodput lands on the benchmark `slos` block. + "goodput", + # The fixed-schedule flags select and shape the profiling phase, so + # they are routed by _apply_phase_loadgen_overrides rather than + # carried on the dataset -- and must stay out of the dataset + # reconciliation, which would otherwise report them as inert. + "fixed_schedule", + "fixed_schedule_auto_offset", + "fixed_schedule_end_offset", + "fixed_schedule_start_offset", + } +) + + +# Dataset-carried fields that are not INPUT_FIELDS members. build_dataset +# reads them (via _VERBATIM_DATASET_FIELDS and the trace helpers), so the +# per-flag reconciliation in _apply_dataset_overrides must consider them too; +# keying that check on INPUT_FIELDS alone let them no-op silently. +DATASET_FIELDS_OUTSIDE_INPUT: frozenset[str] = frozenset( + { + "force_min_tokens", + "inter_turn_delay_cap_seconds", + "max_idle_gap_cap_seconds", + "omit_kv_hints", + "open_loop_replay", + "open_loop_strict", + "replay_speedup", + "system_prompt", + "system_prompt_file", + "trace_idle_gap_cap_seconds", + "trace_session_sample_ratio", + } +) + +# INPUT fields that _apply_dataset_overrides carries onto the dataset block. +# Used to decide whether a config file lacking a dataset is an error for this +# invocation or simply nothing to do. +DATASET_OVERRIDE_FIELDS: frozenset[str] = ( + INPUT_FIELDS - DATASET_SOURCE_FIELDS - _INPUT_NOT_ON_DATASET +) - {"headers", "extra_inputs"} + + +def _build_routed_under_config() -> frozenset[str]: + """Derive the routed set from the resolver's own routing tables. + + Imports are local: ``resolver`` imports this module's reject helper + lazily, and reaching back into it at module scope would close the cycle. + Deriving rather than restating means adding an entry to + ``_ENDPOINT_FIELD_MAP`` or ``_LOADGEN_PHASE_FIELD_MAP`` automatically + counts as routed here. + """ + from aiperf.config.flags._converter_endpoint import _ENDPOINT_FIELD_MAP + from aiperf.config.flags._converter_profiling import ( + _AGENTIC_REPLAY_ROUTES, + _GAMMA_ONLY_ROUTES, + _RAMP_FIELDS, + ) + from aiperf.config.flags.resolver import _LOADGEN_PHASE_FIELD_MAP + + # _apply_endpoint_overrides: the field map, plus --url and the + # --model-names/--model-selection-strategy pair that lands on `models`. + endpoint = ( + set(_ENDPOINT_FIELD_MAP) + | { + "urls", + "model_names", + "model_selection_strategy", + } + | { + # probe sub-blocks, via _maybe_build_reset_kv_cache / + # _maybe_build_server_profiler + field + for field in ENDPOINT_FIELDS + if field.startswith(("reset_kv_cache", "server_profiler")) + } + ) + + # _apply_input_overrides routes headers/extra onto the endpoint block; + # every other dataset-shaping field goes through _apply_dataset_overrides, + # which delegates to build_dataset in override mode. + # + # The exceptions are the fields that would replace the dataset the config + # file declared rather than shape it -- source, type, and format. The YAML + # owns those, so the flags stay rejected (see DATASET_SOURCE_FIELDS). + inputs = (INPUT_FIELDS - DATASET_SOURCE_FIELDS) - _INPUT_NOT_ON_DATASET + inputs |= DATASET_SOURCE_IN_TYPE_FIELDS + + # _apply_phase_loadgen_overrides: the phase field map, the rate-series + # pair, and the AGENTIC_REPLAY fields shared with BasePhaseConfig. + loadgen = ( + {attr for attr, _ in _LOADGEN_PHASE_FIELD_MAP} + | {"request_rate_series", "arrival_pattern"} + | (set(_AGENTIC_REPLAY_ROUTES) & LOADGEN_FIELDS) + # Phase shaping, applied by _apply_phase_shaping_overrides through the + # converter's own helpers. Derived from the same tables those helpers + # read, so a new ramp or gamma-only route counts as routed here too. + | {attr for attr, _ in _RAMP_FIELDS} + | {attr for _, attr in _GAMMA_ONLY_ROUTES} + | {"request_cancellation_rate", "request_cancellation_delay"} + # Warmup phase shaping, applied by _apply_warmup_overrides. + | {field for field in LOADGEN_FIELDS if field.startswith("warmup_")} + # Fixed-schedule phase selection and its offsets, and --goodput, which + # lands on the benchmark slos block. + | {field for field in INPUT_FIELDS if field.startswith("fixed_schedule")} + | {"goodput"} + ) + + # Whole-section builders consumed by build_cli_overrides: build_artifacts + # + resolve_auto_plot, build_tokenizer, build_accuracy. + whole_sections = OUTPUT_FIELDS | TOKENIZER_FIELDS | ACCURACY_FIELDS + + # SWEEPING minus the members verified to resolve cleanly while changing + # nothing (see SWEEP_FIELDS_NOT_ROUTED). The rest -- --search-recipe, + # --search-space and friends -- do take effect. build_sweep / expand_search_recipe + # consume the section, but only conditionally -- e.g. --concurrency-min / + # --concurrency-max / --concurrency-steps under --config verifiably + # produce no sweep at all, while --ttft-sla-ms does take effect alongside + # --search-recipe. Separating genuine drops from "needs a companion flag" + # requires a combination audit that is out of scope here, and erroring on + # the whole section would break valid recipe invocations. Tracked as + # follow-up work; see AIP-1133. + sweeping = set(SWEEPING_FIELDS) - SWEEP_FIELDS_NOT_ROUTED + + return frozenset( + endpoint + | inputs + | loadgen + | whole_sections + | sweeping + | _ROUTED_OUTSIDE_SECTIONS + | DATASET_FIELDS_OUTSIDE_INPUT + ) + + +ROUTED_UNDER_CONFIG: frozenset[str] = _build_routed_under_config() + + +def _build_magic_list_only_fields() -> frozenset[str]: + """Dataset fields whose list form routes as a sweep parameter. + + ``promote_benchmark_magic_lists`` hoists a list-shaped ``--isl 128 256`` + into a ``sweep`` parameter; the scalar form routes onto the dataset block + through ``_apply_dataset_overrides``. Both forms take effect, so these + fields are routed either way. The set records which fields have the two + distinct destinations, so tests (and ``MAGIC_LIST_ONLY_UNDER_CONFIG`` + consumers generally) can assert against the right one. + """ + from aiperf.config.flags.converter import _CLI_DATASET_MAGIC_LIST_PATHS + + return frozenset(attr for attr, _ in _CLI_DATASET_MAGIC_LIST_PATHS) + + +MAGIC_LIST_ONLY_UNDER_CONFIG: frozenset[str] = _build_magic_list_only_fields() + +# Fields silently discarded when --config is supplied. Listed explicitly so +# that a new CLI flag belongs to neither set and trips the classification +# test. Entries move to ROUTED_UNDER_CONFIG as routing is repaired. +UNROUTED_UNDER_CONFIG: frozenset[str] = frozenset( + { + # ----- endpoint ----- + # ----- input: dataset identity, owned by the config file ----- + # Every other INPUT field now routes through _apply_dataset_overrides; + # these would swap the dataset the YAML declared rather than shape it. + *DATASET_SOURCE_FIELDS, + # ----- sweep flags that resolve cleanly and do nothing ----- + *SWEEP_FIELDS_NOT_ROUTED, + # ----- outside every section frozenset ----- + # Still dropped: --sweep-type needs a sweep block to attach to, and + # --disable-auto-fixed-schedule is consumed by phase construction + # which this path does not rebuild. Both need their own routing. + "sweep_type", + "disable_auto_fixed_schedule", + # ----- loadgen: ramps, pacing, cancellation ----- + } +) + + +def flag_names_for(field: str) -> tuple[str, ...]: + """Return the CLI flag spellings for a ``CLIConfig`` field. + + Reads the ``CLIParameter`` annotation metadata rather than kebab-casing + the attribute, because several fields do not follow from their name -- + ``conversation_num`` is ``--num-conversations``, ``warmup_request_count`` + is also ``--num-warmup-requests``. Error messages have to name flags the + user can actually type. + + Returns an empty tuple when the field carries no ``CLIParameter``. + """ + from aiperf.config.flags import CLIConfig + + info = CLIConfig.model_fields.get(field) + if info is None: + return () + # Fields declare their flags via CLIParameter or via cyclopts' Parameter + # (--parameter-sweep-same-seed uses the latter). Match structurally on a + # `name` tuple of dash-prefixed spellings rather than on either class, so + # a third declaration style still yields real flag names. + for meta in info.metadata: + names = getattr(meta, "name", None) + if isinstance(names, (tuple, list)) and any( + isinstance(n, str) and n.startswith("--") for n in names + ): + return tuple(n for n in names if isinstance(n, str)) + return () + + +def _describe(field: str) -> str: + """Render a field as every spelling it accepts, falling back to the name. + + Naming only the first declared spelling meant a user who typed + ``--num-warmup-requests`` was told about ``--warmup-request-count``, and + ``--num-conversations`` was reported as ``--conversation-num`` -- flags + absent from their shell history. This layer cannot see which spelling was + typed (the CLIConfig it receives has already been parsed), so it names + them all rather than guessing. + """ + names = flag_names_for(field) + return "/".join(names) if names else field + + +def reject_unrouted_cli_flags(cli: CLIConfig) -> None: + """Raise when explicitly-set CLI flags cannot be routed under ``--config``. + + Gating is on ``cli.model_fields_set`` -- a flag the user did not pass can + never trigger this, and a flag passed with a falsy value (``--concurrency 0``) + still counts as set. + + Args: + cli: the parsed ``CLIConfig`` for this invocation. + + Raises: + ConfigurationError: naming every offending flag at once, so a user + fixing a long command line learns about all of them in one run. + """ + from aiperf.config.loader.errors import ConfigurationError + + # Keyed on every CLIConfig field rather than on the section frozensets: + # those are opt-in, and a field nobody sectioned was invisible here. + unrouted = cli.model_fields_set - ROUTED_UNDER_CONFIG - EXEMPT_FROM_CONFIG_ROUTING + # Companion-routed flags are inert on their own, so treat a missing + # companion the same as no routing at all. + unrouted |= { + field + for field, companions in COMPANION_ROUTED.items() + if field in cli.model_fields_set and not (companions & cli.model_fields_set) + } + if not unrouted: + return + + flags = ", ".join(sorted(_describe(field) for field in unrouted)) + raise ConfigurationError( + f"These CLI flags cannot be combined with --config and would " + f"otherwise be ignored: {flags}. Set the equivalent values in the " + f"YAML config file, or drop --config and pass the run entirely on " + f"the command line." + ) diff --git a/src/aiperf/config/flags/_converter_dataset.py b/src/aiperf/config/flags/_converter_dataset.py index b165e3b2a4..549c71941d 100644 --- a/src/aiperf/config/flags/_converter_dataset.py +++ b/src/aiperf/config/flags/_converter_dataset.py @@ -359,6 +359,19 @@ def _resolve_entries(cli: CLIConfig) -> int | None: return None +# Per-modality subtables only SyntheticDataset carries. File and public +# datasets take their values from the trace, and leaking one of these onto +# them trips extra_forbidden at validation. +_SYNTHETIC_ONLY_SUBTABLES: tuple[str, ...] = ( + "prompts", + "prefix_prompts", + "rankings", + "audio", + "images", + "video", +) + + def _apply_dataset_type(d: dict[str, Any], cli: CLIConfig, needs_text: bool) -> None: from aiperf.common.enums import DatasetType @@ -376,14 +389,7 @@ def _apply_dataset_type(d: dict[str, Any], cli: CLIConfig, needs_text: bool) -> d["entries"] = entries d["_entries_explicit"] = entries_explicit # PublicDataset doesn't carry per-modality subtables. - for key in ( - "prompts", - "prefix_prompts", - "rankings", - "audio", - "images", - "video", - ): + for key in _SYNTHETIC_ONLY_SUBTABLES: d.pop(key, None) return if cli.input_file: @@ -393,14 +399,7 @@ def _apply_dataset_type(d: dict[str, Any], cli: CLIConfig, needs_text: bool) -> # FileDataset only carries synthesis + osl as auxiliary fields. The # synthetic-only subtables are dropped here; --osl is handled by # _apply_file_osl. - for key in ( - "prompts", - "prefix_prompts", - "rankings", - "audio", - "images", - "video", - ): + for key in _SYNTHETIC_ONLY_SUBTABLES: d.pop(key, None) return d["type"] = DatasetType.SYNTHETIC @@ -466,7 +465,15 @@ def _apply_random_corpus_style_and_range_ratio( d.setdefault("prompts", {})["random_range_ratio"] = cli.prompt_random_range_ratio -def _apply_turns(d: dict[str, Any], cli: CLIConfig) -> None: +def _apply_turns(d: dict[str, Any], cli: CLIConfig, *, only_set: bool = False) -> None: + """Route conversation turn shaping onto the dataset dict. + + ``only_set`` suppresses the companion value each pair normally carries. + Setting just ``--conversation-turn-mean`` emits ``stddev`` too, which is + the right default when the CLI builds a whole dataset from nothing, but + would overwrite a YAML ``turns.stddev`` the user never mentioned when the + result is merged as an override. + """ fields_set = cli.model_fields_set if ( "conversation_turn_mean" in fields_set @@ -476,18 +483,22 @@ def _apply_turns(d: dict[str, Any], cli: CLIConfig) -> None: # placeholder; the sweep block carries the full list. v = cli.conversation_turn_mean turn_mean = v[0] if isinstance(v, list) and v else v - d["turns"] = { - "mean": turn_mean, - "stddev": cli.conversation_turn_stddev, - } + turns: dict[str, Any] = {} + if not only_set or "conversation_turn_mean" in fields_set: + turns["mean"] = turn_mean + if not only_set or "conversation_turn_stddev" in fields_set: + turns["stddev"] = cli.conversation_turn_stddev + d["turns"] = turns if ( "conversation_turn_delay_mean" in fields_set or "conversation_turn_delay_stddev" in fields_set ): - d["turn_delay"] = { - "mean": cli.conversation_turn_delay_mean, - "stddev": cli.conversation_turn_delay_stddev, - } + turn_delay: dict[str, Any] = {} + if not only_set or "conversation_turn_delay_mean" in fields_set: + turn_delay["mean"] = cli.conversation_turn_delay_mean + if not only_set or "conversation_turn_delay_stddev" in fields_set: + turn_delay["stddev"] = cli.conversation_turn_delay_stddev + d["turn_delay"] = turn_delay if "conversation_turn_delay_ratio" in fields_set: d["turn_delay_ratio"] = cli.conversation_turn_delay_ratio @@ -554,6 +565,34 @@ def _apply_implicit_media_batch(d: dict[str, Any], cli: CLIConfig) -> None: media["batch_size"] = 1 +def apply_implicit_media_batch_override( + override: dict[str, Any], existing_dataset: dict[str, Any] +) -> None: + """Materialize batch_size=1 for a media block the override newly shapes, + when neither the override nor the existing YAML dataset already carries + a batch_size for that modality. + + ``_apply_implicit_media_batch`` is CLI-only-path behavior: applied + blindly as an override it would reset a YAML batch size the user never + mentioned. But when the YAML has no block for that modality at all, + there is nothing to protect, and the model default ``batch_size=0`` + means "disabled" -- a media-shape flag (e.g. ``--image-width-mean``) + with no batch_size anywhere then resolves to an *included but empty* + modality instead of the batch size of 1 the CLI-only path applies for + the same flags, and the request silently degrades to text-only. + """ + for media_key in ("images", "audio", "video"): + media = override.get(media_key) + if not media or "batch_size" in media: + continue + existing = existing_dataset.get(media_key) + if isinstance(existing, dict) and ( + "batch_size" in existing or "batchSize" in existing + ): + continue + media["batch_size"] = 1 + + # --- file-dataset incompatibility validation ----------------------------- @@ -615,7 +654,12 @@ def _apply_implicit_media_batch(d: dict[str, Any], cli: CLIConfig) -> None: ) -def _reject_file_dataset_incompatible(cli: CLIConfig) -> None: +def _reject_file_dataset_incompatible( + cli: CLIConfig, + *, + declared_type: Any = None, + declared_format: Any = None, +) -> None: """Reject synthetic-only flags on FILE or PUBLIC (trace) datasets. Flags rejected: prefix prompts, ISL shaping (--isl/--isl-stddev/ @@ -636,15 +680,29 @@ def _reject_file_dataset_incompatible(cli: CLIConfig) -> None: ``FileDataset.osl`` / ``PublicDataset.osl`` by ``_apply_file_osl`` as a per-record fallback. """ - if not cli.input_file and not _implies_public_dataset(cli): + from aiperf.common.enums import DatasetType + from aiperf.plugin.enums import CustomDatasetType + + # Under --config the dataset type comes from the YAML, so --input-file is + # absent even when the target IS a file dataset. Trust the declared value + # when the caller supplies one; fall back to flag inference otherwise. + if declared_type is not None: + if declared_type not in (DatasetType.FILE, DatasetType.PUBLIC): + return + elif not cli.input_file and not _implies_public_dataset(cli): return s = cli.model_fields_set - from aiperf.plugin.enums import CustomDatasetType - is_random_pool = ( - cli.input_file is not None - and cli.custom_dataset_type == CustomDatasetType.RANDOM_POOL - ) + if declared_type is not None: + is_random_pool = ( + declared_type == DatasetType.FILE + and declared_format == CustomDatasetType.RANDOM_POOL + ) + else: + is_random_pool = ( + cli.input_file is not None + and cli.custom_dataset_type == CustomDatasetType.RANDOM_POOL + ) batch_size_attrs = _RANDOM_POOL_BATCH_SIZE_FLAGS non_batch_violations = [ flag @@ -656,9 +714,47 @@ def _reject_file_dataset_incompatible(cli: CLIConfig) -> None: for attr, flag in _FILE_DATASET_INCOMPATIBLE_TRIGGERS if attr in s and attr in batch_size_attrs and not is_random_pool ] + _raise_file_dataset_violation( + cli, + non_batch_violations, + batch_violations, + declared_type=declared_type, + declared_format=declared_format, + ) + + +def _raise_file_dataset_violation( + cli: CLIConfig, + non_batch_violations: list[str], + batch_violations: list[str], + *, + declared_type: Any, + declared_format: Any, +) -> None: + """Raise the message that fits how this dataset was chosen. + + Split out of _reject_file_dataset_incompatible to keep it under the + complexity gate; the branching is inherent to the three ways a dataset + can be selected (config file, --input-file, --public-dataset). + + Under --config the user passed neither --input-file nor --public-dataset + -- the config file owns the dataset, and both flags are rejected on that + path -- so the CLI-only advice cannot be followed. + """ + declared = declared_type is not None if non_batch_violations: + flags = ", ".join(non_batch_violations) + if declared: + raise ValueError( + f"{flags} is only supported with synthetic datasets, but the " + f"config file declares type: {declared_type}" + f"{f' (format: {declared_format})' if declared_format else ''}. " + "Set a synthetic dataset in the config file to apply " + "synthetic-only prompt shaping (ISL, prefix prompts, multimodal " + "generation, multi-turn conversation, etc)." + ) raise ValueError( - f"{', '.join(non_batch_violations)} is only supported with synthetic datasets; " + f"{flags} is only supported with synthetic datasets; " "remove --input-file / --public-dataset (use a synthetic dataset) to " "apply synthetic-only prompt shaping (ISL, prefix prompts, multimodal " "generation, multi-turn conversation, etc)." @@ -669,10 +765,13 @@ def _reject_file_dataset_incompatible(cli: CLIConfig) -> None: # not first told to add --custom-dataset-type random_pool and only then rejected # at load time. The loader repeats an equivalent check for the pool shapes that # are only visible once the files are parsed (inline records, named entries). + s = cli.model_fields_set set_batch_flags = [ flag for attr, flag in _FILE_DATASET_INCOMPATIBLE_TRIGGERS - if attr in s and attr in batch_size_attrs and getattr(cli, attr) != 1 + if attr in s + and attr in _RANDOM_POOL_BATCH_SIZE_FLAGS + and getattr(cli, attr) != 1 ] if set_batch_flags and cli.input_file is not None and Path(cli.input_file).is_dir(): raise ValueError( @@ -681,6 +780,22 @@ def _reject_file_dataset_incompatible(cli: CLIConfig) -> None: "flattens them into one anonymous pool per modality. Point --input-file " "at a single random_pool file, or drop the batch-size flags." ) + if batch_violations: + flags = ", ".join(batch_violations) + if declared: + # The config file owns the format; --custom-dataset-type and + # --input-file are the CLI-only remedies suggested below. + # An omitted `format:` reads back as None from the raw YAML dict + # even though FileDataset.format defaults to single_turn; report + # the effective format rather than a misleading "None". + from aiperf.plugin.enums import DatasetFormat + + effective = declared_format or DatasetFormat.SINGLE_TURN + raise ValueError( + f"{flags} requires a random_pool dataset, but the config file " + f"declares format: {effective}. Set format: random_pool " + "in the config file, or drop these flags." + ) if batch_violations and cli.input_file is not None: remedy = ( "Either add --custom-dataset-type random_pool to keep --input-file" @@ -689,7 +804,7 @@ def _reject_file_dataset_incompatible(cli: CLIConfig) -> None: "to random_pool to keep --input-file" ) raise ValueError( - f"{', '.join(batch_violations)} requires --custom-dataset-type random_pool " + f"{flags} requires --custom-dataset-type random_pool " f"when used with --input-file. {remedy}, or remove --input-file to use a " "synthetic dataset." ) @@ -701,15 +816,27 @@ def _reject_file_dataset_incompatible(cli: CLIConfig) -> None: ) -def _apply_random_pool_batch_sizes(d: dict[str, Any], cli: CLIConfig) -> None: +def _apply_random_pool_batch_sizes( + d: dict[str, Any], + cli: CLIConfig, + *, + declared_format: Any = None, +) -> None: """Route batch-size CLI flags onto FileDataset fields when format is random_pool. FileDataset has no prompts/images/audio/video sub-configs, so batch sizes live as flat fields and are threaded directly to RandomPoolDatasetLoader. + + ``declared_format`` carries the format when a YAML config declared it, in + which case ``--input-file`` and ``--custom-dataset-type`` are absent from + the CLI and cannot be used to recognize a random_pool dataset. """ from aiperf.plugin.enums import CustomDatasetType - if not cli.input_file or cli.custom_dataset_type != CustomDatasetType.RANDOM_POOL: + if declared_format is not None: + if declared_format != CustomDatasetType.RANDOM_POOL: + return + elif not cli.input_file or cli.custom_dataset_type != CustomDatasetType.RANDOM_POOL: return s = cli.model_fields_set if "prompt_batch_size" in s: @@ -738,7 +865,11 @@ def _apply_random_pool_batch_sizes(d: dict[str, Any], cli: CLIConfig) -> None: ) -def _reject_baseten_only_trace_flags(cli: CLIConfig) -> None: +def _reject_baseten_only_trace_flags( + cli: CLIConfig, + *, + declared_format: Any = None, +) -> None: """Reject baseten_trace-only replay knobs on incompatible datasets. These knobs are only consumed by the baseten_trace loader; on any other @@ -763,6 +894,15 @@ def _reject_baseten_only_trace_flags(cli: CLIConfig) -> None: if not set_flags: return msg = f"{', '.join(set_flags)} is only supported by the baseten_trace loader" + if declared_format is not None: + # A config file declared the loader, and --input-file / + # --custom-dataset-type are rejected on that path, so inferring from + # them would reject a combination the YAML explicitly supports. + if declared_format == CustomDatasetType.BASETEN_TRACE: + return + raise ValueError( + f"{msg}, but the config file declares format: {declared_format}." + ) if _implies_public_dataset(cli) or not cli.input_file: raise ValueError( f"{msg}; provide --input-file and --custom-dataset-type baseten_trace." @@ -839,7 +979,11 @@ def _reject_baseten_trace_unsupported_synthesis( ) -def _reject_baseten_trace_extra_input_collisions(cli: CLIConfig) -> None: +def _reject_baseten_trace_extra_input_collisions( + cli: CLIConfig, + *, + declared_format: Any = None, +) -> None: """Reject --extra-inputs keys the baseten_trace loader injects per-turn. Loader-injected per-turn values (``min_tokens`` from the recorded output @@ -851,7 +995,13 @@ def _reject_baseten_trace_extra_input_collisions(cli: CLIConfig) -> None: """ from aiperf.plugin.enums import CustomDatasetType - if cli.custom_dataset_type != CustomDatasetType.BASETEN_TRACE: + # The format can come from the CLI or from a config file; the loader + # injects the same per-turn values either way, so the collision is the + # same collision. + resolved_format = ( + declared_format if declared_format is not None else cli.custom_dataset_type + ) + if resolved_format != CustomDatasetType.BASETEN_TRACE: return extra = dict(cli.extra_inputs or ()) collisions: list[tuple[str, str]] = [] @@ -1174,7 +1324,142 @@ def _determine_needs_text(cli: CLIConfig) -> bool: # --- public entrypoint ---------------------------------------------------- -def build_dataset(cli: CLIConfig) -> dict[str, Any]: +def _reject_source_override_type_switch(cli: CLIConfig, declared_type: Any) -> None: + """Refuse --input-file / --custom-dataset-type unless the YAML says file. + + Both change a value inside a file dataset. Against a synthetic or public + config they would switch the dataset type, invalidating whole groups of + keys the config file declares -- so the user gets a message naming the + declared type instead of a validation error listing fields they never + touched. + """ + from aiperf.common.enums import DatasetType + + if declared_type is None or declared_type == DatasetType.FILE: + return + offenders = [ + flag + for attr, flag in ( + ("input_file", "--input-file"), + ("custom_dataset_type", "--custom-dataset-type"), + ) + if attr in cli.model_fields_set and getattr(cli, attr) is not None + ] + if not offenders: + return + raise ValueError( + f"{', '.join(offenders)} applies to a file dataset, but the config " + f"file declares type: {declared_type}. Change the type in the config " + f"file, or drop these flags -- switching the dataset type from the " + f"command line would invalidate the rest of the dataset block." + ) + + +def _run_dataset_rejections( + cli: CLIConfig, + *, + declared_type: Any = None, + declared_format: Any = None, +) -> None: + """Run the flag-compatibility guards for the resolved dataset identity. + + ``declared_type``/``declared_format`` are set when a YAML config file + declared them, in which case the guards evaluate against those rather + than against flags like ``--input-file`` that the ``--config`` path + rejects and therefore never sees. + """ + from aiperf.common.enums import DatasetType + + override_mode = declared_type is not None + + _reject_file_dataset_incompatible( + cli, declared_type=declared_type, declared_format=declared_format + ) + _reject_source_override_type_switch(cli, declared_type) + _reject_baseten_only_trace_flags( + cli, declared_format=declared_format if override_mode else None + ) + if override_mode: + source = ( + f"--custom-dataset-type {declared_format}" + if "custom_dataset_type" in cli.model_fields_set + else f"YAML format: {declared_format}" + ) + _reject_baseten_trace_unsupported_synthesis( + cli, + declared_format, + dataset_format_source=source, + ) + else: + _reject_baseten_trace_unsupported_synthesis(cli, cli.custom_dataset_type) + _reject_baseten_trace_extra_input_collisions( + cli, declared_format=declared_format if override_mode else None + ) + + if cli.dataset_filters and not ( + _implies_public_dataset(cli) or declared_type == DatasetType.PUBLIC + ): + if override_mode: + # Telling a --config user to pass --public-dataset is bad advice: + # the config file owns the type, and that flag is rejected here. + raise ValueError( + f"--dataset-filter requires a public dataset, but the config " + f"file declares type: {declared_type}." + ) + raise ValueError("--dataset-filter requires --public-dataset") + + +def _seed_declared_identity( + d: dict[str, Any], + cli: CLIConfig, + declared_type: Any, + declared_format: Any, +) -> None: + """Stand in for ``_apply_dataset_type`` when the YAML declared the type. + + Seeds type/format so the type-aware helpers downstream route correctly + (they are stripped again before the override is returned), carries the + ``entries`` count that ``_apply_dataset_type`` would otherwise have + emitted, and drops the synthetic-only subtables for file/public datasets. + """ + from aiperf.common.enums import DatasetType + + d["type"] = declared_type + if declared_format is not None: + d.setdefault("format", declared_format) + + # --num-conversations feeds `entries`. Take it only from the flags that + # name a conversation count: _resolve_entries also falls back to + # --request-count, which makes sense when building a dataset from nothing + # but as an override would reset a YAML entry count from an unrelated + # loadgen flag. + fields_set = cli.model_fields_set + if ( + "conversation_num" in fields_set + and "conversation_num_dataset_entries" not in fields_set + ): + conversations = cli.conversation_num + if isinstance(conversations, list): + conversations = max(conversations) if conversations else None + if conversations is not None: + d["entries"] = conversations + + if declared_type in (DatasetType.FILE, DatasetType.PUBLIC): + # Same strip _apply_dataset_type performs for these types on the + # CLI-only path: neither carries the synthetic per-modality subtables, + # and leaking one trips extra_forbidden. Helpers below + # (--prompt-corpus, random_pool batch sizes) re-attach what these + # types legitimately support. + for key in _SYNTHETIC_ONLY_SUBTABLES: + d.pop(key, None) + + +def build_dataset( + cli: CLIConfig, + *, + declared_type: Any = None, + declared_format: Any = None, +) -> dict[str, Any]: """Build a single dataset entry (without the wrapping ``name`` field). Discriminates among synthetic / file / public based on the populated @@ -1184,27 +1469,61 @@ def build_dataset(cli: CLIConfig) -> dict[str, Any]: when --input-file is set, except batch-size flags when ``--custom-dataset-type random_pool`` is the custom dataset type. + Args: + cli: the parsed CLIConfig for this invocation. + declared_type: dataset ``type`` when the caller already knows it -- + i.e. a YAML config file declared it. Supplying it switches this + function into *override mode*: the type is read rather than + inferred from which flags happen to be populated, defaults are + not materialized for values the user did not set, and the keys + the config file owns (``type``/``format``/``path``/``dataset``) + are stripped from the result. That makes the output safe to + deep-merge onto a YAML dataset block. + declared_format: dataset ``format`` alongside ``declared_type``, so + format-sensitive routing (random_pool batch sizes, baseten trace + guards) evaluates against the declared value. + Returns: - A dict suitable for ``DatasetConfig.model_validate({"name": "main", **out})``. + A dict suitable for ``DatasetConfig.model_validate({"name": "main", **out})``, + or -- in override mode -- for deep-merging onto an existing dataset dict. """ + + override_mode = declared_type is not None + # --custom-dataset-type legitimately overrides the YAML-declared format + # (see _seed_declared_identity / _reject_source_override_type_switch), so + # the format-sensitive guards below must judge the overridden value, not + # the stale pre-override declared_format -- otherwise a guard can reject + # the exact override it is checking for. + effective_format = ( + cli.custom_dataset_type + if "custom_dataset_type" in cli.model_fields_set + else declared_format + ) + needs_text = _determine_needs_text(cli) - _reject_file_dataset_incompatible(cli) - _reject_baseten_only_trace_flags(cli) - _reject_baseten_trace_unsupported_synthesis(cli, cli.custom_dataset_type) - _reject_baseten_trace_extra_input_collisions(cli) - if cli.dataset_filters and not _implies_public_dataset(cli): - raise ValueError("--dataset-filter requires --public-dataset") + _run_dataset_rejections( + cli, declared_type=declared_type, declared_format=effective_format + ) d = _flat_dataset_fields(cli) _attach_subtables(d, cli) - _apply_dataset_type(d, cli, needs_text) + if override_mode: + _seed_declared_identity(d, cli, declared_type, declared_format) + else: + _apply_dataset_type(d, cli, needs_text) _apply_sequence_distribution(d, cli) _apply_random_corpus_style_and_range_ratio(d, cli) - _apply_turns(d, cli) + _apply_turns(d, cli, only_set=override_mode) _apply_synthesis(d, cli) - _apply_implicit_media_batch(d, cli) + if not override_mode: + # Materializes batch_size=1 whenever a media-shape flag is set. As an + # override that would silently reset a YAML batch size the user never + # mentioned, so it is CLI-only-path behavior. + _apply_implicit_media_batch(d, cli) _apply_file_osl(d, cli) - _apply_random_pool_batch_sizes(d, cli) + _apply_random_pool_batch_sizes( + d, cli, declared_format=effective_format if override_mode else None + ) # block_size for FILE hash-id traces is owned by _apply_block_size (which # also rejects weka / non-hash-id formats). Do not also call # _apply_file_block_size — that helper is broader and redundant here. @@ -1215,6 +1534,22 @@ def build_dataset(cli: CLIConfig) -> dict[str, Any]: _apply_corpus_and_cache_bust(d, cli) if "random_seed" in cli.model_fields_set: d["random_seed"] = cli.random_seed + if override_mode: + # The config file owns the dataset TYPE and its public-dataset + # identity; --public-dataset / --hf-weka-dataset are rejected up + # front, so dropping these keys loses nothing. + for owned in ("type", "dataset"): + d.pop(owned, None) + # `path` and `format` are different: --input-file and + # --custom-dataset-type legitimately override them within a file + # dataset (see _reject_source_override_type_switch). Keep them only + # when the user actually asked, so an unset flag cannot clobber the + # config file's value with the declared one seeded earlier. + fields_set = cli.model_fields_set + if "input_file" not in fields_set: + d.pop("path", None) + if "custom_dataset_type" not in fields_set: + d.pop("format", None) return d diff --git a/src/aiperf/config/flags/_converter_profiling.py b/src/aiperf/config/flags/_converter_profiling.py index 29dd67d8d3..594579680c 100644 --- a/src/aiperf/config/flags/_converter_profiling.py +++ b/src/aiperf/config/flags/_converter_profiling.py @@ -391,6 +391,29 @@ def _seed_rate_dimension( prof["rate"] = rate_lo +def apply_cancellation(prof: dict[str, Any], cli: CLIConfig) -> None: + """Route --request-cancellation-rate/-delay onto a phase dict. + + Shared with the YAML+CLI resolver so both paths carry the same shape and + the same dependency guard. + """ + delay_set = "request_cancellation_delay" in cli.model_fields_set + if cli.request_cancellation_rate: + cancel: dict[str, Any] = {"rate": cli.request_cancellation_rate} + if delay_set: + cancel["delay"] = cli.request_cancellation_delay + prof["cancellation"] = cancel + elif delay_set: + # Mirror --arrival-smoothness gating: refuse to silently drop a + # user-supplied flag whose dependency wasn't met. + raise ValueError( + "--request-cancellation-delay requires --request-cancellation-rate " + "to be set (cancellation is disabled when rate is unset). " + "Pass --request-cancellation-rate > 0 to enable cancellation, or " + "drop --request-cancellation-delay." + ) + + def _apply_profiling_ramps(prof: dict[str, Any], cli: CLIConfig) -> None: fields_set = cli.model_fields_set for field, key in _RAMP_FIELDS: @@ -558,21 +581,7 @@ def _validate_profiling(prof: dict[str, Any], cli: CLIConfig) -> None: # Deliberate override of the PhaseConfig default (which would # leave it unbounded). prof.setdefault("requests", 10) - delay_set = "request_cancellation_delay" in cli.model_fields_set - if cli.request_cancellation_rate: - cancel: dict[str, Any] = {"rate": cli.request_cancellation_rate} - if delay_set: - cancel["delay"] = cli.request_cancellation_delay - prof["cancellation"] = cancel - elif delay_set: - # Mirror --arrival-smoothness gating: refuse to silently drop a - # user-supplied flag whose dependency wasn't met. - raise ValueError( - "--request-cancellation-delay requires --request-cancellation-rate " - "to be set (cancellation is disabled when rate is unset). " - "Pass --request-cancellation-rate > 0 to enable cancellation, or " - "drop --request-cancellation-delay." - ) + apply_cancellation(prof, cli) def _maybe_auto_promote_trace( diff --git a/src/aiperf/config/flags/_converter_runtime.py b/src/aiperf/config/flags/_converter_runtime.py index 2dbca45609..aa70cb3f2f 100644 --- a/src/aiperf/config/flags/_converter_runtime.py +++ b/src/aiperf/config/flags/_converter_runtime.py @@ -150,6 +150,8 @@ def _build_communication(cli: CLIConfig) -> dict[str, Any] | None: def build_logging_runtime( cli: CLIConfig, + *, + base_api_port: bool = False, ) -> tuple[dict[str, Any], dict[str, Any]]: """Build (logging, runtime) dicts for AIPerfConfig from a CLIConfig. @@ -162,8 +164,11 @@ def build_logging_runtime( Pydantic defaults on ``RuntimeConfig`` / ``LoggingConfig``. Verbose-driven log-level/UI promotion still writes (it's a derived effect, not a default). """ - # api_host requires api_port to be set explicitly (or via env). - if cli.api_host is not None and cli.api_port is None: + # api_host requires api_port to be set explicitly (or via env, or -- when + # a config file is in play -- by the file itself: base_api_port says the + # YAML already supplies runtime.api_port, so demanding the flag would + # reject a supported combination). + if cli.api_host is not None and cli.api_port is None and not base_api_port: raise ValueError( "api_host requires api_port (or AIPERF_API_SERVER_PORT) to be set" ) diff --git a/src/aiperf/config/flags/_converter_telemetry.py b/src/aiperf/config/flags/_converter_telemetry.py index 3a785325a4..7a4626f568 100644 --- a/src/aiperf/config/flags/_converter_telemetry.py +++ b/src/aiperf/config/flags/_converter_telemetry.py @@ -333,13 +333,23 @@ def _resolve_stream_domains(value: Any) -> tuple[bool, bool]: ) -def build_otel(cli: CLIConfig) -> dict[str, Any]: - """Translate OTel CLI flags into the first-class OTel config dict.""" +def build_otel(cli: CLIConfig, *, base_metrics_url: bool = False) -> dict[str, Any]: + """Translate OTel CLI flags into the first-class OTel config dict. + + ``base_metrics_url`` says a YAML config file already supplies + ``otel.metrics_url``. Without it, ``-f base.yaml --gen-ai-provider X`` + is rejected for missing a flag whose value the config file provides -- + the same shape as ``build_mlflow``'s ``base_tracking_uri``. + """ otel: dict[str, Any] = {} cli_set = cli.model_fields_set if "otel_url" in cli_set and cli.otel_url is not None: otel["metrics_url"] = _normalize_otel_metrics_url(cli.otel_url) + elif base_metrics_url: + # The config file owns metrics_url; emit only the overrides so the + # deep-merge leaves it in place. + pass else: # ``--stream`` and ``--gen-ai-provider`` are OTel-only secondary # flags: they only take effect when ``--otel-url`` is set. Refuse @@ -425,13 +435,18 @@ def _apply_mlflow_secondary_fields(out: dict[str, Any], cli: CLIConfig) -> None: out["parent_run_id"] = cli.mlflow_parent_run_id -def build_mlflow(cli: CLIConfig) -> dict[str, Any]: +def build_mlflow(cli: CLIConfig, *, base_tracking_uri: bool = False) -> dict[str, Any]: """Translate MLflow CLI flags into the first-class MLflow config dict. Ports v1 ``_validate_mlflow_config``: refuses secondary MLflow flags without ``--mlflow-tracking-uri``, rejects empty strings on tracking_uri/experiment/artifact_glob entries, and normalizes whitespace on tracking_uri/experiment/run_name/artifact_globs. + + ``base_tracking_uri`` says a YAML config file already supplies + ``mlflow.tracking_uri``. Without it, ``-f base.yaml --mlflow-experiment X`` + would be rejected for missing a flag whose value the config file already + provides -- the same shape as ``build_wandb``'s ``base_enabled``. """ # Normalize artifact-glob entries first so an "empty glob" error # surfaces before the missing-tracking-uri error. @@ -442,13 +457,21 @@ def build_mlflow(cli: CLIConfig) -> dict[str, Any]: secondary_present = any( key in cli.model_fields_set for key in _MLFLOW_SECONDARY_FIELDS ) - if secondary_present: + if not secondary_present: + return {} + if not base_tracking_uri: raise ValueError( "--mlflow-experiment, --mlflow-run-name, --mlflow-tag, " "--mlflow-artifact-glob, and --mlflow-parent-run-id require " "--mlflow-tracking-uri to be set." ) - return {} + # The config file owns tracking_uri; emit only the overrides so the + # deep-merge leaves it in place. + from_yaml_base: dict[str, Any] = {} + _apply_mlflow_secondary_fields(from_yaml_base, cli) + if artifact_globs is not None: + from_yaml_base["artifact_globs"] = artifact_globs + return from_yaml_base out: dict[str, Any] = {"tracking_uri": tracking_uri} _apply_mlflow_secondary_fields(out, cli) diff --git a/src/aiperf/config/flags/_converter_warmup.py b/src/aiperf/config/flags/_converter_warmup.py index ab582971b8..51798a1600 100644 --- a/src/aiperf/config/flags/_converter_warmup.py +++ b/src/aiperf/config/flags/_converter_warmup.py @@ -76,6 +76,30 @@ def _warmup_pattern_type(w: dict[str, Any], cli: CLIConfig, s: set[str]) -> None w["concurrency"] = warmup_concurrency if warmup_concurrency is not None else 1 +def _warmup_override_pattern(w: dict[str, Any], cli: CLIConfig, s: set[str]) -> None: + """Emit only the pattern fields the user set, for an existing warmup phase. + + ``_warmup_pattern_type`` derives ``type``/``rate``/``concurrency`` together, + falling back to the non-warmup ``--concurrency`` / ``--request-rate`` / + ``--arrival-pattern`` when the warmup-specific flag is absent. That is right + when building a phase from nothing and wrong as an override: it would + recompute the phase type the config file declared from flags aimed at the + profiling phase. + """ + if "warmup_concurrency" in s and not isinstance(cli.warmup_concurrency, list): + w["concurrency"] = cli.warmup_concurrency + if "warmup_request_rate" in s and cli.warmup_request_rate is not None: + w["rate"] = cli.warmup_request_rate + if "warmup_arrival_pattern" in s: + match cli.warmup_arrival_pattern: + case ArrivalPattern.GAMMA: + w["type"] = PhaseType.GAMMA + case ArrivalPattern.CONSTANT: + w["type"] = PhaseType.CONSTANT + case _: + w["type"] = PhaseType.POISSON + + def _warmup_ramps(w: dict[str, Any], cli: CLIConfig, s: set[str]) -> None: def _pick(warmup_field: str, fallback_field: str) -> Any: if warmup_field in s: @@ -106,7 +130,7 @@ def _pick(warmup_field: str, fallback_field: str) -> Any: w["rate_ramp"] = {"duration": rr} -def build_warmup(cli: CLIConfig) -> dict[str, Any] | None: +def build_warmup(cli: CLIConfig, *, base_warmup: bool = False) -> dict[str, Any] | None: """Build a warmup phase dict from CLIConfig, or return None. The warmup phase is only emitted when the caller explicitly set one of the @@ -124,7 +148,13 @@ def build_warmup(cli: CLIConfig) -> dict[str, Any] | None: # "concurrency": 10, "requests": 50} """ s = cli.model_fields_set - if not ({"warmup_request_count", "warmup_num_sessions", "warmup_duration"} & s): + # base_warmup: a YAML config file already declares a warmup phase, so the + # trigger the CLI-only path requires is already satisfied -- the flags + # have somewhere to land, and dropping them would be the silent-ignore + # behavior this exists to prevent. + if not base_warmup and not ( + {"warmup_request_count", "warmup_num_sessions", "warmup_duration"} & s + ): # No warmup trigger -> no warmup phase. Refuse to silently drop # secondary warmup-only flags the user supplied — except under a # --scenario, where the auto-synthesized agentic warmup consumes @@ -139,9 +169,15 @@ def build_warmup(cli: CLIConfig) -> dict[str, Any] | None: "the grace period, or drop --warmup-grace-period." ) return None - w: dict[str, Any] = {"exclude_from_results": True} + # Overriding an existing phase emits only what the user set: the config + # file owns exclude_from_results, and the phase type it declares must not + # be recomputed from flags the user did not pass. + w: dict[str, Any] = {} if base_warmup else {"exclude_from_results": True} _warmup_count_field(w, cli) - _warmup_pattern_type(w, cli, s) + if base_warmup: + _warmup_override_pattern(w, cli, s) + else: + _warmup_pattern_type(w, cli, s) _warmup_ramps(w, cli, s) _apply_agentic_replay_fields(w, cli) if "warmup_prefill_concurrency" in s: @@ -156,7 +192,7 @@ def build_warmup(cli: CLIConfig) -> dict[str, Any] | None: # has a non-None default and so cannot be reliably distinguished from # explicit user input downstream), warmup_grace_period defaults to None, # so a non-None value means the user explicitly asked for it. - if "duration" not in w: + if "duration" not in w and not base_warmup: if cli.scenario is not None: # Under a scenario the agentic warmup barrier consumes the # grace via _apply_agentic_replay_fields; the user-declared diff --git a/src/aiperf/config/flags/_section_fields.py b/src/aiperf/config/flags/_section_fields.py index 7b8b25f6d5..0a845ccc4b 100644 --- a/src/aiperf/config/flags/_section_fields.py +++ b/src/aiperf/config/flags/_section_fields.py @@ -90,6 +90,8 @@ "prompt_prefix_length", "prompt_prefix_shared_system_length", "prompt_prefix_user_context_length", + "prompt_random_corpus_style", + "prompt_random_range_ratio", "prompt_sequence_distribution", # ----- image modality ----- "image_width_mean", diff --git a/src/aiperf/config/flags/cli_config.py b/src/aiperf/config/flags/cli_config.py index 490018f2e5..bd4f1de832 100644 --- a/src/aiperf/config/flags/cli_config.py +++ b/src/aiperf/config/flags/cli_config.py @@ -788,8 +788,11 @@ def url(self) -> str: Field( default=None, description=( - "Path to a YAML configuration file. " - "CLI flags override values from the config file." + "Path to a YAML configuration file. CLI flags override " + "values from the config file. A flag that cannot be applied " + "on top of a config file is rejected by name rather than " + "ignored, so a run never silently differs from what was " + "asked for." ), ), CLIParameter( diff --git a/src/aiperf/config/flags/resolver.py b/src/aiperf/config/flags/resolver.py index decb5c3546..5d5eadc576 100644 --- a/src/aiperf/config/flags/resolver.py +++ b/src/aiperf/config/flags/resolver.py @@ -7,9 +7,16 @@ (``aiperf profile`` and ``aiperf service``). When both are supplied, the YAML supplies the base configuration and any explicitly-set CLI flags on ``cli_config`` are deep-merged on top before AIPerfConfig validation -- so -``aiperf profile --config foo.yaml --search-recipe X --ttft-sla-ms 200`` -works the way users intuit instead of throwing +``aiperf profile --config foo.yaml --streaming --search-recipe X`` works the +way users intuit instead of throwing ``CLIConfig.endpoint.modelNames: Field required``. + +Not every flag can be applied this way. Anything this path cannot route is +rejected up front by ``reject_unrouted_cli_flags`` with an error naming the +flag, rather than being silently discarded -- see ``_config_flag_routing`` +and ``docs/dev/global-invariants.md`` for the classification and the tests +that keep it honest. ``--ttft-sla-ms`` is one such flag today: it does not +take effect under ``--config`` even alongside a recipe, so it errors. """ from __future__ import annotations @@ -18,8 +25,6 @@ import logging from typing import TYPE_CHECKING, Any -from pydantic.alias_generators import to_camel - from aiperf.common.enums import DatasetType from aiperf.common.phase import infer_legacy_phase_kind from aiperf.config.flags._resolver_gpu_telemetry import ( @@ -38,7 +43,7 @@ OUTPUT_FIELDS, SWEEPING_FIELDS, ) -from aiperf.plugin.enums import ArrivalPattern, DatasetFormat, PhaseType +from aiperf.plugin.enums import ArrivalPattern, PhaseType if TYPE_CHECKING: from pathlib import Path @@ -83,8 +88,14 @@ def resolve_config( return convert_cli_to_aiperf(cli_config) from aiperf.config import AIPerfConfig + from aiperf.config.flags._config_flag_routing import reject_unrouted_cli_flags from aiperf.config.loader import load_config_dict + # Fail before any merging: a flag this path cannot route would otherwise + # be dropped without a word, handing the user a benchmark that silently + # ignored what they asked for. + reject_unrouted_cli_flags(cli_config) + yaml_dict = load_config_dict(config_file) _normalize_loaded_benchmark_shorthands(yaml_dict) # Build the recipe's view of BenchmarkConfig from YAML + the @@ -111,10 +122,9 @@ def resolve_config( yaml_dict = normalize_gpu_telemetry_base_for_override(yaml_dict, overrides) yaml_dict = normalize_server_metrics_base_for_override(yaml_dict, overrides) merged = deep_merge(yaml_dict, overrides) if overrides else yaml_dict - _apply_dataset_synthesis_overrides(merged, cli_config) - _apply_dataset_filter_overrides(merged, cli_config) - _apply_random_pool_batch_size_overrides(merged, cli_config) + _apply_dataset_overrides(merged, cli_config) _apply_phase_loadgen_overrides(merged, cli_config) + _apply_warmup_overrides(merged, cli_config) promote_benchmark_magic_lists( merged, cli_config, @@ -178,7 +188,12 @@ def build_cli_overrides( build_tokenizer, ) from aiperf.config.flags._converter_runtime import build_logging_runtime - from aiperf.config.flags._converter_telemetry import build_wandb + from aiperf.config.flags._converter_telemetry import ( + build_mlflow, + build_network_latency, + build_otel, + build_wandb, + ) out: dict[str, Any] = {} _apply_endpoint_overrides(out, cli) @@ -193,21 +208,78 @@ def build_cli_overrides( _apply_optional_section( out, "wandb", build_wandb(cli, base_enabled=wandb_base_enabled) ) + # These three builders already existed and are used by the CLI-only + # converter; this path simply never called them, so --mlflow-*, + # --otel-url, and the network-latency flags were dropped whenever a + # config file was supplied. They gate on model_fields_set, so an unset + # flag leaves the YAML block alone. + _apply_optional_section(out, "network_latency", build_network_latency(cli)) + otel_base_url = benchmark_config is not None and bool( + benchmark_config.otel.metrics_url + ) + _apply_optional_section( + out, "otel", build_otel(cli, base_metrics_url=otel_base_url) + ) + mlflow_base_uri = benchmark_config is not None and bool( + benchmark_config.mlflow.tracking_uri + ) + _apply_optional_section( + out, "mlflow", build_mlflow(cli, base_tracking_uri=mlflow_base_uri) + ) + _apply_scenario_overrides(out, cli) + + if cli.goodput: + # Recipe-emitted SLOs win on key collision, matching + # convert_cli_to_aiperf: a goodput-style recipe owns the SLO contract + # for its run, so a stray --goodput must not override its thresholds. + slos = dict(cli.goodput) + slos.update(out.get("slos") or {}) + out["slos"] = slos if "no_sweep_table" in cli.model_fields_set: out["no_sweep_table"] = cli.no_sweep_table + # random_seed is an envelope key distinct from the dataset's own seed + # field (written by build_dataset onto the dataset block, which only + # feeds SessionIDGenerator). AIPerfConfig.random_seed is what + # resolve_run_seed threads into rng.init(...) for every child service + # process, so every other rng.derive(...) consumer -- synthetic prompt + # content, media generation, per-conversation turn shaping -- needs it + # too, matching _assemble_optional on the CLI-only path. + if "random_seed" in cli.model_fields_set: + out["random_seed"] = cli.random_seed + # Service-runtime CLI flags (--ui, --log-level, --verbose, ZMQ knobs) # land on RuntimeConfig / LoggingConfig in AIPerfConfig. build_logging_runtime # already gates on cli.model_fields_set, so YAML defaults stay # intact when the user didn't pass these flags. - logging_dict, runtime_dict = build_logging_runtime(cli) + runtime_base_port = benchmark_config is not None and ( + benchmark_config.runtime.api_port is not None + ) + logging_dict, runtime_dict = build_logging_runtime( + cli, base_api_port=runtime_base_port + ) _apply_optional_section(out, "logging", logging_dict) _apply_optional_section(out, "runtime", runtime_dict) return out +def _apply_scenario_overrides(out: dict[str, Any], cli: CLIConfig) -> None: + """Mirror the converter's scenario-lock fields onto the override dict. + + ``scenario`` and ``unsafe_override`` are plain data on ``BenchmarkConfig`` + rather than envelope keys, so ``_wrap_under_envelope`` moves them under + ``benchmark:`` exactly as ``_apply_scenario_fields`` relies on for the + CLI-only path. + """ + set_fields = cli.model_fields_set + if "scenario" in set_fields: + out["scenario"] = cli.scenario + if "unsafe_override" in set_fields: + out["unsafe_override"] = cli.unsafe_override + + def _apply_optional_section( out: dict[str, Any], key: str, value: dict[str, Any] | None ) -> None: @@ -339,7 +411,11 @@ def _apply_endpoint_overrides(out: dict[str, Any], cli: CLIConfig) -> None: ``--model-names`` lives on the CLIConfig endpoint section but maps to the ``models.items`` block on AIPerfConfig; everything else stays on ``endpoint``. """ - from aiperf.config.flags._converter_endpoint import _ENDPOINT_FIELD_MAP + from aiperf.config.flags._converter_endpoint import ( + _ENDPOINT_FIELD_MAP, + _maybe_build_reset_kv_cache, + _maybe_build_server_profiler, + ) ep_set = cli.model_fields_set & ENDPOINT_FIELDS if not ep_set: @@ -350,6 +426,15 @@ def _apply_endpoint_overrides(out: dict[str, Any], cli: CLIConfig) -> None: for cli_field, aiperf_key in _ENDPOINT_FIELD_MAP.items(): if cli_field in ep_set: endpoint[aiperf_key] = getattr(cli, cli_field) + # The probe sub-blocks have builders the CLI-only path already uses; this + # path simply never called them. They emit only fields the user set, so a + # YAML block keeps whatever the flags do not mention. + for key, built in ( + ("reset_kv_cache", _maybe_build_reset_kv_cache(cli)), + ("server_profiler", _maybe_build_server_profiler(cli)), + ): + if built is not None: + endpoint[key] = built if endpoint: out["endpoint"] = endpoint if "model_names" in ep_set and cli.model_names: @@ -376,219 +461,213 @@ def _apply_input_overrides(out: dict[str, Any], cli: CLIConfig) -> None: out.pop("endpoint", None) -def _apply_dataset_filter_overrides(merged: dict[str, Any], cli: CLIConfig) -> None: - if "dataset_filters" not in cli.model_fields_set: - return - - from aiperf.config.flags._converter_dataset import _parse_dataset_filters +def _locate_yaml_dataset(merged: dict[str, Any]) -> dict[str, Any] | None: + """Return the YAML dataset dict that CLI overrides apply to. + Accepts both the ``benchmark.dataset`` shorthand and the canonical + ``benchmark.datasets`` list. The multi-dataset branch below anticipates a + cap that has not been lifted yet, not current behavior: ``AIPerfConfig + .datasets`` is ``max_length=1``, and ``resolve_config`` validates before + overrides run, so a YAML with more than one dataset entry fails + validation before this function is ever reached. Kept as future-proofing + -- matching the long-standing convention of the dataset-filter and + synthesis overlays this function replaces, from when the cap did not + exist -- for whenever the limit lifts. + """ benchmark = merged.get("benchmark") if not isinstance(benchmark, dict): - raise ValueError("--dataset-filter requires a public dataset") - dataset = benchmark.get("dataset") - if not isinstance(dataset, dict): - datasets = benchmark.get("datasets") - if not isinstance(datasets, list) or not datasets: - raise ValueError("--dataset-filter requires a public dataset") - if len(datasets) > 1: - logger.warning( - "--dataset-filter with multiple YAML datasets applies only to " - "the first dataset" - ) - dataset = datasets[0] - if not isinstance(dataset, dict): - raise ValueError("--dataset-filter requires a public dataset") - if dataset.get("type") != DatasetType.PUBLIC: - raise ValueError("--dataset-filter requires a public dataset") - filters = dataset.setdefault("filters", {}) - filters.update(_parse_dataset_filters(cli.dataset_filters)) - - -def _first_yaml_dataset( - benchmark: dict[str, Any], *, warn_context: str -) -> dict[str, Any] | None: - """Resolve the singular ``dataset`` or first entry of ``datasets`` from a - merged YAML ``benchmark`` mapping. Returns ``None`` if neither is present. - - ``warn_context`` names the flag/feature in the "multiple datasets" warning - (e.g. ``"Batch-size flags"``), consistent with the convention shared by - ``_apply_dataset_filter_overrides`` and ``_apply_dataset_synthesis_overrides``. - """ + return None dataset = benchmark.get("dataset") if isinstance(dataset, dict): return dataset - datasets = benchmark.get("datasets") if not isinstance(datasets, list) or not datasets: return None if len(datasets) > 1: logger.warning( - "%s with multiple YAML datasets apply only to the first dataset", - warn_context, + "Dataset CLI flags with multiple YAML datasets apply only to the " + "first dataset" ) - dataset = datasets[0] - return dataset if isinstance(dataset, dict) else None + first = datasets[0] + return first if isinstance(first, dict) else None -# Maps CLIConfig attribute name -> (FileDataset field name, CLI flag display name). -# Used by _apply_random_pool_batch_size_overrides for gating and error messages. -_RANDOM_POOL_BATCH_SIZE_OVERRIDE_MAP: tuple[tuple[str, str, str], ...] = ( - ("prompt_batch_size", "prompt_batch_size", "--prompt-batch-size"), - ("image_batch_size", "image_batch_size", "--image-batch-size"), - ("audio_batch_size", "audio_batch_size", "--audio-batch-size"), - ("video_batch_size", "video_batch_size", "--video-batch-size"), -) +def _snake_to_camel(name: str) -> str: + head, *rest = name.split("_") + return head + "".join(word[:1].upper() + word[1:] for word in rest) + + +def _drop_alias_spellings(base: dict[str, Any], override: dict[str, Any]) -> None: + """Remove the camelCase spelling of every key the override sets. + + YAML configs address fields by their camelCase alias (``maxOsl``) while + ``build_dataset`` emits the snake_case field name (``max_osl``). Merging + the two as-is would leave both spellings of the same field in the dict and + let validation pick a winner -- which is exactly the kind of quiet + ambiguity this work exists to remove. Deleting the alias first makes the + CLI value win outright, whichever spelling the config file used. + """ + for key, value in override.items(): + alias = _snake_to_camel(key) + if alias != key and alias in base: + if isinstance(value, dict) and isinstance(base[alias], dict): + # Same field, different spelling: keep the YAML's siblings by + # folding its sub-dict onto the canonical key before merging. + base[key] = deep_merge(base.pop(alias), base.get(key) or {}) + else: + base.pop(alias, None) + if isinstance(value, dict) and isinstance(base.get(key), dict): + _drop_alias_spellings(base[key], value) + + +def _inert_dataset_flags( + cli: CLIConfig, + declared_type: Any, + declared_format: Any, + candidates: set[str], +) -> list[str]: + """Return the flags in ``candidates`` that contribute nothing on their own. + + Attribution is by re-running the real builder with exactly one flag set, + rather than by inspecting the combined result: a flag that emits nothing + alone emits nothing in company either, and asking ``build_dataset`` + directly means the answer cannot drift from what it actually does. + + A flag whose solo build raises is treated as contributing -- it is loud, + which is the property that matters, and the combined build already + succeeded. + """ + # CLIConfig is a TYPE_CHECKING-only import at module scope; it has to be + # imported for real here, and the narrow `except` below is what would + # have surfaced that rather than silently treating every flag as routed. + from aiperf.config.flags import CLIConfig as _CLIConfig + from aiperf.config.flags._converter_dataset import build_dataset + from aiperf.config.loader.errors import ConfigurationError + + inert: list[str] = [] + for field in candidates: + # Solo construction is deliberately outside the try: a CLIConfig + # cross-field validator failing on this single field alone would be + # a real bug in this function's premise (the combined CLIConfig + # already validated), and must surface rather than be swallowed by + # the except below, which exists for build_dataset only. + solo = _CLIConfig(**{field: getattr(cli, field)}) + try: + emitted = build_dataset( + solo, declared_type=declared_type, declared_format=declared_format + ) + except (ValueError, ConfigurationError): + # Loud on its own, which is the property that matters; the + # combined build already succeeded. Deliberately narrow: a broad + # `except` here would turn a bug in this function into a silently + # disabled guard. + continue + if not emitted: + inert.append(field) + return inert -def _apply_random_pool_batch_size_overrides( - merged: dict[str, Any], cli: CLIConfig +def _reject_inert_dataset_flags( + cli: CLIConfig, dataset: dict[str, Any], declared_type: Any ) -> None: - """Overlay explicit batch-size CLI flags onto a YAML-supplied random_pool dataset. - - In the YAML+CLI path ``_apply_input_overrides`` only routes ``headers`` and - ``extra_inputs``; every other ``INPUT_FIELDS`` member (including the four - batch-size fields added by this PR) was silently discarded. This function - closes that gap for the four fields that ``RandomPoolDatasetLoader`` consumes. - - Gating is on ``cli.model_fields_set`` — not truthiness, not ``is not None`` - against the field value — so an unset flag never clobbers a YAML-supplied value. - Zero is a valid value (``image/audio/video_batch_size=0`` disables that modality). - - Only applies to ``type: file`` datasets. Synthetic and public datasets have no - ``format`` field at all, so this function must not touch or reject them here. - Note this does NOT mean the flags take effect there: nothing in the YAML+CLI - path currently routes batch-size flags onto ``SyntheticDataset.prompts/ - images/audio/video.batch_size`` (``_apply_input_overrides`` only handles - ``headers``/``extra_inputs``), so a batch-size flag against a synthetic YAML - dataset has no effect -- a pre-existing gap this function does not close and - is out of scope to fix here. It logs a warning rather than dropping the flag - silently, since the neighbouring wrong-format case raises loudly. The CLI-only - path (no ``--config``) applies these flags correctly; only the YAML+CLI overlay - drops them. - - For a ``type: file`` dataset that isn't ``format: random_pool``, a ``ValueError`` - is raised with a message that names the flag and the format, matching the - friendly error the CLI-only path produces instead of letting the ``FileDataset`` - model validator fire a raw Pydantic trace. - - With multiple YAML datasets the override applies to the first dataset only, - consistent with the convention in ``_apply_dataset_synthesis_overrides`` and - ``_apply_dataset_filter_overrides``. + """Raise for dataset flags that resolve cleanly while doing nothing. + + Checking only whether the whole override came back empty let any inert + flag ride along with a routable one: the result was non-empty, the guard + never fired, and the inert flag was dropped exactly as before this work. + Every multi-flag command line touching the dataset escaped the guarantee, + so the reconciliation is per flag. + + ``declared_type`` is the caller's already-defaulted value (``dataset.get( + "type") or DatasetType.SYNTHETIC``), passed in rather than re-derived so + the error message reports the same type the check actually ran against + -- re-deriving it here previously meant the message rendered the raw, + possibly-``None`` YAML value instead. """ - set_fields = cli.model_fields_set & { - cli_attr for cli_attr, _, _ in _RANDOM_POOL_BATCH_SIZE_OVERRIDE_MAP - } - if not set_fields: - return + from aiperf.config.flags._config_flag_routing import ( + DATASET_FIELDS_OUTSIDE_INPUT, + DATASET_OVERRIDE_FIELDS, + flag_names_for, + ) + from aiperf.config.loader.errors import ConfigurationError - benchmark = merged.get("benchmark") - if not isinstance(benchmark, dict): + candidates = cli.model_fields_set & ( + DATASET_OVERRIDE_FIELDS | DATASET_FIELDS_OUTSIDE_INPUT + ) + if not candidates: return - dataset = _first_yaml_dataset(benchmark, warn_context="Batch-size flags") - if dataset is None: - return - if dataset.get("type") != DatasetType.FILE: - # Adjacent to a loud ValueError for a file dataset of the wrong format, so - # do not drop this one in silence: the flag genuinely has no effect here. - logger.warning( - "%s ignored: batch-size flags are only applied to a YAML dataset with " - "type: file and format: random_pool (got type: %s). The CLI-only path " - "(no --config) applies them normally.", - ", ".join( - flag - for cli_attr, _, flag in _RANDOM_POOL_BATCH_SIZE_OVERRIDE_MAP - if cli_attr in set_fields - ), - dataset.get("type"), - ) + inert = _inert_dataset_flags( + cli, + declared_type, + dataset.get("format"), + candidates, + ) + if not inert: return - # dataset.get("format") reads the raw pre-validation YAML dict, so an omitted - # `format:` key reads back as None here even though FileDataset.format defaults - # to DatasetFormat.SINGLE_TURN -- fall back to that default so the error message - # below reports the actual effective format instead of a misleading "None". - fmt = dataset.get("format") or DatasetFormat.SINGLE_TURN - if fmt != DatasetFormat.RANDOM_POOL: - flag_names = ", ".join( - flag - for cli_attr, _, flag in _RANDOM_POOL_BATCH_SIZE_OVERRIDE_MAP - if cli_attr in set_fields - ) - raise ValueError( - f"{flag_names} requires format: random_pool on the YAML dataset " - f"(got format: {fmt}). Either set format: random_pool in the dataset " - "config, or remove these flags." - ) + names = sorted("/".join(flag_names_for(f) or (f,)) for f in inert) + raise ConfigurationError( + f"These CLI flags have no effect on a dataset of type " + f"{str(declared_type)!r}: {', '.join(names)}. Remove them, or use a " + f"dataset type that supports them." + ) - for cli_attr, dataset_field, _ in _RANDOM_POOL_BATCH_SIZE_OVERRIDE_MAP: - if cli_attr in set_fields: - # FileDataset uses alias_generator=to_camel with extra="forbid": if the - # YAML already supplied this field under its camelCase alias (e.g. - # promptBatchSize, the shipped template idiom), writing the snake_case - # key here leaves both present and Pydantic rejects the snake_case one - # as extra. Drop whichever spelling is already there before writing. - dataset.pop(to_camel(dataset_field), None) - dataset.pop(dataset_field, None) - dataset[dataset_field] = getattr(cli, cli_attr) - - -def _apply_dataset_synthesis_overrides(merged: dict[str, Any], cli: CLIConfig) -> None: - """Overlay explicit synthesis flags onto a YAML-supplied dataset.""" - if not any( - field == "allow_dataset_wrap" or field.startswith("synthesis_") - for field in cli.model_fields_set - ): - return - from aiperf.config.dataset.trace import SynthesisConfig +def _apply_dataset_overrides(merged: dict[str, Any], cli: CLIConfig) -> None: + """Overlay explicitly-set dataset flags onto the YAML-supplied dataset. + + Delegates to ``build_dataset`` -- the same builder the CLI-only path uses + -- in override mode, so the two paths share one implementation of how a + flag maps onto the dataset shape. Previously this file carried a + hand-written routing function per field class (synthesis, filters, + random_pool batch sizes), each of which had to be kept in step with the + converter by hand; anything nobody wrote a function for was silently + dropped. + + ``build_dataset`` emits only keys backed by ``cli.model_fields_set``, so + an unset flag cannot clobber a YAML value, and the config file keeps + ownership of dataset type, format, and source. + """ + from aiperf.config.flags._config_flag_routing import DATASET_OVERRIDE_FIELDS from aiperf.config.flags._converter_dataset import ( - _apply_synthesis, - _reject_baseten_trace_unsupported_synthesis, + apply_implicit_media_batch_override, + build_dataset, ) - benchmark = merged.get("benchmark") - datasets = benchmark.get("datasets") if isinstance(benchmark, dict) else None - if not isinstance(datasets, list) or not datasets: - raise ValueError("synthesis flags require a file or public dataset") - if len(datasets) > 1: - logger.warning( - "Synthesis flags with multiple YAML datasets apply only to the first dataset" - ) - dataset = datasets[0] - if not isinstance(dataset, dict): - raise ValueError("synthesis flags require a file or public dataset") - if dataset.get("type") not in (DatasetType.FILE, DatasetType.PUBLIC): - logger.warning( - "Synthesis flags require a file or public dataset; ignoring them " - "for dataset type %r", - dataset.get("type"), - ) + dataset = _locate_yaml_dataset(merged) + if dataset is None: + if cli.model_fields_set & DATASET_OVERRIDE_FIELDS: + from aiperf.config.loader.errors import ConfigurationError + + raise ConfigurationError( + "Dataset CLI flags require a dataset in the config file, but " + "none was found under benchmark.dataset / benchmark.datasets." + ) return - if dataset.get("type") == DatasetType.FILE: - _reject_baseten_trace_unsupported_synthesis( - cli, - dataset.get("format"), - dataset_format_source="YAML format: baseten_trace", - ) + # A YAML dataset may omit `type`; validation resolves that to synthetic. + # Passing None here would instead switch build_dataset back to inferring + # the type from flags -- the CLI-only path, which materializes defaults + # meant for building a dataset from nothing. Default it so both spellings + # of "synthetic" take the same route. + declared_type = dataset.get("type") or DatasetType.SYNTHETIC + override = build_dataset( + cli, + declared_type=declared_type, + declared_format=dataset.get("format"), + ) + _reject_inert_dataset_flags(cli, dataset, declared_type) + if not override: + return - override = {"type": dataset.get("type")} - _apply_synthesis(override, cli) - if synthesis := override.get("synthesis"): - base = SynthesisConfig.model_validate( - dataset.get("synthesis") or {} - ).model_dump(by_alias=True, exclude_unset=True) - update = SynthesisConfig.model_validate(synthesis).model_dump( - by_alias=True, exclude_unset=True - ) - dataset["synthesis"] = deep_merge(base, update) + apply_implicit_media_batch_override(override, dataset) + _drop_alias_spellings(dataset, override) + merged_dataset = deep_merge(dataset, override) + dataset.clear() + dataset.update(merged_dataset) -# CLI loadgen flag -> phase field. Each entry is (loadgen_attr, phase_key). -# The CLI help promises "CLI flags override values from the config file"; -# this table makes that real for YAML-supplied phase shapes by overlaying -# the explicit CLI value onto the resolved profiling phase. _LOADGEN_PHASE_FIELD_MAP: tuple[tuple[str, str], ...] = ( ("request_count", "requests"), ("benchmark_duration", "duration"), @@ -598,6 +677,22 @@ def _apply_dataset_synthesis_overrides(merged: dict[str, Any], cli: CLIConfig) - ("request_rate", "rate"), ("user_centric_rate", "rate"), ("num_users", "users"), + # --num-conversations sets the profiling phase's session count as well as + # the dataset entry count, matching convert_cli_to_aiperf. It lives in + # INPUT_FIELDS rather than LOADGEN_FIELDS, hence the explicit mention in + # the gate below. + ("conversation_num", "sessions"), +) + +# Fields routed onto the profiling phase that are not LOADGEN_FIELDS members. +_NON_LOADGEN_PHASE_FIELDS: frozenset[str] = frozenset( + { + "conversation_num", + "fixed_schedule", + "fixed_schedule_auto_offset", + "fixed_schedule_start_offset", + "fixed_schedule_end_offset", + } ) @@ -626,7 +721,7 @@ def _apply_phase_loadgen_overrides(merged: dict[str, Any], cli: CLIConfig) -> No _apply_agentic_replay_fields, ) - loadgen_set = cli.model_fields_set & LOADGEN_FIELDS + loadgen_set = cli.model_fields_set & (LOADGEN_FIELDS | _NON_LOADGEN_PHASE_FIELDS) agentic_set = cli.model_fields_set.intersection(_AGENTIC_REPLAY_ROUTES) if not loadgen_set and not agentic_set: return @@ -673,7 +768,164 @@ def _apply_phase_loadgen_overrides(merged: dict[str, Any], cli: CLIConfig) -> No ): target["grace_period"] = cli.benchmark_grace_period + if cli.fixed_schedule: + # _profiling_phase_type does this when building a phase from flags. + # Set it before _apply_phase_shaping_overrides so the fixed-schedule + # offset routes, which validate against the phase type, can apply. + target["type"] = PhaseType.FIXED_SCHEDULE + _apply_agentic_replay_fields(target, cli) + _apply_phase_shaping_overrides(target, cli) + + +def _apply_phase_shaping_overrides(target: dict[str, Any], cli: CLIConfig) -> None: + """Route ramps, cancellation and arrival smoothness onto the phase. + + These were built only by the CLI-only converter, so under ``--config`` + they were dropped and later rejected. Reuse the converter's own helpers + rather than restating the field maps here: ``_RAMP_FIELDS`` and the + gamma-only routes stay in one place, and a new entry in either is picked + up by both paths. + + ``_apply_phase_specific_routes`` validates against ``prof["type"]``, which + on this path is whatever the YAML declared -- so ``--arrival-smoothness`` + against a non-gamma YAML phase is rejected with the converter's message + instead of being silently dropped. + """ + from aiperf.config.flags._converter_profiling import ( + _apply_phase_specific_routes, + _apply_profiling_ramps, + apply_cancellation, + ) + + _apply_profiling_ramps(target, cli) + apply_cancellation(target, cli) + if "type" in target: + _apply_phase_specific_routes(target, cli) + # An explicit start offset contradicts auto_offset, whose default is + # True; build_profiling clears it the same way at + # _converter_profiling.py:531. Without this the user gets a raw + # "auto_offset cannot be True when start_offset is set" from + # validation for a combination the CLI-only path accepts. + if target["type"] == PhaseType.FIXED_SCHEDULE and "start_offset" in target: + target.setdefault("auto_offset", False) + + +# CLI flags that shape the warmup phase. Derived from the section frozenset +# rather than restated so a new warmup_* flag is covered automatically. +_WARMUP_FIELDS: frozenset[str] = frozenset( + field for field in LOADGEN_FIELDS if field.startswith("warmup_") +) + + +def _find_warmup_phase(phases: list[Any]) -> dict[str, Any] | None: + """Return the YAML-declared warmup phase, if there is one.""" + for entry in phases: + if not isinstance(entry, dict): + continue + kind = infer_legacy_phase_kind(entry.get("name"), entry.get("kind")) + if kind == "warmup": + return entry + return None + + +def _apply_warmup_overrides(merged: dict[str, Any], cli: CLIConfig) -> None: + """Overlay explicitly-set ``--warmup-*`` flags onto the warmup phase. + + ``_apply_phase_loadgen_overrides`` deliberately targets only the profiling + phase, so that ``--request-count`` cannot clobber a warmup ramp. That left + the warmup flags with nowhere to go: they were dropped before the + classification gate and rejected after it. + + Three cases, matching what the CLI-only path does where it can: + + - the config file declares a warmup phase -> merge the set flags onto it, + leaving everything the user did not mention alone; + - it does not, but a trigger flag (--warmup-request-count / + --warmup-num-sessions / --warmup-duration) is set -> build the phase, as + ``convert_cli_to_aiperf`` does; + - neither -> raise, because a secondary flag such as + ``--warmup-concurrency`` has nothing to attach to and would otherwise be + silently ignored. + """ + from aiperf.config.flags._converter_warmup import build_warmup + from aiperf.config.loader.errors import ConfigurationError + + warmup_set = cli.model_fields_set & _WARMUP_FIELDS + if not warmup_set: + return + + benchmark = merged.get("benchmark") + if not isinstance(benchmark, dict): + return + phases = benchmark.get("phases") + if not isinstance(phases, list) or not phases: + return + + existing = _find_warmup_phase(phases) + built = build_warmup(cli, base_warmup=existing is not None) + if built is None: + from aiperf.config.flags._config_flag_routing import flag_names_for + + names = sorted("/".join(flag_names_for(f) or (f,)) for f in warmup_set) + raise ConfigurationError( + f"{', '.join(names)} needs a warmup phase to apply to. Declare one " + f"in the config file, or pass a warmup trigger " + f"(--warmup-request-count / --warmup-num-sessions / " + f"--warmup-duration)." + ) + + if existing is None: + phases.insert(0, {"name": "warmup", "kind": "warmup", **built}) + return + + _drop_alias_spellings(existing, built) + merged_warmup = deep_merge(existing, built) + _reject_incompatible_warmup_transition(merged_warmup, warmup_set) + existing.clear() + existing.update(merged_warmup) + + +def _reject_incompatible_warmup_transition( + merged_warmup: dict[str, Any], warmup_set: frozenset[str] | set[str] +) -> None: + """Raise before writing back a warmup override that would merge into an + invalid phase. + + ``_warmup_override_pattern`` emits ``rate``/``concurrency``/``type`` + independently, so a flag that only touches one side of a type transition + can produce a structurally invalid phase: ``--warmup-request-rate`` onto + an existing concurrency phase adds ``rate``, which ``ConcurrencyPhase`` + forbids as an extra field; ``--warmup-arrival-pattern`` onto one switches + ``type`` to a rate phase without a ``rate``, which that phase requires. + Passing both together is a complete, valid transition and is not + rejected here. + """ + from aiperf.config.loader.errors import ConfigurationError + + final_type = merged_warmup.get("type") + if ( + "warmup_request_rate" in warmup_set + and final_type == PhaseType.CONCURRENCY + and merged_warmup.get("rate") is not None + ): + raise ConfigurationError( + "--warmup-request-rate has no effect: the warmup phase is " + "type: concurrency, which has no rate field. Pass " + "--warmup-arrival-pattern too to switch it to a rate-controlled " + "phase, or drop --warmup-request-rate." + ) + if ( + "warmup_arrival_pattern" in warmup_set + and final_type != PhaseType.CONCURRENCY + and merged_warmup.get("rate") is None + and merged_warmup.get("rate_series") is None + ): + raise ConfigurationError( + f"--warmup-arrival-pattern switches the warmup phase to " + f"type: {final_type}, which requires a rate. Pass " + "--warmup-request-rate too, or drop --warmup-arrival-pattern." + ) def _reject_loadgen_target_collisions(fields_set: set[str]) -> None: diff --git a/src/aiperf/config/loader/parsing.py b/src/aiperf/config/loader/parsing.py index 3f154fc70f..20a851c12d 100644 --- a/src/aiperf/config/loader/parsing.py +++ b/src/aiperf/config/loader/parsing.py @@ -292,32 +292,38 @@ def parse_str_or_list_of_positive_values(input: Any) -> list[Any]: return output -def parse_file(value: str | None) -> Path | None: +def parse_file(value: str | Path | None) -> Path | None: """Parse an existing file/directory path from a CLI value. Args: - value: Path string from CLI/config input. ``None`` or an empty string - disables the path and returns ``None``. + value: Path string from CLI/config input, or a ``Path`` this + function already returned -- accepted as a pass-through so the + validator is idempotent when a caller round-trips an + already-parsed field value back through it (e.g. reconstructing + a model from another instance's field values). ``None`` or an + empty string disables the path and returns ``None``. Returns: A ``Path`` pointing to an existing file or directory, or ``None`` when no value was provided. Raises: - ValueError: If ``value`` is not a string, or if the path does not exist as - a file or directory. + ValueError: If ``value`` is not a string or Path, or if the path does not + exist as a file or directory. """ if not value: return None + elif isinstance(value, Path): + path = value elif not isinstance(value, str): raise ValueError(f"Expected a string, but got {type(value).__name__}") else: path = Path(value) - if path.is_file() or path.is_dir(): - return path - else: - raise ValueError(f"'{value}' is not a valid file or directory") + if path.is_file() or path.is_dir(): + return path + else: + raise ValueError(f"'{value}' is not a valid file or directory") def validate_sequence_distribution(v: str | None) -> str | None: diff --git a/tests/unit/cli_runner/test_random_pool_batch_size_yaml_override.py b/tests/unit/cli_runner/test_random_pool_batch_size_yaml_override.py index 108bf77693..f3a3a0efcc 100644 --- a/tests/unit/cli_runner/test_random_pool_batch_size_yaml_override.py +++ b/tests/unit/cli_runner/test_random_pool_batch_size_yaml_override.py @@ -238,7 +238,7 @@ def test_batch_size_on_yaml_with_omitted_format_reports_effective_default( yaml_path = tmp_path / "no_format.yaml" yaml_path.write_text(yaml_content) cli = _cli(prompt_batch_size=4) - with pytest.raises(ValueError, match=re.escape("got format: single_turn")): + with pytest.raises(ValueError, match=re.escape("declares format: single_turn")): resolve_config(cli, yaml_path) @@ -298,39 +298,36 @@ def test_batch_size_flag_on_synthetic_yaml_does_not_raise( assert cfg.benchmark.datasets[0].type == "synthetic" -def test_prompt_batch_size_flag_on_synthetic_yaml_is_dropped_with_warning( - tmp_path: Path, caplog: pytest.LogCaptureFixture +def test_prompt_batch_size_flag_overrides_synthetic_yaml( + tmp_path: Path, ) -> None: - """Pins the known (pre-existing, out-of-scope-to-fix-here) gap: nothing in - the YAML+CLI path routes --prompt-batch-size onto a synthetic dataset's - prompts.batch_size, unlike the CLI-only path (no --config) where the same - flag applies correctly. The YAML value survives untouched and the explicit - CLI flag has no effect, inverting normal CLI-overrides-YAML precedence -- - so it must at least warn, since the adjacent wrong-format file-dataset case - raises loudly for the same flag. If the value assertion starts failing, the - gap has been closed -- update this test to assert the override took effect. + """The gap this used to pin is closed. + + Nothing in the YAML+CLI path routed --prompt-batch-size onto a synthetic + dataset's prompts.batch_size, so the flag was silently ignored and the + YAML value survived -- inverting normal CLI-overrides-YAML precedence. + build_dataset now serves both paths, so the flag applies here exactly as + it does without --config. """ yaml_path = _write_synthetic_yaml(tmp_path, batch_size=1) cli = _cli(prompt_batch_size=4) - with caplog.at_level("WARNING", logger="aiperf.config.flags.resolver"): - cfg = resolve_config(cli, yaml_path) - assert cfg.benchmark.datasets[0].prompts.batch_size == 1 - assert "--prompt-batch-size ignored" in caplog.text + cfg = resolve_config(cli, yaml_path) + assert cfg.benchmark.datasets[0].prompts.batch_size == 4 -def test_image_batch_size_flag_on_synthetic_yaml_is_dropped_with_warning( - tmp_path: Path, caplog: pytest.LogCaptureFixture +def test_image_batch_size_flag_creates_the_images_block( + tmp_path: Path, ) -> None: - """Companion to the text case: --image-batch-size against a synthetic YAML - dataset with no `images:` block doesn't even create one -- the flag has - zero effect end to end, not just a value mismatch, and warns accordingly. + """Companion to the text case. + + --image-batch-size against a synthetic YAML with no `images:` block used + not to even create one: zero effect end to end, not just a value + mismatch. It now applies. """ yaml_path = _write_synthetic_yaml(tmp_path, batch_size=1) cli = _cli(image_batch_size=2) - with caplog.at_level("WARNING", logger="aiperf.config.flags.resolver"): - cfg = resolve_config(cli, yaml_path) - assert cfg.benchmark.datasets[0].images is None - assert "--image-batch-size ignored" in caplog.text + cfg = resolve_config(cli, yaml_path) + assert cfg.benchmark.datasets[0].images.batch_size == 2 def test_batch_size_flag_applies_without_cli_custom_dataset_type( @@ -351,3 +348,72 @@ def test_batch_size_flag_applies_without_cli_custom_dataset_type( assert "custom_dataset_type" not in cli.model_fields_set cfg = resolve_config(cli, yaml_path) assert _dataset(cfg).prompt_batch_size == 4 + + +def _write_single_turn_yaml(tmp_path: Path, pool_path: Path) -> Path: + """Write a minimal YAML config with a non-random_pool file dataset.""" + yaml_content = f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: single_turn + path: {pool_path} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + cfg_path = tmp_path / "single_turn.yaml" + cfg_path.write_text(yaml_content) + return cfg_path + + +def test_batch_size_flag_applies_when_custom_dataset_type_overrides_yaml_format( + tmp_path: Path, +) -> None: + """--custom-dataset-type random_pool overrides a non-random_pool YAML + format; the batch-size guard must judge the overridden format, not the + stale pre-override YAML value. + + Regression: declared_format was read straight off the raw YAML dict + before build_dataset applies the --custom-dataset-type override, so this + raised "requires a random_pool dataset... declares format: single_turn" + even though --custom-dataset-type random_pool is exactly the format the + guard is checking for -- the guard and the resolver disagreed about what + the format was. + """ + pool = tmp_path / "pool.jsonl" + pool.touch() + yaml_path = _write_single_turn_yaml(tmp_path, pool) + cli = _cli(custom_dataset_type="random_pool", prompt_batch_size=4) + cfg = resolve_config(cli, yaml_path) + dataset = _dataset(cfg) + assert dataset.format == "random_pool" + assert dataset.prompt_batch_size == 4 + + +def test_image_shape_flag_on_synthetic_yaml_with_no_images_block_still_generates_images( + tmp_path: Path, +) -> None: + """--image-width-mean against a synthetic YAML with no `images:` block must + include images, not silently resolve to batch_size=0 ("disabled"). + + Regression: _apply_implicit_media_batch (which materializes batch_size=1 + whenever a media-shape flag is set without an explicit batch size) was + suppressed wholesale in override mode to avoid stomping a YAML batch size + the user never mentioned. But when the YAML has no images: block at all, + there is nothing to stomp, and the model default batch_size=0 means + "disabled" -- a multimodal benchmark request silently degrades to + text-only with no error, since endpoints/base_endpoint.py skips falsy + content strings. + """ + yaml_path = _write_synthetic_yaml(tmp_path) + cli = _cli(image_width_mean=64) + cfg = resolve_config(cli, yaml_path) + images = cfg.benchmark.datasets[0].images + assert images is not None + assert images.batch_size == 1 diff --git a/tests/unit/cli_runner/test_random_seed_envelope_override.py b/tests/unit/cli_runner/test_random_seed_envelope_override.py new file mode 100644 index 0000000000..b4cb47bf6c --- /dev/null +++ b/tests/unit/cli_runner/test_random_seed_envelope_override.py @@ -0,0 +1,75 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Regression: ``--random-seed`` under ``--config`` must reach the envelope seed. + +``build_dataset`` writes ``cli.random_seed`` onto the dataset block, but +``AIPerfConfig.random_seed`` -- the envelope-level seed that +``resolve_run_seed`` feeds into ``rng.init(...)`` for every child service +process -- is a distinct field that ``build_cli_overrides`` never wrote in +the YAML+CLI path. The dataset seed only drives ``SessionIDGenerator``; +every other ``rng.derive(...)`` consumer (synthetic prompt content, +image/audio/video generation, per-conversation turn count and delay) reads +off the envelope seed instead, so it stayed non-reproducible despite +``--random-seed`` appearing to take effect. +""" + +from __future__ import annotations + +from pathlib import Path + +from aiperf.config.flags import CLIConfig +from aiperf.config.flags.resolver import resolve_config + + +def _cli(**kwargs: object) -> CLIConfig: + """Build a CLIConfig with only the supplied fields in model_fields_set.""" + return CLIConfig(**CLIConfig(**kwargs).model_dump(exclude_unset=True)) # type: ignore[arg-type] + + +def _write_minimal_yaml(tmp_path: Path) -> Path: + yaml_content = """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + cfg_path = tmp_path / "base.yaml" + cfg_path.write_text(yaml_content) + return cfg_path + + +def test_random_seed_flag_sets_the_envelope_seed(tmp_path: Path) -> None: + """--random-seed under --config must reach AIPerfConfig.random_seed, + not just the dataset's own seed field.""" + yaml_path = _write_minimal_yaml(tmp_path) + cli = _cli(random_seed=42) + cfg = resolve_config(cli, yaml_path) + assert cfg.random_seed == 42 + + +def test_random_seed_flag_still_reaches_the_dataset_seed(tmp_path: Path) -> None: + """The existing dataset-seed routing must not regress while fixing the + envelope gap -- SessionIDGenerator takes the dataset seed with + precedence, so both fields must carry the flag's value.""" + yaml_path = _write_minimal_yaml(tmp_path) + cli = _cli(random_seed=42) + cfg = resolve_config(cli, yaml_path) + assert cfg.benchmark.datasets[0].random_seed == 42 + + +def test_random_seed_flag_absent_leaves_envelope_seed_unset(tmp_path: Path) -> None: + """An unset --random-seed must not clobber the YAML with a materialized + default; the envelope seed stays whatever the YAML (or its own default) + supplies.""" + yaml_path = _write_minimal_yaml(tmp_path) + cli = _cli() + assert "random_seed" not in cli.model_fields_set + cfg = resolve_config(cli, yaml_path) + assert cfg.random_seed is None diff --git a/tests/unit/common/config/test_config_validators.py b/tests/unit/common/config/test_config_validators.py index 9e03a09897..2370ac7286 100644 --- a/tests/unit/common/config/test_config_validators.py +++ b/tests/unit/common/config/test_config_validators.py @@ -1,6 +1,7 @@ # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 +from pathlib import Path from typing import Any import pytest @@ -9,12 +10,40 @@ coerce_value, normalize_http_url, normalize_http_urls, + parse_file, parse_str_as_numeric_dict, parse_str_or_dict_as_tuple_list, parse_str_or_list_of_positive_values, ) +class TestParseFile: + """Test suite for the parse_file function.""" + + def test_parse_file_path_output_returns_same_path(self, tmp_path: Path) -> None: + """Must be idempotent: re-applying to its own output must not raise. + + Regression: CLIConfig.input_file's BeforeValidator only accepted + ``str`` input, so re-constructing a CLIConfig from an + already-parsed ``cli.input_file`` (a Path, per the return type) + raised "Expected a string, but got PosixPath" -- surfaced by + _inert_dataset_flags's solo-reconstruction pattern + (`_CLIConfig(**{field: getattr(cli, field)})`), which round-trips + an already-validated field's value back through its own validator. + """ + target = tmp_path / "pool.jsonl" + target.touch() + parsed = parse_file(str(target)) + assert parse_file(parsed) == parsed + + def test_parse_file_none_input_returns_none(self) -> None: + assert parse_file(None) is None + + def test_parse_file_missing_path_raises_error(self) -> None: + with pytest.raises(ValueError, match="not a valid file or directory"): + parse_file("/no/such/path/here") + + class TestCoerceValue: """Test suite for the coerce_value function.""" diff --git a/tests/unit/config/test_config_flag_routing.py b/tests/unit/config/test_config_flag_routing.py new file mode 100644 index 0000000000..eb9068c05c --- /dev/null +++ b/tests/unit/config/test_config_flag_routing.py @@ -0,0 +1,317 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""CLI flags combined with ``--config`` must never be silently ignored. + +``resolve_config`` historically routed only a small subset of ``CLIConfig`` +fields into the YAML-supplied base config; every other explicitly-set flag was +silently discarded. A user passing ``-f base.yaml --random-seed 42`` got a run +with a different seed than they asked for, with no diagnostic. + +These tests pin the guarantee that replaces that behavior: a flag that cannot +be routed under ``--config`` raises a ``ConfigurationError`` naming the flag, +rather than being dropped. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from aiperf.config.flags import CLIConfig +from aiperf.config.flags._section_fields import ( + ACCURACY_FIELDS, + ENDPOINT_FIELDS, + INPUT_FIELDS, + LOADGEN_FIELDS, + OUTPUT_FIELDS, + SWEEPING_FIELDS, + TOKENIZER_FIELDS, +) +from aiperf.config.flags.resolver import resolve_config +from aiperf.config.loader.errors import ConfigurationError + +ALL_SECTIONS = ( + ACCURACY_FIELDS + | ENDPOINT_FIELDS + | INPUT_FIELDS + | LOADGEN_FIELDS + | OUTPUT_FIELDS + | SWEEPING_FIELDS + | TOKENIZER_FIELDS +) + + +@pytest.fixture +def base_yaml(tmp_path: Path) -> Path: + """A minimal, valid YAML config with a synthetic dataset.""" + cfg = tmp_path / "base.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +def cli(**kwargs: object) -> CLIConfig: + """Build a CLIConfig whose model_fields_set is exactly ``kwargs``.""" + return CLIConfig(**CLIConfig(**kwargs).model_dump(exclude_unset=True)) # type: ignore[arg-type] + + +# --------------------------------------------------------------------------- +# Unrouted flags must be loud +# --------------------------------------------------------------------------- + + +def test_unrouted_input_flag_raises_naming_the_flag(base_yaml: Path) -> None: + """--public-dataset would switch the dataset TYPE, so it errors. + + (--input-file, which this case used to cover, now overrides the path + within a file dataset -- switching the type is the part that stays + rejected.) + """ + with pytest.raises(ConfigurationError, match=r"--public-dataset"): + resolve_config(cli(public_dataset="sharegpt"), base_yaml) + + +def test_unrouted_sweeping_flag_raises(base_yaml: Path) -> None: + """Sweep bounds resolve cleanly and change nothing, so they must error. + + (The warmup flags this used to check are routed now.) + """ + with pytest.raises(ConfigurationError, match=r"--concurrency-min"): + resolve_config(cli(concurrency_min=2), base_yaml) + + +def test_dataset_type_switch_flag_raises(base_yaml: Path) -> None: + """--hf-weka-dataset implies a public dataset, switching the type.""" + with pytest.raises(ConfigurationError, match=r"--hf-weka-dataset"): + resolve_config(cli(hf_weka_dataset="some/dataset"), base_yaml) + + +def test_error_names_every_offending_flag(base_yaml: Path) -> None: + """All unrouted flags are reported at once, not one per run.""" + with pytest.raises(ConfigurationError) as excinfo: + resolve_config(cli(concurrency_min=2, public_dataset="sharegpt"), base_yaml) + message = str(excinfo.value) + assert "--concurrency-min" in message + assert "--public-dataset" in message + + +def test_error_mentions_config_flag_as_the_cause(base_yaml: Path) -> None: + """The message must tell users WHY the flag was rejected.""" + with pytest.raises(ConfigurationError, match=r"--config"): + resolve_config(cli(concurrency_min=2), base_yaml) + + +# --------------------------------------------------------------------------- +# Routed flags keep working +# --------------------------------------------------------------------------- + + +def test_routed_endpoint_flag_still_applies(base_yaml: Path) -> None: + """--streaming is routed; it must override YAML without raising.""" + resolved = resolve_config(cli(streaming=True), base_yaml) + assert resolved.benchmark.endpoint.streaming is True + + +def test_routed_output_flag_still_applies(base_yaml: Path, tmp_path: Path) -> None: + """--artifact-dir is routed through build_artifacts; must not raise.""" + target = tmp_path / "artifacts" + resolved = resolve_config(cli(artifact_directory=target), base_yaml) + assert Path(resolved.benchmark.artifacts.dir) == target + + +def test_no_flags_with_config_is_fine(base_yaml: Path) -> None: + """A bare --config invocation must not trip the guard.""" + resolved = resolve_config(cli(), base_yaml) + assert resolved.benchmark.models.items[0].name == "test-model" + + +# --------------------------------------------------------------------------- +# Magic lists: routed as sweep parameters, but only when list-shaped +# --------------------------------------------------------------------------- + + +def test_magic_list_isl_is_routed_as_sweep_parameter(base_yaml: Path) -> None: + """A list-shaped --isl becomes a sweep parameter, so it must not raise.""" + resolved = resolve_config(cli(prompt_input_tokens_mean=[128, 256]), base_yaml) + assert resolved.sweep is not None + assert resolved.sweep.parameters["datasets.default.prompts.isl.mean"] == [128, 256] + + +def test_scalar_isl_routes_onto_the_dataset(base_yaml: Path) -> None: + """The scalar form reaches the dataset block rather than a sweep. + + The magic-list form becomes a sweep parameter; the scalar form is routed + by _apply_dataset_overrides. Both must take effect -- neither may be + silently dropped. + """ + resolved = resolve_config(cli(prompt_input_tokens_mean=128), base_yaml) + assert resolved.benchmark.datasets[0].prompts.isl.mean == 128 + + +# --------------------------------------------------------------------------- +# The CLI-only path is untouched +# --------------------------------------------------------------------------- + + +def test_unrouted_flag_without_config_is_allowed() -> None: + """Without --config the converter handles every flag; no guard applies.""" + resolved = resolve_config( + cli( + random_seed=1234, + model_names=["m"], + urls=["http://localhost:8000"], + ), + None, + ) + assert resolved.benchmark.datasets[0].random_seed == 1234 + + +# --------------------------------------------------------------------------- +# Totality: no field may be silently unclassified +# --------------------------------------------------------------------------- + + +def test_every_cli_config_field_is_classified() -> None: + """EVERY CLIConfig field must be classified, not just sectioned ones. + + The section frozensets are opt-in: adding a field to ``CLIConfig`` + requires nobody to touch ``_section_fields.py``, and + ``test_section_fields_partition_cli_config`` only checks that section + entries exist on CLIConfig -- never the reverse. So an unsectioned field + was invisible to the guard and silently dropped under ``--config``, which + is how 17 fields (all mlflow_*, --otel-url, network_latency_*, --scenario) + came to be dropped without anyone noticing. + + Keying this on CLIConfig.model_fields instead makes classification + mandatory: a newly-added flag belongs to no set and fails here, at + authoring time. + """ + from aiperf.config.flags._config_flag_routing import ( + EXEMPT_FROM_CONFIG_ROUTING, + ROUTED_UNDER_CONFIG, + UNROUTED_UNDER_CONFIG, + ) + + unclassified = ( + frozenset(CLIConfig.model_fields) + - ROUTED_UNDER_CONFIG + - UNROUTED_UNDER_CONFIG + - EXEMPT_FROM_CONFIG_ROUTING + ) + assert not unclassified, ( + f"CLI fields are unclassified for the --config path: " + f"{sorted(unclassified)}. Every flag must be routed, listed in " + f"UNROUTED_UNDER_CONFIG so users get a loud error, or exempted in " + f"EXEMPT_FROM_CONFIG_ROUTING with a reason." + ) + + +def test_exempt_fields_are_not_also_classified() -> None: + """An exemption must not overlap the routed or unrouted sets.""" + from aiperf.config.flags._config_flag_routing import ( + EXEMPT_FROM_CONFIG_ROUTING, + ROUTED_UNDER_CONFIG, + UNROUTED_UNDER_CONFIG, + ) + + overlap = EXEMPT_FROM_CONFIG_ROUTING & (ROUTED_UNDER_CONFIG | UNROUTED_UNDER_CONFIG) + assert not overlap, f"exempt fields also classified: {sorted(overlap)}" + + +def test_every_section_field_is_classified() -> None: + """Each section field is either routed under --config or known-unrouted. + + This is the regression gate. A newly-added CLI flag that nobody wired into + the resolver lands in neither set and fails here, at authoring time, + instead of being silently dropped for users. + """ + from aiperf.config.flags._config_flag_routing import ( + ROUTED_UNDER_CONFIG, + UNROUTED_UNDER_CONFIG, + ) + + unclassified = ALL_SECTIONS - ROUTED_UNDER_CONFIG - UNROUTED_UNDER_CONFIG + assert not unclassified, ( + f"CLI fields are neither routed nor listed as unrouted under --config: " + f"{sorted(unclassified)}. Route them in resolver.py, or add them to " + f"UNROUTED_UNDER_CONFIG so users get a loud error instead of silence." + ) + + +def test_routed_and_unrouted_are_disjoint() -> None: + """A field cannot be both routed and rejected.""" + from aiperf.config.flags._config_flag_routing import ( + ROUTED_UNDER_CONFIG, + UNROUTED_UNDER_CONFIG, + ) + + assert not (ROUTED_UNDER_CONFIG & UNROUTED_UNDER_CONFIG) + + +def test_every_unrouted_field_has_a_real_flag_name() -> None: + """Error messages must name actual CLI flags, not guessed kebab-case.""" + from aiperf.config.flags._config_flag_routing import ( + UNROUTED_UNDER_CONFIG, + flag_names_for, + ) + + missing = sorted(f for f in UNROUTED_UNDER_CONFIG if not flag_names_for(f)) + assert not missing, f"No CLIParameter flag name found for: {missing}" + + +# --------------------------------------------------------------------------- +# Error messages must name a flag the user can find in their shell history +# --------------------------------------------------------------------------- + + +def test_dataset_source_in_type_fields_names_input_file_and_custom_dataset_type() -> ( + None +): + """``DATASET_SOURCE_IN_TYPE_FIELDS`` must actually name the two flags its + own docstring describes ("the trace file and the loader that reads it"), + not sit empty. + + Regression: `14e94535f` moved ``input_file``/``custom_dataset_type`` out + of ``DATASET_SOURCE_FIELDS`` and introduced this set as their documented + new home, but never populated it -- routing was (and is) still correct, + since both land in ``ROUTED_UNDER_CONFIG`` via the ``INPUT_FIELDS - + DATASET_SOURCE_FIELDS`` subtraction regardless of this set's contents, + which made the emptiness easy to miss. An empty set under a docstring + describing exactly this behavior misleads anyone auditing routing for + these flags into concluding the opposite of what actually happens. + """ + from aiperf.config.flags._config_flag_routing import ( + DATASET_SOURCE_IN_TYPE_FIELDS, + ROUTED_UNDER_CONFIG, + ) + + assert {"input_file", "custom_dataset_type"} == DATASET_SOURCE_IN_TYPE_FIELDS + assert DATASET_SOURCE_IN_TYPE_FIELDS <= ROUTED_UNDER_CONFIG + + +def test_error_lists_every_spelling_of_a_multi_alias_flag(base_yaml: Path) -> None: + """Naming only the first declared spelling sends users looking for a flag + they never typed. + + ``--sweep-variant`` was reported as ``--variant``. Since the resolver + cannot see which spelling was typed, it names them all. + """ + with pytest.raises(ConfigurationError) as excinfo: + resolve_config(cli(sweep_variants=["concurrency=2"]), base_yaml) + message = str(excinfo.value) + assert "--variant" in message + assert "--sweep-variant" in message diff --git a/tests/unit/config/test_config_input_overrides.py b/tests/unit/config/test_config_input_overrides.py new file mode 100644 index 0000000000..431e0d8ca7 --- /dev/null +++ b/tests/unit/config/test_config_input_overrides.py @@ -0,0 +1,694 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Dataset-shaping CLI flags must override a YAML-supplied dataset. + +Rather than hand-writing a routing function per field class, the YAML+CLI +path reuses ``build_dataset`` -- the same builder the CLI-only path uses -- +with the dataset type supplied by the YAML instead of inferred from flags. + +Two properties matter and are pinned here: + +1. an explicitly-set flag reaches the resolved dataset, and +2. a flag the user did NOT set never overwrites the YAML value, including + the defaults ``build_dataset`` materializes for the CLI-only path + (``images.batch_size``, ``turns.stddev``). +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from aiperf.config import AIPerfConfig +from aiperf.config.flags import CLIConfig +from aiperf.config.flags.resolver import resolve_config +from aiperf.config.loader.errors import ConfigurationError + + +def cli(**kwargs: object) -> CLIConfig: + """Build a CLIConfig whose model_fields_set is exactly ``kwargs``.""" + return CLIConfig(**CLIConfig(**kwargs).model_dump(exclude_unset=True)) # type: ignore[arg-type] + + +def dataset(cfg: AIPerfConfig): + return cfg.benchmark.datasets[0] + + +@pytest.fixture +def synthetic_yaml(tmp_path: Path) -> Path: + """Synthetic dataset with values a CLI flag could clobber.""" + cfg = tmp_path / "synthetic.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + random_seed: 11 + entries: 16 + prompts: + isl: + mean: 32 + stddev: 4 + osl: + mean: 8 + stddev: 1 + turns: + mean: 3 + stddev: 2 + images: + width: + mean: 64 + batch_size: 4 + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +@pytest.fixture +def random_pool_yaml(tmp_path: Path) -> Path: + pool = tmp_path / "pool.jsonl" + pool.write_text('{"text": "hi"}\n') + cfg = tmp_path / "random_pool.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: random_pool + path: {pool} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +# --------------------------------------------------------------------------- +# Flags reach the resolved dataset +# --------------------------------------------------------------------------- + + +def test_random_seed_overrides_yaml(synthetic_yaml: Path) -> None: + assert ( + dataset(resolve_config(cli(random_seed=99), synthetic_yaml)).random_seed == 99 + ) + + +def test_image_width_mean_overrides_yaml(synthetic_yaml: Path) -> None: + resolved = dataset(resolve_config(cli(image_width_mean=128), synthetic_yaml)) + assert resolved.images.width.mean == 128 + + +def test_scalar_isl_overrides_yaml(synthetic_yaml: Path) -> None: + """The scalar form must now route, not just the magic-list form.""" + resolved = dataset( + resolve_config(cli(prompt_input_tokens_mean=256), synthetic_yaml) + ) + assert resolved.prompts.isl.mean == 256 + + +def test_prefix_pool_size_overrides_yaml(synthetic_yaml: Path) -> None: + resolved = dataset(resolve_config(cli(prompt_prefix_pool_size=7), synthetic_yaml)) + assert resolved.prefix_prompts.pool_size == 7 + + +def test_conversation_turn_mean_overrides_yaml(synthetic_yaml: Path) -> None: + resolved = dataset(resolve_config(cli(conversation_turn_mean=5), synthetic_yaml)) + assert resolved.turns.mean == 5 + + +def test_audio_length_mean_overrides_yaml(synthetic_yaml: Path) -> None: + resolved = dataset(resolve_config(cli(audio_length_mean=2.5), synthetic_yaml)) + assert resolved.audio.length.mean == 2.5 + + +def test_video_duration_overrides_yaml(synthetic_yaml: Path) -> None: + resolved = dataset(resolve_config(cli(video_duration=9), synthetic_yaml)) + assert resolved.video.duration == 9 + + +def test_rankings_passages_mean_overrides_yaml(synthetic_yaml: Path) -> None: + resolved = dataset(resolve_config(cli(rankings_passages_mean=6), synthetic_yaml)) + assert resolved.rankings.passages.mean == 6 + + +# --------------------------------------------------------------------------- +# Unset flags must never clobber YAML +# --------------------------------------------------------------------------- + + +def test_unset_fields_leave_yaml_untouched(synthetic_yaml: Path) -> None: + resolved = dataset(resolve_config(cli(), synthetic_yaml)) + assert resolved.random_seed == 11 + assert resolved.prompts.isl.mean == 32 + assert resolved.turns.stddev == 2 + assert resolved.images.batch_size == 4 + + +def test_image_width_flag_does_not_reset_yaml_batch_size(synthetic_yaml: Path) -> None: + """build_dataset materializes images.batch_size=1 for the CLI-only path. + + Leaking that default into the override would silently reset a YAML batch + size the user never mentioned -- the exact class of bug this work exists + to remove. + """ + resolved = dataset(resolve_config(cli(image_width_mean=128), synthetic_yaml)) + assert resolved.images.width.mean == 128 + assert resolved.images.batch_size == 4 + + +def test_turn_mean_flag_does_not_reset_yaml_turn_stddev(synthetic_yaml: Path) -> None: + """Likewise for the turns.stddev=0 default.""" + resolved = dataset(resolve_config(cli(conversation_turn_mean=5), synthetic_yaml)) + assert resolved.turns.mean == 5 + assert resolved.turns.stddev == 2 + + +def test_isl_flag_does_not_reset_yaml_osl(synthetic_yaml: Path) -> None: + resolved = dataset( + resolve_config(cli(prompt_input_tokens_mean=256), synthetic_yaml) + ) + assert resolved.prompts.osl.mean == 8 + + +def test_isl_mean_flag_does_not_reset_yaml_isl_stddev(synthetic_yaml: Path) -> None: + """Likewise for prompts.isl.stddev -- --isl only sets prompts.isl.mean.""" + resolved = dataset( + resolve_config(cli(prompt_input_tokens_mean=256), synthetic_yaml) + ) + assert resolved.prompts.isl.mean == 256 + assert resolved.prompts.isl.stddev == 4 + + +def test_osl_mean_flag_does_not_reset_yaml_osl_stddev(synthetic_yaml: Path) -> None: + """Likewise for prompts.osl.stddev -- --osl only sets prompts.osl.mean.""" + resolved = dataset( + resolve_config(cli(prompt_output_tokens_mean=64), synthetic_yaml) + ) + assert resolved.prompts.osl.mean == 64 + assert resolved.prompts.osl.stddev == 1 + + +# --------------------------------------------------------------------------- +# The YAML owns the dataset type and source +# --------------------------------------------------------------------------- + + +def test_synthetic_only_flag_on_file_dataset_is_rejected( + random_pool_yaml: Path, +) -> None: + """A synthetic-shaping flag against a YAML file dataset must say so. + + The declared type comes from the YAML, so the converter's file-dataset + guard has to fire on it rather than on --input-file being absent. + """ + with pytest.raises((ValueError, ConfigurationError)) as excinfo: + resolve_config(cli(prompt_prefix_pool_size=4), random_pool_yaml) + assert "prefix" in str(excinfo.value).lower() + + +def test_random_pool_batch_size_still_routes(random_pool_yaml: Path) -> None: + """Regression guard for PR #1274 behavior after the refactor.""" + resolved = dataset(resolve_config(cli(prompt_batch_size=7), random_pool_yaml)) + assert resolved.prompt_batch_size == 7 + + +def test_zero_batch_size_overrides_yaml(tmp_path: Path) -> None: + """``--prompt-batch-size 0`` must win over a non-zero YAML value. + + Zero disables the text modality on random_pool (FileDataset allows ge=0 + as of the base branch). It is also falsy, so an override path gating on + truthiness rather than ``model_fields_set`` would drop it and leave the + YAML batch size in place -- text still enabled, contrary to the request. + """ + pool = tmp_path / "pool.jsonl" + pool.write_text('{"text": "hi"}\n') + cfg = tmp_path / "rp.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: random_pool + path: {pool} + prompt_batch_size: 4 + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + assert dataset(resolve_config(cli(prompt_batch_size=0), cfg)).prompt_batch_size == 0 + # ...and an unset flag still leaves the YAML value alone. + assert dataset(resolve_config(cli(), cfg)).prompt_batch_size == 4 + + +def test_public_dataset_flag_remains_rejected(synthetic_yaml: Path) -> None: + """--public-dataset would swap the dataset type the YAML declared.""" + with pytest.raises(ConfigurationError, match=r"--public-dataset"): + resolve_config(cli(public_dataset="sharegpt"), synthetic_yaml) + + +# --------------------------------------------------------------------------- +# --num-conversations: must match CLI-only semantics, not error +# --------------------------------------------------------------------------- + + +def test_conversation_num_sets_entries_and_sessions(synthetic_yaml: Path) -> None: + """``--num-conversations`` must do under --config what it does without it. + + On the CLI-only path the flag sets both ``dataset.entries`` and the + profiling phase's ``sessions``. Under --config it produced neither, and + the resolver rejected it with "no effect on a dataset of type + 'synthetic'" -- factually wrong, on one of the most-used flags in the + tool. + """ + resolved = resolve_config(cli(conversation_num=13), synthetic_yaml) + assert dataset(resolved).entries == 13 + profiling = [p for p in resolved.benchmark.phases if p.name == "profiling"] + assert profiling and profiling[0].sessions == 13 + + +def test_conversation_num_dataset_entries_sets_entries(synthetic_yaml: Path) -> None: + """The explicit entry-count flag wins for ``entries``.""" + resolved = resolve_config(cli(conversation_num_dataset_entries=21), synthetic_yaml) + assert dataset(resolved).entries == 21 + + +def test_request_count_does_not_become_dataset_entries(synthetic_yaml: Path) -> None: + """--request-count must not silently reset a YAML ``entries``. + + _resolve_entries falls back to request_count on the CLI-only path, where + it builds a dataset from nothing. As an override that fallback would + overwrite the config file's entry count from an unrelated loadgen flag. + """ + resolved = resolve_config(cli(request_count=50), synthetic_yaml) + assert dataset(resolved).entries == 16 # the YAML value + + +# --------------------------------------------------------------------------- +# Per-flag reconciliation: a flag is not excused by its neighbours +# --------------------------------------------------------------------------- + + +def test_inert_flag_paired_with_routable_one_still_errors( + synthetic_yaml: Path, +) -> None: + """An unroutable dataset flag must be caught even in company. + + The guard used to fire only when build_dataset produced nothing at all, + so pairing an inert flag with a working one left the result non-empty + and the inert flag was discarded exactly as before this work. Every + multi-flag command line touching the dataset escaped the guarantee. + """ + with pytest.raises(ConfigurationError, match=r"--synthesis-max-isl"): + resolve_config( + cli(prompt_input_tokens_mean=128, synthesis_max_isl=512), synthetic_yaml + ) + + +def test_inert_media_flag_paired_on_file_dataset_errors( + random_pool_yaml: Path, +) -> None: + """Same, for the media flags against a file dataset.""" + with pytest.raises(ConfigurationError, match=r"--image-width-mean"): + resolve_config(cli(prompt_batch_size=4, image_width_mean=128), random_pool_yaml) + + +def test_two_routable_flags_together_are_fine(synthetic_yaml: Path) -> None: + """The reconciliation must not reject flags that do take effect.""" + resolved = dataset( + resolve_config( + cli(prompt_input_tokens_mean=128, image_width_mean=64), synthetic_yaml + ) + ) + assert resolved.prompts.isl.mean == 128 + assert resolved.images.width.mean == 64 + + +def test_dataset_flag_outside_input_fields_is_reconciled( + synthetic_yaml: Path, +) -> None: + """--inter-turn-delay-cap-seconds is dataset-carried but not INPUT_FIELDS. + + Fields outside INPUT_FIELDS were never considered by the guard at all, + so this one no-oped silently on a synthetic dataset. + """ + with pytest.raises(ConfigurationError, match=r"--inter-turn-delay-cap-seconds"): + resolve_config(cli(inter_turn_delay_cap_seconds=3.0), synthetic_yaml) + + +# --------------------------------------------------------------------------- +# YAML-declared baseten_trace: the guards must read the declared identity +# --------------------------------------------------------------------------- + + +@pytest.fixture +def baseten_yaml(tmp_path: Path) -> Path: + """A file dataset that declares format: baseten_trace in the YAML.""" + trace = tmp_path / "baseten.jsonl" + trace.write_text('{"text": "hi"}\n') + cfg = tmp_path / "baseten.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: baseten_trace + path: {trace} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +@pytest.mark.parametrize( + ("field", "value"), + [ + ("replay_speedup", 2.0), + ("max_idle_gap_cap_seconds", 3.0), + ("trace_session_sample_ratio", 0.5), + ("open_loop_replay", True), + ("omit_kv_hints", True), + ], +) +def test_baseten_only_flags_accepted_on_yaml_declared_baseten_trace( + baseten_yaml: Path, field: str, value: object +) -> None: + """These are baseten_trace-only, and the YAML declares baseten_trace. + + The guard inferred the loader from --input-file / --custom-dataset-type, + which the --config path rejects outright because the config file owns + the dataset source and format. So a supported combination failed before + the run started. + """ + resolved = dataset(resolve_config(cli(**{field: value}), baseten_yaml)) + assert getattr(resolved, field) == value + + +def test_baseten_only_flag_still_rejected_on_other_trace_format( + random_pool_yaml: Path, +) -> None: + """The guard must still fire when the YAML declares a different loader.""" + with pytest.raises(ValueError, match="baseten_trace"): + resolve_config(cli(replay_speedup=2.0), random_pool_yaml) + + +def test_baseten_extra_input_collision_rejected_from_yaml_format( + baseten_yaml: Path, +) -> None: + """Loader-injected extras collide whether the format came from YAML or CLI. + + The collision guard keyed off cli.custom_dataset_type, so under --config + it never ran and the user's value was clobbered on the wire instead. + """ + with pytest.raises(ValueError, match="min_tokens"): + resolve_config(cli(extra_inputs=["min_tokens:5"]), baseten_yaml) + + +def test_synthetic_only_flag_message_is_actionable_under_config( + random_pool_yaml: Path, +) -> None: + """The advice must be something a --config user can act on. + + The CLI-only wording says to remove --input-file / --public-dataset. On + this path the user passed neither, and both are rejected outright, so + that advice is unfollowable. Point at the config file instead -- the same + correction already made for --dataset-filter one function over. + """ + with pytest.raises(ValueError) as excinfo: + resolve_config(cli(prompt_prefix_pool_size=4), random_pool_yaml) + message = str(excinfo.value) + assert "--input-file" not in message + assert "--public-dataset" not in message + assert "config file" in message + + +def test_batch_size_message_is_actionable_under_config(tmp_path: Path) -> None: + """Same for the batch-size variant of the guard.""" + trace = tmp_path / "t.jsonl" + trace.write_text('{"text": "hi"}\n') + cfg = tmp_path / "mooncake.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: mooncake_trace + path: {trace} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + with pytest.raises(ValueError) as excinfo: + resolve_config(cli(image_batch_size=2), cfg) + message = str(excinfo.value) + assert "--input-file" not in message + assert "config file" in message + + +def test_dataset_without_explicit_type_uses_override_mode(tmp_path: Path) -> None: + """A YAML dataset may omit ``type``; validation resolves it to synthetic. + + Passing that absent type through as None switched build_dataset back to + inferring from flags -- the CLI-only path, which materializes defaults + built for constructing a dataset from nothing. Both spellings of + "synthetic" must take the same route. + """ + cfg = tmp_path / "no_type.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + entries: 16 + turns: + mean: 3 + stddev: 2 + images: + width: + mean: 64 + batch_size: 4 + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + resolved = dataset(resolve_config(cli(image_width_mean=128), cfg)) + assert resolved.type == "synthetic" + assert resolved.images.width.mean == 128 + # Nothing the user did not set may be disturbed. + assert resolved.images.batch_size == 4 + assert resolved.turns.stddev == 2 + assert resolved.entries == 16 + + +def test_trace_idle_gap_cap_routes_on_a_trace_dataset(tmp_path: Path) -> None: + """--trace-idle-gap-cap-seconds is carried by build_dataset for traces. + + It was classified unrouted from a probe run only against a synthetic + dataset, where it correctly emits nothing -- so this PR hard-rejected a + flag that works. + """ + trace = tmp_path / "t.jsonl" + trace.write_text('{"text": "hi"}\n') + cfg = tmp_path / "trace.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: mooncake_trace + path: {trace} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + resolved = dataset(resolve_config(cli(trace_idle_gap_cap_seconds=3.0), cfg)) + assert resolved.trace_idle_gap_cap_seconds == 3.0 + + +def test_trace_idle_gap_cap_still_errors_on_synthetic(synthetic_yaml: Path) -> None: + """On a synthetic dataset it genuinely carries nothing, so it must be loud.""" + with pytest.raises(ConfigurationError, match=r"--trace-idle-gap-cap-seconds"): + resolve_config(cli(trace_idle_gap_cap_seconds=3.0), synthetic_yaml) + + +# --------------------------------------------------------------------------- +# Dataset source: override within the declared type, never switch it +# --------------------------------------------------------------------------- + + +def test_input_file_swaps_the_path_on_a_file_dataset( + random_pool_yaml: Path, tmp_path: Path +) -> None: + """Running the same benchmark against a different trace is a normal loop. + + --model already overrides models.items and --url overrides + endpoint.urls, so the config file does not own identity in general; + there is no reason the dataset path should be the exception. + """ + other = tmp_path / "other.jsonl" + other.write_text('{"text": "other"}\n') + resolved = dataset( + resolve_config(CLIConfig(input_file=str(other)), random_pool_yaml) + ) + assert Path(resolved.path) == other + + +def test_input_file_leaves_the_rest_of_the_dataset_alone( + tmp_path: Path, +) -> None: + """Swapping the path must not disturb the rest of the block.""" + pool = tmp_path / "pool.jsonl" + pool.write_text('{"text": "hi"}\n') + other = tmp_path / "other.jsonl" + other.write_text('{"text": "other"}\n') + cfg = tmp_path / "rp.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: random_pool + path: {pool} + prompt_batch_size: 4 + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + resolved = dataset(resolve_config(CLIConfig(input_file=str(other)), cfg)) + assert Path(resolved.path) == other + assert resolved.format == "random_pool" + assert resolved.prompt_batch_size == 4 + + +def test_custom_dataset_type_swaps_the_loader(tmp_path: Path) -> None: + """--custom-dataset-type changes the loader within type: file.""" + trace = tmp_path / "t.jsonl" + trace.write_text('{"text": "hi"}\n') + cfg = tmp_path / "mooncake.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: mooncake_trace + path: {trace} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + resolved = dataset(resolve_config(cli(custom_dataset_type="random_pool"), cfg)) + assert resolved.format == "random_pool" + + +def test_input_file_against_a_synthetic_config_errors( + synthetic_yaml: Path, tmp_path: Path +) -> None: + """Switching the dataset TYPE is a different thing, and stays rejected. + + The variants barely overlap -- FileDataset carries a dozen fields + PublicDataset rejects -- so a type switch silently invalidates whole + groups of keys the user wrote in the config file. + """ + other = tmp_path / "other.jsonl" + other.write_text('{"text": "other"}\n') + with pytest.raises(ValueError, match=r"synthetic"): + resolve_config(CLIConfig(input_file=str(other)), synthetic_yaml) + + +def test_public_dataset_still_switches_type_and_errors( + random_pool_yaml: Path, +) -> None: + with pytest.raises(ConfigurationError, match=r"--public-dataset"): + resolve_config(cli(public_dataset="sharegpt"), random_pool_yaml) + + +def test_inert_flag_message_reports_synthetic_for_an_omitted_yaml_type( + tmp_path: Path, +) -> None: + """An inert flag against a YAML dataset with no `type:` key must name the + type it actually resolves to (synthetic), not the raw None. + + Regression: the inert-flag check correctly defaults `dataset.get("type") + or DatasetType.SYNTHETIC` for the check itself, but the error message + interpolated the raw (unset) value, so an omitted `type:` -- legal, and + validated as synthetic -- printed "dataset of type None". Needs a + non-dataset flag (--streaming) alongside the inert one: without it, + ``build_cli_overrides`` returns nothing, ``pre_merged`` and ``merged`` + alias the same dict, and the preliminary ``AIPerfConfig.model_validate`` + call's own type-defaulting mutates the shared dict in place before this + check runs, masking the bug. + """ + cfg_path = tmp_path / "no_type.yaml" + cfg_path.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: {} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + with pytest.raises(ConfigurationError, match=r"dataset of type 'synthetic'"): + resolve_config(cli(synthesis_max_isl=100, streaming=True), cfg_path) diff --git a/tests/unit/config/test_config_override_invariants.py b/tests/unit/config/test_config_override_invariants.py new file mode 100644 index 0000000000..f0a9e24195 --- /dev/null +++ b/tests/unit/config/test_config_override_invariants.py @@ -0,0 +1,554 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Mechanical invariants over every dataset flag, not a hand-picked sample. + +Two properties are checked for all of ``INPUT_FIELDS`` at once, so a field +added later is covered without anyone remembering to write a test for it: + +**No spontaneous keys.** In override mode ``build_dataset`` must emit only +values the user actually supplied. The detector runs the builder twice with +two *different* values for the same field: any emitted key whose value is +identical across both runs cannot be derived from the input, so it is a +materialized default. Merged onto a YAML dataset, such a key silently +overwrites a value the user never mentioned -- how ``images.batch_size=1`` +and ``turns.stddev=0`` would have leaked before this was guarded. + +**No dead routing.** Every field the registry claims is routed under +``--config`` must actually change the resolved config. ``ROUTED_UNDER_CONFIG`` +computes the input section as a complement, so a newly-added field is +classified as routed automatically; this test is what stops that convenience +from quietly re-opening the bug for a field ``build_dataset`` does not carry. +""" + +from __future__ import annotations + +import enum +import types +import typing +from pathlib import Path +from typing import Any + +import pytest + +from aiperf.common.enums import DatasetType +from aiperf.config.flags import CLIConfig +from aiperf.config.flags._config_flag_routing import ( + DATASET_OVERRIDE_FIELDS, + MAGIC_LIST_ONLY_UNDER_CONFIG, + ROUTED_UNDER_CONFIG, +) +from aiperf.config.flags._converter_dataset import build_dataset +from aiperf.config.flags.resolver import resolve_config +from aiperf.config.loader.errors import ConfigurationError + +# Every field the registry claims to route, which is what the guarantee is +# about -- not just the dataset ones. +_ROUTED_FIELDS_UNDER_TEST = ROUTED_UNDER_CONFIG + + +def cli(**kwargs: object) -> CLIConfig: + return CLIConfig(**CLIConfig(**kwargs).model_dump(exclude_unset=True)) # type: ignore[arg-type] + + +def _with_companion(field: str, value: Any) -> dict[str, Any]: + """Flag kwargs for ``field``, plus the companion its schema requires. + + ``images.width`` and friends validate as a normal distribution, so a + stddev with no mean is rejected outright. Driving the pair keeps these + fields covered instead of erroring into a skip. + """ + kwargs: dict[str, Any] = {field: value} + if field.endswith("_stddev"): + mean_field = field[: -len("_stddev")] + "_mean" + if mean_field in CLIConfig.model_fields: + kwargs[mean_field] = 8 + return kwargs + + +def _unwrap(annotation: Any) -> Any: + while True: + origin = typing.get_origin(annotation) + if origin is typing.Annotated: + annotation = typing.get_args(annotation)[0] + continue + if origin in (typing.Union, types.UnionType): + args = [a for a in typing.get_args(annotation) if a is not type(None)] + if not args: + return None + # Magic-list flags are declared `list[int] | int | None`. Prefer + # the scalar member: it is the form this path routes, and taking + # args[0] would report the field as undrivable. + scalars = [a for a in args if typing.get_origin(a) is not list] + annotation = scalars[0] if scalars else args[0] + continue + return annotation + + +# Values for fields whose annotation cannot produce them: ``typing.Any`` +# (the magic-list flags), or types whose contents must parse. Without these +# the fields would report as undrivable and go untested. +FIELD_PROBE_VALUES: dict[str, list[Any]] = { + "request_count": [2, 3], + "concurrency": [2, 3], + "num_users": [2, 3], + "prefill_concurrency": [2, 3], + "request_rate": [2.0, 3.0], + "benchmark_duration": [2.0, 3.0], + "headers": [["x-probe-a:1"], ["x-probe-b:2"]], + "extra_inputs": [["probe_a:1"], ["probe_b:2"]], + "mlflow_tags": [["probe_a:1"], ["probe_b:2"]], + "server_metrics": [ + ["http://localhost:9090/metrics"], + ["http://localhost:9091/metrics"], + ], + "gpu_telemetry": [ + ["http://localhost:9400/metrics"], + ["http://localhost:9401/metrics"], + ], + "isl_osl_pairs": [["128,16"], ["256,32"]], + # --goodput takes space-separated "metric:value" pairs, not a list. + "goodput": ["ttft:200", "ttft:300"], + "server_metrics_formats": [["json"], ["csv"]], + "sweep_variants": [["concurrency=2"], ["concurrency=4"]], + # --search-recipe names a registered plugin, not an arbitrary string. + "search_recipe": ["prefill-ttft-curve", "concurrency-ramp"], +} + + +def _candidate_values(field: str) -> list[Any]: + """Valid values to drive ``field`` with, or [] if none can be generated. + + Returning [] (rather than guessing) keeps the test honest: a field it + cannot drive is reported as skipped instead of silently passing. + """ + if field in FIELD_PROBE_VALUES: + return FIELD_PROBE_VALUES[field] + info = CLIConfig.model_fields.get(field) + if info is None: + return [] + annotation = _unwrap(info.annotation) + if typing.get_origin(annotation) is typing.Literal: + return list(typing.get_args(annotation)) + if isinstance(annotation, type) and issubclass(annotation, enum.Enum): + return list(annotation) + if annotation is bool: + return [True, False] + if annotation is int: + return [2, 3] + if annotation is float: + return [2.0, 3.0] + if annotation is str: + return ["aiperf-probe-a", "aiperf-probe-b"] + if annotation is Path: + return [Path("/tmp/aiperf-probe-a"), Path("/tmp/aiperf-probe-b")] + # list[str] and friends: a one-element list is enough to vary the value. + # Fields whose contents must parse (--header k:v, --search-sla ...) reject + # these and raise, which the invariant accepts as loud. + if typing.get_origin(annotation) is list: + args = typing.get_args(annotation) + if args and args[0] is str: + return [["aiperf-probe-a"], ["aiperf-probe-b"]] + return [] + + +def _value_pair(field: str) -> tuple[Any, Any] | None: + """Two distinct valid values for ``field``, or None if unavailable.""" + values = _candidate_values(field) + return (values[0], values[1]) if len(values) >= 2 else None + + +def _leaves(value: Any, prefix: str = "") -> dict[str, Any]: + """Flatten a nested dict into ``{dotted.path: leaf}``.""" + if not isinstance(value, dict): + return {prefix: value} + out: dict[str, Any] = {} + for key, sub in value.items(): + out.update(_leaves(sub, f"{prefix}.{key}" if prefix else str(key))) + return out + + +# Fields the value generator cannot drive from their annotation (lists, +# free-form strings, parsed DSL strings). Listing them explicitly rather than +# skipping on the fly means a newly-added field of an unsupported type fails +# this suite instead of quietly reporting "skipped" -- a skip is invisible in +# CI, which is how a coverage hole hides. +UNDRIVABLE_FIELDS: frozenset[str] = frozenset( + { + # list-shaped + "audio_depths", + "audio_sample_rates", + "conversation_num", + "conversation_turn_mean", + "dataset_filters", + "prompt_input_tokens_mean", + "prompt_input_tokens_stddev", + "prompt_output_tokens_mean", + "prompt_output_tokens_stddev", + "video_audio_depth", + # needs a path that exists on disk, which a module-level constant + # cannot provide; covered directly by the dataset-source tests in + # test_config_input_overrides.py + "input_file", + # free-form / parsed strings with no safe auto-generated value + "hf_dataset_subset", + "prompt_sequence_distribution", + "video_codec", + } +) + + +def _require_drivable(field: str) -> None: + """Fail rather than skip when a field cannot be driven and is not listed.""" + assert field in UNDRIVABLE_FIELDS, ( + f"{field} cannot be driven from its annotation, so it is untested. " + f"Extend _candidate_values to cover its type, or add it to " + f"UNDRIVABLE_FIELDS with a reason." + ) + + +# Fields whose emitted shape legitimately carries a constant companion key. +# Each entry needs a reason; an unexplained entry is a bug being suppressed. +# Empty today -- no field currently needs an exemption; add one here (with a +# reason) if a future field's constant-leaf check is a legitimate structural +# wrapper key rather than a materialized default. +_CONSTANT_KEY_ALLOWLIST: dict[str, set[str]] = {} + + +@pytest.mark.parametrize("field", sorted(DATASET_OVERRIDE_FIELDS)) +def test_override_emits_no_key_the_user_did_not_set(field: str) -> None: + """An emitted key that ignores the input value is a materialized default.""" + pair = _value_pair(field) + if pair is None: + _require_drivable(field) + pytest.skip(f"{field} is listed in UNDRIVABLE_FIELDS") + first, second = pair + + try: + low = build_dataset(cli(**{field: first}), declared_type=DatasetType.SYNTHETIC) + high = build_dataset( + cli(**{field: second}), declared_type=DatasetType.SYNTHETIC + ) + except (ValueError, ConfigurationError): + pytest.skip(f"{field} is not valid against a synthetic dataset alone") + + low_leaves, high_leaves = _leaves(low), _leaves(high) + constant = { + path + for path in low_leaves.keys() & high_leaves.keys() + if low_leaves[path] == high_leaves[path] + } + constant -= _CONSTANT_KEY_ALLOWLIST.get(field, set()) + + assert not constant, ( + f"build_dataset emitted {sorted(constant)} for --{field.replace('_', '-')} " + f"with a value independent of the flag, so it is a default rather than " + f"something the user asked for. Merged onto a YAML dataset it would " + f"silently overwrite the config file's value. Suppress it in override " + f"mode, as _apply_implicit_media_batch is." + ) + + +@pytest.fixture +def trace_yaml(tmp_path: Path) -> Path: + """A file/trace dataset, for the flags that only apply to one.""" + pool = tmp_path / "trace.jsonl" + pool.write_text('{"text": "hi", "hash_ids": [1]}\n') + cfg = tmp_path / "trace.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: mooncake_trace + path: {pool} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +@pytest.fixture +def rich_yaml(tmp_path: Path) -> Path: + """A synthetic dataset carrying values an override could clobber.""" + cfg = tmp_path / "rich.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + random_seed: 11 + entries: 16 + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +@pytest.mark.parametrize("field", sorted(DATASET_OVERRIDE_FIELDS)) +def test_routed_dataset_field_actually_changes_the_resolved_config( + field: str, rich_yaml: Path, trace_yaml: Path +) -> None: + """A field claimed as routed must have an observable effect. + + Checked against both a synthetic and a file dataset: many flags apply to + only one (synthesis_* is file/public-only, prefix prompts are + synthetic-only), so requiring an effect on a single type would flag + correct routing as broken. + """ + if field in MAGIC_LIST_ONLY_UNDER_CONFIG: + pytest.skip(f"{field} routes as a sweep parameter in its list form") + candidates = _candidate_values(field) + if not candidates: + _require_drivable(field) + pytest.skip(f"{field} is listed in UNDRIVABLE_FIELDS") + + # Try every candidate: one may coincide with the model's own default + # (DatasetSamplingStrategy.SEQUENTIAL does), which would look like "no + # effect" without meaning the flag is dropped. + resolutions = [] + errors: list[str] = [] + changed = None + for config_yaml in (rich_yaml, trace_yaml): + baseline = resolve_config(cli(), config_yaml).model_dump(mode="json") + for value in candidates: + try: + resolved = resolve_config( + cli(**_with_companion(field, value)), config_yaml + ).model_dump(mode="json") + except (ValueError, ConfigurationError) as exc: + errors.append(str(exc)) + continue + resolutions.append(resolved) + if resolved != baseline: + changed = resolved + if not resolutions: + # "has no effect on a dataset of type X" is OUR error for a field + # nothing routes -- the bug, not a domain guard. Anything else (weka + # -only, baseten-only, needs a companion flag) is a real constraint + # and the flag is loud rather than dropped, so skipping is right. + assert not all("have no effect" in e for e in errors), ( + f"--{field.replace('_', '-')} is classified as routed but nothing " + f"routes it: resolving raises {errors[0]!r}. Route it, or list it " + f"in UNROUTED_UNDER_CONFIG so the error names the flag properly." + ) + pytest.skip(f"{field} is not valid against a synthetic dataset alone") + assert changed is not None, ( + f"--{field.replace('_', '-')} is classified as routed under --config but " + f"setting it changed nothing in the resolved config -- it is being " + f"silently dropped. Route it, or list it in UNROUTED_UNDER_CONFIG so " + f"users get an error." + ) + + +# --------------------------------------------------------------------------- +# The whole-CLIConfig guarantee +# --------------------------------------------------------------------------- + +# Enums with a single member: no second value exists to prove an effect with. +SINGLE_VALUED_FIELDS: frozenset[str] = frozenset( + { + # URLSelectionStrategy has exactly one member. + "url_selection_strategy", + # Every mode but the default fails validation on its own, and setting + # a field to the value it already has cannot change anything -- so no + # observation here can distinguish routed from dropped. + "wait_for_model_mode", + } +) + + +def _companions_for(field: str) -> dict[str, Any]: + """Flags that must accompany ``field`` for it to mean anything.""" + from aiperf.config.flags._config_flag_routing import COMPANION_ROUTED + + companions: dict[str, Any] = {} + for companion in COMPANION_ROUTED.get(field, ()): # type: ignore[call-overload] + if companion == "model_names": + companions["model_names"] = ["companion-model"] + elif companion in ("use_server_token_count", "streaming"): + companions[companion] = True + if field.endswith("_stddev"): + mean_field = field[: -len("_stddev")] + "_mean" + if mean_field in CLIConfig.model_fields: + companions[mean_field] = 8 + return companions + + +@pytest.mark.parametrize("field", sorted(_ROUTED_FIELDS_UNDER_TEST)) +def test_routed_field_never_silently_no_ops( + field: str, rich_yaml: Path, trace_yaml: Path +) -> None: + """Every routed flag must change the config or raise -- never both quiet. + + This is the guarantee in one assertion, and it covers all of + ROUTED_UNDER_CONFIG rather than the dataset subset. Sections whose + builders consume the whole section (OUTPUT/TOKENIZER/ACCURACY/SWEEPING) + are marked routed wholesale, so a newly-added member of one of them is + classified as routed automatically; without this test nothing would ever + check whether that claim is true, and the flag would be silently dropped + exactly as before. + + Raising is acceptable: a flag rejected for needing a companion, a + different dataset type, or a streaming endpoint is loud, and loud is the + property we care about. The forbidden outcome is resolving successfully + while ignoring what the user asked for. + """ + if field in MAGIC_LIST_ONLY_UNDER_CONFIG: + pytest.skip(f"{field} routes as a sweep parameter in its list form") + if field in SINGLE_VALUED_FIELDS: + pytest.skip(f"{field} has a single valid value; no effect to observe") + candidates = _candidate_values(field) + if not candidates: + _require_drivable(field) + pytest.skip(f"{field} is listed in UNDRIVABLE_FIELDS") + + companions = _companions_for(field) + # Assert per YAML, not across the union: a field that works on a trace + # dataset and silently no-ops on a synthetic one is a silent drop for + # every user of a synthetic config, and accumulating `changed` across + # both fixtures hid exactly that. + observed_baseline = False + for config_yaml in (rich_yaml, trace_yaml): + try: + baseline = resolve_config(cli(**companions), config_yaml).model_dump( + mode="json" + ) + except (ValueError, TypeError, ConfigurationError): + # TypeError: _build_adaptive_search's "--search-* flags require + # --search-space" -- a real, intentional loud signal for the + # ROUTED_UNDER_CONFIG-wide field set this test covers, not just + # the dataset-scoped ValueError/ConfigurationError pair other + # loops in this file use. + continue + observed_baseline = True + changed = silent_noop = False + for value in candidates: + try: + resolved = resolve_config( + cli(**{field: value}, **companions), config_yaml + ).model_dump(mode="json") + except (ValueError, TypeError, ConfigurationError): + continue # loud: acceptable + if resolved != baseline: + changed = True + else: + silent_noop = True + + assert changed or not silent_noop, ( + f"--{field.replace('_', '-')} is classified as routed under " + f"--config, but with {config_yaml.name} it resolves successfully " + f"and changes nothing -- it is silently ignored. Route it, or move " + f"it to UNROUTED_UNDER_CONFIG so the user gets an error naming it." + ) + assert observed_baseline, ( + f"neither rich_yaml nor trace_yaml produced a baseline for " + f"--{field.replace('_', '-')} (both raised on companions alone), so " + f"this case checked nothing. Fix the companion/fixture combination, " + f"or move the field to UNDRIVABLE_FIELDS with a reason." + ) + + +# One routable flag per dataset type, used as an anchor so the flag under +# test is never the only thing driving the override. +_ANCHOR_FLAGS: dict[str, dict[str, Any]] = { + "rich": {"random_seed": 4242}, + "trace": {"random_seed": 4242}, +} + + +@pytest.mark.parametrize("field", sorted(DATASET_OVERRIDE_FIELDS)) +def test_dataset_flag_is_not_excused_by_a_neighbouring_flag( + field: str, rich_yaml: Path, trace_yaml: Path +) -> None: + """A flag must not become silently droppable by being passed with others. + + The single-flag parametrization above cannot see this: the guard used to + fire only when the whole override came back empty, so any inert flag + riding along with a routable one was discarded with no diagnostic. Every + multi-flag command line touching the dataset escaped the guarantee. + """ + if field in MAGIC_LIST_ONLY_UNDER_CONFIG or field in SINGLE_VALUED_FIELDS: + pytest.skip(f"{field} is covered by the single-flag invariant only") + candidates = _candidate_values(field) + if not candidates: + _require_drivable(field) + pytest.skip(f"{field} is listed in UNDRIVABLE_FIELDS") + + companions = _companions_for(field) + for label, config_yaml in (("rich", rich_yaml), ("trace", trace_yaml)): + anchor = _ANCHOR_FLAGS[label] + if field in anchor: + continue + try: + baseline = resolve_config( + cli(**anchor, **companions), config_yaml + ).model_dump(mode="json") + except Exception: + continue + changed = silent_noop = False + # Every candidate, not just one: a bool's False and an enum's first + # member frequently ARE the resolved default, and setting a field to + # the value it already holds cannot change anything. + for value in candidates: + try: + resolved = resolve_config( + cli(**{field: value}, **anchor, **companions), config_yaml + ).model_dump(mode="json") + except Exception: + continue # loud: acceptable + if resolved != baseline: + changed = True + else: + silent_noop = True + assert changed or not silent_noop, ( + f"--{field.replace('_', '-')} resolves cleanly and changes nothing " + f"when passed alongside {sorted(anchor)} against {config_yaml.name}. " + f"Paired with another flag it is silently dropped, even though it " + f"may error correctly on its own." + ) + + +def test_inert_probe_lets_solo_construction_errors_propagate( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A field whose SOLO CLIConfig construction fails must propagate as a + real error, not be silently treated as "loud" and skipped by the + inert-flag probe. + + ``_inert_dataset_flags`` used to wrap both the solo ``CLIConfig(...)`` + construction and the ``build_dataset(...)`` call in one ``try``, so a + construction failure -- a ``ValueError``, e.g. from a cross-field + validator on CLIConfig itself; theoretical today, no current field hits + it -- was caught by the same ``except (ValueError, ConfigurationError)`` + meant only for ``build_dataset``, and the field silently vanished from + the inert list instead of the failure surfacing. Simulated via + monkeypatch since no real field currently reaches this path. + """ + import aiperf.config.flags as flags_module + from aiperf.config.flags.resolver import _inert_dataset_flags + + class _RaisingCLIConfig: + def __init__(self, **kwargs: object) -> None: + raise ValueError("boom: solo construction failed") + + monkeypatch.setattr(flags_module, "CLIConfig", _RaisingCLIConfig) + + real_cli = cli(synthesis_max_isl=100) + with pytest.raises(ValueError, match="boom"): + _inert_dataset_flags( + real_cli, DatasetType.SYNTHETIC, None, {"synthesis_max_isl"} + ) diff --git a/tests/unit/config/test_config_phase_overrides.py b/tests/unit/config/test_config_phase_overrides.py new file mode 100644 index 0000000000..f0007f6c3f --- /dev/null +++ b/tests/unit/config/test_config_phase_overrides.py @@ -0,0 +1,354 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Phase-shaping CLI flags must survive ``--config``. + +The resolver overlaid only the flat loadgen fields onto the profiling phase. +The ramp durations, request cancellation, and arrival smoothness were built +solely by the CLI-only converter, so under ``--config`` they were dropped -- +and then, once the classification gate landed, rejected outright. + +They are routed here by reusing the converter's own helpers rather than +restating the mapping, so the two paths cannot drift. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from aiperf.config import AIPerfConfig +from aiperf.config.flags import CLIConfig +from aiperf.config.flags.resolver import resolve_config +from aiperf.config.loader.errors import ConfigurationError +from aiperf.config.phases import PhaseConfig + + +def cli(**kwargs: object) -> CLIConfig: + return CLIConfig(**CLIConfig(**kwargs).model_dump(exclude_unset=True)) # type: ignore[arg-type] + + +def profiling(cfg: AIPerfConfig) -> PhaseConfig: + return next(p for p in cfg.benchmark.phases if p.name == "profiling") + + +@pytest.fixture +def concurrency_yaml(tmp_path: Path) -> Path: + cfg = tmp_path / "conc.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + phases: + type: concurrency + concurrency: 4 + requests: 5 +""" + ) + return cfg + + +@pytest.fixture +def gamma_yaml(tmp_path: Path) -> Path: + cfg = tmp_path / "gamma.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + phases: + type: gamma + rate: 10 + requests: 5 +""" + ) + return cfg + + +# --------------------------------------------------------------------------- +# Ramps +# --------------------------------------------------------------------------- + + +def test_concurrency_ramp_duration_routes(concurrency_yaml: Path) -> None: + resolved = profiling( + resolve_config(cli(concurrency_ramp_duration=30), concurrency_yaml) + ) + assert resolved.concurrency_ramp is not None + assert resolved.concurrency_ramp.duration == 30 + + +def test_prefill_concurrency_ramp_duration_routes(concurrency_yaml: Path) -> None: + resolved = profiling( + resolve_config(cli(prefill_concurrency_ramp_duration=15), concurrency_yaml) + ) + assert resolved.prefill_ramp is not None + assert resolved.prefill_ramp.duration == 15 + + +def test_ramp_does_not_disturb_other_phase_values(concurrency_yaml: Path) -> None: + resolved = profiling( + resolve_config(cli(concurrency_ramp_duration=30), concurrency_yaml) + ) + assert resolved.concurrency == 4 + assert resolved.requests == 5 + + +# --------------------------------------------------------------------------- +# Cancellation +# --------------------------------------------------------------------------- + + +def test_request_cancellation_rate_routes(concurrency_yaml: Path) -> None: + resolved = profiling( + resolve_config(cli(request_cancellation_rate=25.0), concurrency_yaml) + ) + assert resolved.cancellation is not None + assert resolved.cancellation.rate == 25.0 + + +def test_request_cancellation_delay_routes_with_rate(concurrency_yaml: Path) -> None: + resolved = profiling( + resolve_config( + cli(request_cancellation_rate=25.0, request_cancellation_delay=2.0), + concurrency_yaml, + ) + ) + assert resolved.cancellation.delay == 2.0 + + +def test_cancellation_delay_without_rate_still_errors(concurrency_yaml: Path) -> None: + """The converter's dependency guard must hold on this path too.""" + with pytest.raises(ValueError, match="requires --request-cancellation-rate"): + resolve_config(cli(request_cancellation_delay=2.0), concurrency_yaml) + + +# --------------------------------------------------------------------------- +# Arrival smoothness +# --------------------------------------------------------------------------- + + +def test_arrival_smoothness_routes_on_gamma_phase(gamma_yaml: Path) -> None: + resolved = profiling(resolve_config(cli(arrival_smoothness=0.5), gamma_yaml)) + assert resolved.smoothness == 0.5 + + +def test_arrival_smoothness_rejected_on_non_gamma_phase( + concurrency_yaml: Path, +) -> None: + with pytest.raises(ValueError, match="arrival-smoothness"): + resolve_config(cli(arrival_smoothness=0.5), concurrency_yaml) + + +# --------------------------------------------------------------------------- +# Warmup +# --------------------------------------------------------------------------- + + +def warmup(cfg: AIPerfConfig) -> PhaseConfig | None: + return next((p for p in cfg.benchmark.phases if p.name == "warmup"), None) + + +@pytest.fixture +def warmup_yaml(tmp_path: Path) -> Path: + """A config that already declares a warmup phase.""" + cfg = tmp_path / "warmup.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + phases: + - name: warmup + kind: warmup + type: concurrency + concurrency: 2 + requests: 3 + - name: profiling + kind: profiling + type: concurrency + concurrency: 4 + requests: 5 +""" + ) + return cfg + + +def test_warmup_flag_overrides_existing_warmup_phase(warmup_yaml: Path) -> None: + resolved = resolve_config(cli(warmup_request_count=9), warmup_yaml) + assert warmup(resolved).requests == 9 + + +def test_warmup_flag_leaves_untouched_warmup_values_alone(warmup_yaml: Path) -> None: + """Only what the user set may change on the YAML's warmup phase.""" + resolved = resolve_config(cli(warmup_request_count=9), warmup_yaml) + assert warmup(resolved).concurrency == 2 + + +def test_warmup_flag_does_not_disturb_the_profiling_phase(warmup_yaml: Path) -> None: + resolved = resolve_config(cli(warmup_request_count=9), warmup_yaml) + assert profiling(resolved).requests == 5 + assert profiling(resolved).concurrency == 4 + + +def test_secondary_warmup_flag_applies_to_an_existing_phase( + warmup_yaml: Path, +) -> None: + """--warmup-concurrency has no trigger of its own. + + On the CLI-only path that means "no warmup phase", so the flag is + ignored. Here the config file already declares the phase, so the flag + has somewhere to land and must be applied rather than dropped. + """ + resolved = resolve_config(cli(warmup_concurrency=7), warmup_yaml) + assert warmup(resolved).concurrency == 7 + + +def test_warmup_trigger_creates_a_phase_when_yaml_has_none( + concurrency_yaml: Path, +) -> None: + """A trigger flag builds the warmup phase, as it does CLI-only.""" + resolved = resolve_config(cli(warmup_request_count=4), concurrency_yaml) + created = warmup(resolved) + assert created is not None + assert created.requests == 4 + assert created.exclude_from_results is True + + +def test_secondary_warmup_flag_without_a_phase_or_trigger_errors( + concurrency_yaml: Path, +) -> None: + """Nowhere to land and nothing to create it: must be loud, not dropped.""" + with pytest.raises(ConfigurationError, match=r"warmup"): + resolve_config(cli(warmup_concurrency=7), concurrency_yaml) + + +def test_warmup_request_rate_onto_concurrency_yaml_errors(warmup_yaml: Path) -> None: + """--warmup-request-rate alone onto a YAML concurrency warmup phase must + raise a clear, named error rather than merge into an invalid phase. + + _warmup_override_pattern emits `rate` independently of `type`, so this + used to produce {type: concurrency, rate: 5.0, ...} and fail deep inside + AIPerfConfig.model_validate with a raw "Extra inputs are not permitted" + pydantic error instead of naming the flag. + """ + with pytest.raises(ConfigurationError, match=r"--warmup-request-rate"): + resolve_config(cli(warmup_request_rate=5.0), warmup_yaml) + + +def test_warmup_arrival_pattern_onto_concurrency_yaml_errors(warmup_yaml: Path) -> None: + """--warmup-arrival-pattern alone onto a YAML concurrency warmup phase + must raise a clear, named error rather than merge into an invalid phase. + + _warmup_override_pattern switches `type` to the rate phase without + setting `rate`, so this used to fail deep inside + AIPerfConfig.model_validate with "rate-controlled phases require rate or + rate_series" instead of naming the flag. + """ + with pytest.raises(ConfigurationError, match=r"--warmup-arrival-pattern"): + resolve_config(cli(warmup_arrival_pattern="constant"), warmup_yaml) + + +def test_warmup_request_rate_and_arrival_pattern_together_transitions_cleanly( + warmup_yaml: Path, +) -> None: + """Setting both flags together is a complete, valid transition and must + not be rejected.""" + resolved = resolve_config( + cli(warmup_request_rate=5.0, warmup_arrival_pattern="constant"), + warmup_yaml, + ) + w = warmup(resolved) + assert w.type == "constant" + assert w.rate == 5.0 + + +# --------------------------------------------------------------------------- +# Goodput -> the slos block +# --------------------------------------------------------------------------- + + +def test_goodput_routes_to_slos(concurrency_yaml: Path) -> None: + resolved = resolve_config(cli(goodput="ttft:200"), concurrency_yaml) + assert resolved.benchmark.slos == {"ttft": 200.0} + + +def test_goodput_merges_with_yaml_slos(tmp_path: Path) -> None: + """A YAML slos block keeps the keys the flag does not mention.""" + cfg = tmp_path / "slos.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + slos: + tpot: 50 + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + resolved = resolve_config(cli(goodput="ttft:200"), cfg) + assert resolved.benchmark.slos == {"tpot": 50.0, "ttft": 200.0} + + +# --------------------------------------------------------------------------- +# Fixed schedule +# --------------------------------------------------------------------------- + + +@pytest.fixture +def trace_phase_yaml(tmp_path: Path) -> Path: + trace = tmp_path / "t.jsonl" + trace.write_text('{"timestamp": 0, "text": "hi"}\n') + cfg = tmp_path / "trace_phase.yaml" + cfg.write_text( + f"""\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: file + format: mooncake_trace + path: {trace} + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +def test_fixed_schedule_switches_the_phase_type(trace_phase_yaml: Path) -> None: + resolved = resolve_config(cli(fixed_schedule=True), trace_phase_yaml) + assert profiling(resolved).type == "fixed_schedule" + + +def test_fixed_schedule_offsets_route(trace_phase_yaml: Path) -> None: + resolved = profiling( + resolve_config( + cli(fixed_schedule=True, fixed_schedule_start_offset=5), trace_phase_yaml + ) + ) + assert resolved.start_offset == 5 diff --git a/tests/unit/config/test_config_telemetry_overrides.py b/tests/unit/config/test_config_telemetry_overrides.py new file mode 100644 index 0000000000..438d09c815 --- /dev/null +++ b/tests/unit/config/test_config_telemetry_overrides.py @@ -0,0 +1,251 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Telemetry and scenario flags must survive ``--config``. + +``build_mlflow``, ``build_otel``, and ``build_network_latency`` have always +existed and are called by the CLI-only converter, but ``build_cli_overrides`` +never called them -- so every ``--mlflow-*`` flag, ``--otel-url``, and the +network-latency flags were silently discarded whenever a config file was +supplied. Same story for ``--scenario`` / ``--unsafe-override``, which the +converter writes onto the benchmark body via ``_apply_scenario_fields``. + +These flags configure where results are *published*. Dropping them silently +means a run reports to the wrong MLflow experiment, or to none at all, with +no diagnostic. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from aiperf.config.flags import CLIConfig +from aiperf.config.flags.resolver import resolve_config + + +def cli(**kwargs: object) -> CLIConfig: + return CLIConfig(**CLIConfig(**kwargs).model_dump(exclude_unset=True)) # type: ignore[arg-type] + + +@pytest.fixture +def base_yaml(tmp_path: Path) -> Path: + cfg = tmp_path / "base.yaml" + cfg.write_text( + """\ +schemaVersion: "2.0" +benchmark: + model: test-model + endpoint: + url: http://localhost:8000 + dataset: + type: synthetic + phases: + type: concurrency + concurrency: 1 + requests: 5 +""" + ) + return cfg + + +# --------------------------------------------------------------------------- +# MLflow +# --------------------------------------------------------------------------- + + +def test_mlflow_tracking_uri_routes(base_yaml: Path) -> None: + resolved = resolve_config(cli(mlflow_tracking_uri="http://mlflow:5000"), base_yaml) + assert resolved.benchmark.mlflow.tracking_uri == "http://mlflow:5000" + + +def test_mlflow_experiment_routes(base_yaml: Path) -> None: + resolved = resolve_config( + cli( + mlflow_tracking_uri="http://mlflow:5000", mlflow_experiment="my-experiment" + ), + base_yaml, + ) + assert resolved.benchmark.mlflow.experiment == "my-experiment" + + +def test_mlflow_run_name_routes(base_yaml: Path) -> None: + resolved = resolve_config( + cli(mlflow_tracking_uri="http://mlflow:5000", mlflow_run_name="run-7"), + base_yaml, + ) + assert resolved.benchmark.mlflow.run_name == "run-7" + + +def test_mlflow_secondary_flag_without_tracking_uri_still_errors( + base_yaml: Path, +) -> None: + """The guard stands when neither the CLI nor the YAML supplies a URI.""" + with pytest.raises(ValueError, match="require --mlflow-tracking-uri"): + resolve_config(cli(mlflow_experiment="orphan"), base_yaml) + + +def test_mlflow_flag_overrides_yaml_value(base_yaml: Path, tmp_path: Path) -> None: + """A YAML-supplied mlflow block must lose to the explicit flag.""" + cfg = tmp_path / "with_mlflow.yaml" + cfg.write_text( + base_yaml.read_text().replace( + " phases:", + " mlflow:\n experiment: from-yaml\n tracking_uri: http://yaml:5000\n phases:", + ) + ) + resolved = resolve_config(cli(mlflow_experiment="from-cli"), cfg) + assert resolved.benchmark.mlflow.experiment == "from-cli" + # The flag the user did not pass must survive untouched. + assert resolved.benchmark.mlflow.tracking_uri == "http://yaml:5000" + + +# --------------------------------------------------------------------------- +# OpenTelemetry +# --------------------------------------------------------------------------- + + +def test_otel_url_routes(base_yaml: Path) -> None: + resolved = resolve_config(cli(otel_url="http://otel:4318"), base_yaml) + assert resolved.benchmark.otel.metrics_url is not None + + +def test_gen_ai_provider_routes(base_yaml: Path) -> None: + resolved = resolve_config( + cli(otel_url="http://otel:4318", gen_ai_provider="nvidia"), base_yaml + ) + assert resolved.benchmark.otel.gen_ai_provider == "nvidia" + + +# --------------------------------------------------------------------------- +# Network latency +# --------------------------------------------------------------------------- + + +def test_network_latency_mean_routes(base_yaml: Path) -> None: + resolved = resolve_config(cli(network_latency_mean=12.5), base_yaml) + assert resolved.benchmark.network_latency.mean_ms == 12.5 + + +def test_network_latency_automatic_routes(base_yaml: Path) -> None: + resolved = resolve_config(cli(network_latency_automatic=True), base_yaml) + assert resolved.benchmark.network_latency.enabled is True + + +# --------------------------------------------------------------------------- +# Scenario lock +# --------------------------------------------------------------------------- + + +def test_unsafe_override_routes(base_yaml: Path) -> None: + resolved = resolve_config(cli(unsafe_override=True), base_yaml) + assert resolved.benchmark.unsafe_override is True + + +# --------------------------------------------------------------------------- +# Runtime: --api-host with a YAML-supplied api_port +# --------------------------------------------------------------------------- + + +@pytest.fixture +def api_port_yaml(base_yaml: Path, tmp_path: Path) -> Path: + cfg = tmp_path / "with_port.yaml" + cfg.write_text( + base_yaml.read_text().replace( + " phases:", " runtime:\n api_port: 19090\n phases:" + ) + ) + return cfg + + +def test_api_host_accepts_port_from_yaml(api_port_yaml: Path) -> None: + """--api-host must not demand a flag the config file already supplies. + + build_logging_runtime raises when api_host is set and api_port is None, + which under --config is exactly the supported case: the port came from + the file. + """ + resolved = resolve_config(cli(api_host="0.0.0.0"), api_port_yaml) + assert resolved.benchmark.runtime.api_host == "0.0.0.0" + assert resolved.benchmark.runtime.api_port == 19090 + + +def test_api_host_without_any_port_still_errors(base_yaml: Path) -> None: + """The guard stands when neither the CLI nor the YAML supplies a port.""" + with pytest.raises(ValueError, match="api_host requires api_port"): + resolve_config(cli(api_host="0.0.0.0"), base_yaml) + + +@pytest.fixture +def otel_yaml(base_yaml: Path, tmp_path: Path) -> Path: + cfg = tmp_path / "with_otel.yaml" + cfg.write_text( + base_yaml.read_text().replace( + " phases:", + " otel:\n metrics_url: http://yaml-otel:4318/v1/metrics\n phases:", + ) + ) + return cfg + + +def test_gen_ai_provider_accepts_otel_url_from_yaml(otel_yaml: Path) -> None: + """Secondary OTel flags must not demand a URL the config file supplies.""" + resolved = resolve_config(cli(gen_ai_provider="nvidia"), otel_yaml) + assert resolved.benchmark.otel.gen_ai_provider == "nvidia" + assert str(resolved.benchmark.otel.metrics_url).startswith("http://yaml-otel") + + +def test_otel_secondary_flag_without_any_url_still_errors(base_yaml: Path) -> None: + """The guard stands when neither source supplies a metrics URL.""" + with pytest.raises(ValueError, match="requires --otel-url"): + resolve_config(cli(gen_ai_provider="nvidia"), base_yaml) + + +# --------------------------------------------------------------------------- +# Endpoint probes: reset-kv-cache and server-profiler +# --------------------------------------------------------------------------- + + +def test_reset_kv_cache_routes(base_yaml: Path) -> None: + resolved = resolve_config(cli(reset_kv_cache=True), base_yaml) + assert resolved.benchmark.endpoint.reset_kv_cache is not None + + +def test_reset_kv_cache_path_routes(base_yaml: Path) -> None: + resolved = resolve_config( + cli(reset_kv_cache=True, reset_kv_cache_path="/reset"), base_yaml + ) + assert resolved.benchmark.endpoint.reset_kv_cache.path == "/reset" + + +def test_server_profiler_routes(base_yaml: Path) -> None: + resolved = resolve_config( + cli(server_profiler=True, server_profiler_start_path="/start"), base_yaml + ) + assert resolved.benchmark.endpoint.server_profiler.start_path == "/start" + + +def test_probe_flag_overrides_yaml_block(base_yaml: Path, tmp_path: Path) -> None: + """A YAML-supplied probe block must lose to the explicit flag, field-wise.""" + cfg = tmp_path / "with_probe.yaml" + cfg.write_text( + base_yaml.read_text() + .replace( + " dataset:", + " endpoint2_placeholder: 0\n dataset:", + ) + .replace(" endpoint2_placeholder: 0\n", "") + ) + cfg.write_text( + base_yaml.read_text().replace( + " url: http://localhost:8000", + " url: http://localhost:8000\n" + " reset_kv_cache:\n" + " path: /from-yaml\n" + " timeout_seconds: 30", + ) + ) + resolved = resolve_config(cli(reset_kv_cache_path="/from-cli"), cfg) + assert resolved.benchmark.endpoint.reset_kv_cache.path == "/from-cli" + # The field the user did not pass survives. + assert resolved.benchmark.endpoint.reset_kv_cache.timeout_seconds == 30 diff --git a/tests/unit/config/test_trace_flag_routing.py b/tests/unit/config/test_trace_flag_routing.py index 0b3ae2633e..c108177b6e 100644 --- a/tests/unit/config/test_trace_flag_routing.py +++ b/tests/unit/config/test_trace_flag_routing.py @@ -15,9 +15,10 @@ from aiperf.config.flags.cli_config import CLIConfig from aiperf.config.flags.converter import convert_cli_to_aiperf from aiperf.config.flags.resolver import ( - _apply_dataset_synthesis_overrides, + _apply_dataset_overrides, resolve_config, ) +from aiperf.config.loader.errors import ConfigurationError from aiperf.plugin.enums import CustomDatasetType, PublicDatasetType _WEKA_HF = PublicDatasetType.SEMIANALYSIS_CC_TRACES_WEKA_WITH_SUBAGENTS @@ -253,9 +254,9 @@ def test_invalid_yaml_datasets_raise(self, datasets: list[object]) -> None: merged = {"benchmark": {"datasets": datasets}} with pytest.raises( - ValueError, match="synthesis flags require a file or public dataset" + ConfigurationError, match="require a dataset in the config file" ): - _apply_dataset_synthesis_overrides(merged, CLIConfig(synthesis_max_osl=512)) + _apply_dataset_overrides(merged, CLIConfig(synthesis_max_osl=512)) def test_multiple_yaml_datasets_warns_and_updates_only_first( self, caplog: pytest.LogCaptureFixture @@ -268,24 +269,25 @@ def test_multiple_yaml_datasets_warns_and_updates_only_first( merged = {"benchmark": {"datasets": [first, second]}} with caplog.at_level("WARNING", logger="aiperf.config.flags.resolver"): - _apply_dataset_synthesis_overrides( - merged, CLIConfig(synthesis_max_osl=12000) - ) + _apply_dataset_overrides(merged, CLIConfig(synthesis_max_osl=12000)) assert "apply only to the first dataset" in caplog.text - assert first["synthesis"] == {"speedupRatio": 2.0, "maxOsl": 12000} + assert first["synthesis"] == {"speedupRatio": 2.0, "max_osl": 12000} assert second["synthesis"] == {"maxOsl": 8000} - def test_non_trace_yaml_dataset_warns_and_is_unchanged( - self, caplog: pytest.LogCaptureFixture - ) -> None: + def test_non_trace_yaml_dataset_is_rejected(self) -> None: + """Synthesis has no meaning on a synthetic dataset, so say so. + + This previously warned and dropped the flag; a warning on stderr is + not enough when the result is a benchmark that ignored what the user + asked for. + """ dataset = {"type": "synthetic"} merged = {"benchmark": {"datasets": [dataset]}} - with caplog.at_level("WARNING", logger="aiperf.config.flags.resolver"): - _apply_dataset_synthesis_overrides(merged, CLIConfig(synthesis_max_osl=512)) + with pytest.raises(ConfigurationError, match="have no effect"): + _apply_dataset_overrides(merged, CLIConfig(synthesis_max_osl=512)) - assert "require a file or public dataset" in caplog.text assert dataset == {"type": "synthetic"} @pytest.mark.parametrize( @@ -311,7 +313,7 @@ def test_baseten_yaml_rejects_unsupported_synthesis( } with pytest.raises(ValueError, match=match) as exc_info: - _apply_dataset_synthesis_overrides(merged, CLIConfig(**cli_kwargs)) + _apply_dataset_overrides(merged, CLIConfig(**cli_kwargs)) assert "YAML format: baseten_trace" in str(exc_info.value) assert "--custom-dataset-type" not in str(exc_info.value) @@ -320,11 +322,9 @@ def test_baseten_yaml_accepts_output_len_synthesis(self) -> None: dataset = {"type": "file", "format": "baseten_trace"} merged = {"benchmark": {"datasets": [dataset]}} - _apply_dataset_synthesis_overrides( - merged, CLIConfig(synthesis_output_len_multiplier=2.0) - ) + _apply_dataset_overrides(merged, CLIConfig(synthesis_output_len_multiplier=2.0)) - assert dataset["synthesis"] == {"outputLenMultiplier": 2.0} + assert dataset["synthesis"] == {"output_len_multiplier": 2.0} class TestOslFallbackRouting: