DRAFT: feat(config): consolidate all durations and deadlines into one Timeouts model - #409
DRAFT: feat(config): consolidate all durations and deadlines into one Timeouts model#409viraatc wants to merge 5 commits into
Conversation
…ts model One frozen pydantic model (config/timeouts.py, mounted at settings.timeouts) now owns every global time knob: min/max duration, per-phase drain deadlines, service-ready deadline, metrics drain budget, and the whole-run watchdog. - BenchmarkConfig.timeout was a silent no-op consumed nowhere; it is now settings.timeouts.run_timeout_s (--timeout alias preserved): a real whole-run watchdog that SIGTERMs the metrics aggregator (INTERRUPTED final snapshot), stops the session, and exits non-zero. A fired watchdog can never produce a COMPLETE report (locked by integration test). - DrainConfig deleted; drain fields moved to settings.timeouts with CLI aliases unchanged. metrics_drain_timeout_s normalized to None=unlimited (was 0=unlimited); the aggregator argv boundary still speaks 0. - metrics_tokenizer_workers is not a timeout: moved to settings.metrics (new MetricsConfig block). - min/max_duration_ms (+ suffix parsing + cross-validator) moved from RuntimeConfig to Timeouts; RuntimeSettings resolves them to plain values at setup, so nothing in the hot path reads pydantic. - ServiceLauncher.terminate_all() added (graceful SIGTERM counterpart to kill_all) for the watchdog path. - Templates regenerated; examples/docs/tests migrated (YAML keys moved, no back-compat shims per repo convention). Example run_timeout_s values were dropped rather than carried over: the old top-level timeout was inert, and enforcing stale values would abort valid runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration schema by centralizing all global durations, deadlines, and timeouts into a new frozen Pydantic model Timeouts (accessible via settings.timeouts). This separates workload durations from failure-handling deadlines. Additionally, a whole-run watchdog (run_timeout_s) has been introduced to gracefully abort stuck runs, signaling managed subprocesses via SIGTERM to write an interrupted final snapshot before exiting non-zero. All configuration templates, examples, and tests have been updated to align with this new schema, and new integration tests have been added to verify the watchdog behavior. No review comments were provided, so there is no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…on semantics - performance_drain_timeout_s default 240.0 -> None (wait indefinitely), matching the accuracy drain default. - min/max_duration_ms help/description now state they bound the whole performance phase (single phase; not per-dataset) and never bound warmup/accuracy. Templates regenerated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
schema.py (1415 lines) now holds only BenchmarkConfig/EndpointConfig and re-exports the full schema surface; models move to sibling modules: enums.py, audit.py, model_params.py, datasets.py, settings.py (timeouts.py already existed). Import sites are unchanged — config.schema remains the single import point. Dead code deleted: SystemDefaults (DEFAULT_TIMEOUT had no consumers; DEFAULT_METRIC inlined into rulesets/mlcommons/rules.py) and the unused TEMPLATE_TYPE_MAP. regenerate-templates pre-commit hook now watches the new modules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Findings from the review council (Codex review + adversary council), all verified against the code before fixing: - Watchdog now stays armed through the unbounded metrics drain: it was cancelled right after session.run, so run_timeout_s could not bound a stuck aggregator drain (wait_for_exit(None)). Cancelled after services exit instead. - Watchdog SIGTERMs only the metrics aggregator (ServiceLauncher.terminate with module suffix, replacing terminate_all): SIGTERMing the event logger dropped its buffered events.jsonl tail; the logger flushes on the ENDED event, which session.stop() still delivers. - A timed-out run skips accuracy scoring in finalize: phases that never started KeyError in scorer init and partial phases would yield misleading subset scores. Artifacts are still salvaged. - Teardown race no longer skips finalization: if session.run raises after the watchdog fired, fall through with an empty SessionResult so result_summary.json (INTERRUPTED, complete=false) is always written; run_benchmark raises the timeout ExecutionError after finalize. - run_audit maps a watchdog fire to ExecutionError naming the timeout instead of the Ctrl-C KeyboardInterrupt path (exit 130). - MetricsConfig gets cyclopts.Parameter(name='*') matching sibling settings blocks (flat --tokenizer-workers + --metrics-tokenizer-workers). - Stale drain-key name fixed in session.py docstring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # (launched once from top-level model_params), so a per-dataset override would | ||
| # desync ISL/OSL/TTFT/TPOT accounting without changing what is measured. Rejected | ||
| # as generation_config_override keys — they are per-run/identity, not per-dataset. | ||
| _METRICS_DECOUPLED_OVERRIDE_KEYS = frozenset({"name", "streaming", "tokenizer_name"}) |
viraatc
changed the title
…entinels min_duration_ms and n_samples_to_issue are now mutually exclusive (Settings validator): the sample count is either an explicit count or derived from QPS x duration — previously an explicit count silently won and the configured duration was dead weight. - min_duration_ms: int | None = None. Omitting both knobs issues the dataset once. BEHAVIOR CHANGE: a bare config (neither set) previously derived qps x 600s worth of samples; it now runs the dataset once. The 0 = 'all dataset samples' sentinel is gone (0 now rejected). - max_duration_ms: int | None = None, replacing the 0 = 'no limit' sentinel; the argv-boundary 0->None conversion in RuntimeSettings is deleted (internal RuntimeSettings stays permissive for programmatic callers passing 0). - Examples/docs/templates migrated: dropped min_duration where an explicit count is set (it was silently ignored), dropped --duration 0 usage, fixed compare_with_vllm.py double-cap (it set max_duration_ms from the same value it passed as --timeout, capping one budget via two mechanisms), corrected CLI_QUICK_REFERENCE/LOCAL_TESTING stale defaults and key locations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| - `--runtime.n-samples-to-issue --num-samples` - Explicit sample count override | ||
| - `--min-duration-ms --duration` - Min perf-phase duration: ms default, or with suffix (600s, 10m); sample count = QPS x duration | ||
| - `--runtime.n-samples-to-issue --num-samples` - Explicit sample count | ||
| - `--duration` and `--num-samples` are mutually exclusive; omit both to issue the dataset once (the default) |
There was a problem hiding this comment.
Do you mean if both are set, both will be omitted?
What is the current loadgen behavior? I am thinking whether taking the max of the 2 will make more sense or we should error out
There was a problem hiding this comment.
Can we drop duration - this is overloading with the timeout as the duration of a benchmark can be the bounded max-runtime. We can rely on num-samples to specify exactly how many samples the user wants to run.
This will bound the lower end - and we can use the timeout on the upper end.
I think this would make it easier to understand rather than having to recall the details every time we specify duration, timeout and num-samples.
There was a problem hiding this comment.
I think he is trying to say if we set neither, it will just sweep once for the dataset we sepcified ? But yeah, a little unclear at initial read.
Also +1 to @arekay-nv . Having --min-duration-ms --duration representing the same it confusing
| scheduler_random_seed: 42 # For Poisson/distribution sampling | ||
| dataloader_random_seed: 42 # For dataset shuffling | ||
| timeouts: | ||
| min_duration_ms: 600000 # 10 minutes (or set runtime.n_samples_to_issue instead; omit both = dataset once) |
There was a problem hiding this comment.
Trying to think why min_duration is a timeout? Sounds like it's a lower bound instead of a upper bound
| - Sample priority: `--num-samples` > dataset size (when `--duration 0`) > calculated (target_qps × duration) | ||
| - Default duration: 600000ms (10 minutes) | ||
| - By default (no `--num-samples`, no `--duration`) a run stops after issuing the dataset once | ||
| - `--num-samples` (explicit count) and `--duration` (count = target_qps × duration) are mutually exclusive |
There was a problem hiding this comment.
Does duration work when running without target_qps (fixed concurrency or offline)?
| min_duration_ms: 6000 # 6 seconds | ||
| max_duration_ms: 60000 # 1 minute |
There was a problem hiding this comment.
It's a bit weird to put runtime lower/upper bound in the timeouts?
|
|
||
| timeouts: | ||
| min_duration_ms: 600000 # 10 minutes (mutually exclusive with runtime.n_samples_to_issue) | ||
| warmup_drain_timeout_s: 240.0 # Warmup drain timeout in seconds (None = wait indefinitely) |
There was a problem hiding this comment.
Should we consider removing this? It seems that we are trying to prevent a hang in warmup (or excessive OSL), but a hang in warmup might signal something wrong or simply means that the warmup is not set correctly. (Or what does the drain mean here)
cc: @arekay-nv
There was a problem hiding this comment.
Yes. We should drop it. One option is to move to per-phase timeouts which limit each phase and have a global timeout like the one in the current PR.
The global timeout enforces the overall duration.
We are already doing something similar for drain - there is a per-phase drain config
Suggestion - Remove warmup timeout here, and create an issue to promote warmup to a phase type .
arekay-nv
left a comment
arekay-nv
left a comment
There was a problem hiding this comment.
I think the schema breakdown make sense and is a lot cleaner.
Regarding the timeouts - two suggestions, and feedback is welcome:
- Remove the duration field - makes it simpler especially since we are mostly going to be doing concurrency based runs.
- Modularize the phases with an explicit type of phases and dependencies, but move the per-phase timeouts/drains etc there.
So a global timeout for everything - and a per-phase config for controlling how a phase behaves. We can have some explicit dependencies such aswarmupalways goes beforeperformance,reportingcomes afteraccuracyetc.
| - `--runtime.n-samples-to-issue --num-samples` - Explicit sample count override | ||
| - `--min-duration-ms --duration` - Min perf-phase duration: ms default, or with suffix (600s, 10m); sample count = QPS x duration | ||
| - `--runtime.n-samples-to-issue --num-samples` - Explicit sample count | ||
| - `--duration` and `--num-samples` are mutually exclusive; omit both to issue the dataset once (the default) |
There was a problem hiding this comment.
Can we drop duration - this is overloading with the timeout as the duration of a benchmark can be the bounded max-runtime. We can rely on num-samples to specify exactly how many samples the user wants to run.
This will bound the lower end - and we can use the timeout on the upper end.
I think this would make it easier to understand rather than having to recall the details every time we specify duration, timeout and num-samples.
| - Default duration: 600000ms (10 minutes) | ||
| - `--num-samples` and `--duration` are mutually exclusive: explicit count, or calculated (target_qps × duration) | ||
| - Default (neither set): issue the dataset once |
There was a problem hiding this comment.
Dropping duration makes it simpler. And by default num-samples is none which means a single pass of the dataset.
|
|
||
| timeouts: | ||
| min_duration_ms: 600000 # 10 minutes (mutually exclusive with runtime.n_samples_to_issue) | ||
| warmup_drain_timeout_s: 240.0 # Warmup drain timeout in seconds (None = wait indefinitely) |
There was a problem hiding this comment.
Yes. We should drop it. One option is to move to per-phase timeouts which limit each phase and have a global timeout like the one in the current PR.
The global timeout enforces the overall duration.
We are already doing something similar for drain - there is a per-phase drain config
Suggestion - Remove warmup timeout here, and create an issue to promote warmup to a phase type .
| - `--model-params.osl-distribution.min --min-output-tokens` - Min output tokens (default: 1) | ||
| - `--model-params.streaming --streaming` - Streaming mode: auto/on/off (default: auto) | ||
| - `--runtime.min-duration-ms --duration` - Min duration: ms default, or with suffix (600s, 10m) (default: 600000) | ||
| - `--runtime.n-samples-to-issue --num-samples` - Explicit sample count override |
There was a problem hiding this comment.
Do you think we should also add another line to explain max_duration_ms? and run_timeout_s Wonder what is the expectd behavior when max_duration_ms is 0 ?
| does not expose a CLI override for `report_dir`. Set it in the YAML only if you need to control | ||
| the output location; otherwise a default report directory is used. | ||
| - `--timeout` - Global timeout in seconds | ||
| - `--timeout` - Whole-run watchdog in seconds (off by default). If it fires, the run is aborted and the report is marked INTERRUPTED (exits non-zero). |
There was a problem hiding this comment.
This is again, confusing to me. We have timeout already in settings.timeout. And under that we have another timeout?
If we really need this, I would suggest also update Desgin.md as well and comeup with a different name?
| | `min_duration_ms` | `int` | runtime config | | ||
| | `max_duration_ms` | `int` | runtime config | | ||
| | `min_duration_ms` | `int` | `settings.timeouts` | | ||
| | `max_duration_ms` | `int` | `settings.timeouts` | |
There was a problem hiding this comment.
Maybe we can talk in a different topic. The yaml file and CMD flag are hard to correlate to each other. We are using yaml so it is fine., but we need to structure it if we want to convince more people to use it.
| num_requests: Number of requests to send | ||
| max_output_tokens: Maximum output tokens per request | ||
| workers: Number of parallel http-client workers | ||
| timeout: Timeout in seconds |
There was a problem hiding this comment.
If timeout is not used at all. Suggest to delete it everywhere
| timeouts: | ||
| min_duration_ms: 600000 # 10 minutes (mutually exclusive with runtime.n_samples_to_issue) | ||
| warmup_drain_timeout_s: 240.0 # Warmup drain timeout in seconds (None = wait indefinitely) | ||
| performance_drain_timeout_s: null # Performance drain timeout in seconds (None = wait indefinitely) |
There was a problem hiding this comment.
Wonder where is this coming from? never see these flag in any logs or readme files.
Also do we want to simply the timeout story a bit? To many flags make it hard for us to use. Especially when they coexists or conflicts. For example, now you have min_duration_ms which is doing the same thing as performance_drain_timeout_s
| if proc.poll() is None: | ||
| proc.kill() | ||
|
|
||
| def terminate(self, module_suffix: str) -> None: |
There was a problem hiding this comment.
Is it possible to use def terminate_metrics_aggregator(self) -> None: intead? I feel like the scope of the function is too big. We should not desgin our existing code based on some future requirement which might not be true.
| run_timeout_s: null # Whole-run watchdog in seconds (None = off). Covers every phase including drains; firing aborts the run, marks the report INTERRUPTED, and exits non-zero. Never derives per-stage deadlines. | ||
| service_ready_timeout_s: 30.0 # Seconds to wait for metrics-aggregator/event-logger services to become ready. | ||
| warmup_drain_timeout_s: 240.0 # Warmup drain timeout in seconds (None = wait indefinitely) | ||
| performance_drain_timeout_s: null # Performance drain timeout in seconds (None = wait indefinitely) |
There was a problem hiding this comment.
Would be good to include these in read me instruction and explain the use case and priority of each one
| @@ -0,0 +1,264 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
wonder why you seperate part of the config and left some in the schema.py? What is your critira to do the splitting?
| loop, max_duration_ms, _on_perf_phase_timeout | ||
| ) | ||
| run_watchdog = ( | ||
| loop.call_later(run_timeout_s, _on_run_timeout) |
There was a problem hiding this comment.
this seems only start after service readiness. And server start up is bounded by service_ready_timeout_s.
Consider to start this earlier.
| @@ -0,0 +1,195 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
can we add a normal completion before timeout? And another timeout during metric darining?
4 participants
What does this PR do?
Consolidates every global time knob into one frozen pydantic model —
Timeoutsin the newsrc/inference_endpoint/config/timeouts.py, mounted atsettings.timeouts— gives the previously dead--timeoutflag real semantics, splits the 1400-lineconfig/schema.pyinto focused modules, and makes the sample-count knobs mutually exclusive.The headline bug this fixes
BenchmarkConfig.timeout(--timeout, top-leveltimeout:YAML key) was consumed nowhere — a silent no-op. Several example YAMLs set it believing it bounded the run. It is nowsettings.timeouts.run_timeout_s: a real whole-run watchdog. When it fires, it SIGTERMs the metrics aggregator (whose handler writes an INTERRUPTEDfinal_snapshot.json), stops the session, andrun_benchmarkraisesExecutionError(non-zero exit) after finalization — a fired watchdog can never yield a COMPLETE report and always leavesresult_summary.jsonwithcomplete: false. Locked bytests/integration/commands/test_run_timeout.py.The one model
All CLI flag aliases are unchanged. Only YAML key locations and auto-generated dotted flags moved.
Sample count: explicit XOR duration-derived (new)
runtime.n_samples_to_issue(--num-samples) andtimeouts.min_duration_ms(--duration) are now mutually exclusive (Settings validator). Previously an explicit count silently won and the configured duration was dead weight — several examples carried both. Omitting both issues the dataset once, which also deletes the oldmin_duration_ms: 0= "all samples" andmax_duration_ms: 0= "no limit" sentinels (0 is now rejected loudly).qps × 600sworth of samples; it now runs the dataset once.performance_drain_timeout_sdefault is now None (unbounded). A default run has no time backstop on a wedged endpoint — setrun_timeout_sfor unattended runs.metrics_drain_timeout_s: 0/min_duration_ms: 0/max_duration_ms: 0now error loudly.run_timeout_svalues were dropped, not carried over (old top-leveltimeout:was inert; enforcing stale values would hard-kill valid runs). Also fixedcompare_with_vllm.py, which capped the same budget twice viamax_duration_msand--timeout.schema.py split
config/schema.pynow owns onlyBenchmarkConfig/EndpointConfigand re-exports the full schema surface (import sites unchanged). Models moved toconfig/enums.py,audit.py,model_params.py,datasets.py,settings.py(+ existingtimeouts.py). Dead code deleted:SystemDefaults(DEFAULT_TIMEOUThad zero consumers),TEMPLATE_TYPE_MAP.Design invariants (multi-model design review: Codex/Opus/Grok/Gemini)
run_timeout_snever derives per-stage deadlines (no remaining-budget propagation).min/max_duration_msbound the whole performance phase (single phase; not per-dataset);max_duration_msremains the sole perf ceiling.turn_timeout_sstays dataset-scoped; worker lifecycle timeouts stay onsettings.client.*.Multi-AI review hardening
A Codex review + adversary-council pass found and fixed: watchdog now covers the unbounded metrics drain; watchdog SIGTERMs only the aggregator (event logger kept its buffered
events.jsonl); timed-out runs skip accuracy scoring (artifacts still salvaged); teardown race can no longer skipresult_summary.json;run_auditmaps a watchdog fire toExecutionErrorinstead of the Ctrl-C exit-130 path.Type of change
Testing
tests/unit: 1522 passed, 5 skipped; integration command suites green (incl. fulltest_cli.pye2e)--helpverified aliases unchangedChecklist
🤖 Generated with Claude Code