feat(config): consolidate all time knobs into settings.timeouts; make --timeout a real run watchdog - #409
feat(config): consolidate all time knobs into settings.timeouts; make --timeout a real run watchdog#409viraatc wants to merge 9 commits into
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
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.
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>
| - `--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
There was a problem hiding this comment.
Do you mean if both are set, both will be omitted?
this originally was is both are set, raise validation error - nothing is run.
if neither is set, we would run 1 dataset epoch.
if any is set, that value would be used
Can we drop duration
+1, dropped: duration and min_duration_ms so no longer applies.
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.
| @@ -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?
There was a problem hiding this comment.
The intent was to shrink schema.py (~1700 lines and growing) and refocus it on what it's actually for: the top-level aggregates — BenchmarkConfig and EndpointConfig — plus the cross-field validation that has to see every domain at once. Everything else moved out along one rule: one module per config domain, every name (model, enum, helper) beside its owner.
▏ ┌─────────────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
▏ │ file │ contains │
▏ ├─────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
▏ │ schema.py │ BenchmarkConfig/EndpointConfig (root aggregates), cross-field validation, root-level TestType/TestMode, and the re-export hub — existing config.schema imports unchanged │
▏ ├─────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
▏ │ settings.py │ the settings: block: Settings, RuntimeConfig, LoadPattern(+LoadPatternType), WarmupConfig, ProfilingConfig(+ProfilerEngine), EarlyStoppingConfig │
▏ ├─────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
▏ │ timeouts.py │ Timeouts — every global wait/deadline (settings.timeouts) │
▏ ├─────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
▏ │ model_params.py │ ModelParams(+StreamingMode), OSLDistribution(+OSLDistributionType), SubmissionReference │
▏ ├─────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
▏ │ datasets.py │ Dataset(+DatasetType), AccuracyConfig(+EvalMethod/ScorerMethod), AgenticInferenceConfig, the generation-override merge helpers │
▏ ├─────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
▏ │ audit.py │ AuditTestId, OutputCachingTestConfig (the audit: block) │
▏ └─────────────────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
| @@ -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?
…s; make --timeout a real run watchdog Reworked from PR #409 review feedback, rebuilt on latest main: - New frozen Timeouts model at settings.timeouts holds every give-up deadline: run_timeout_s (--timeout, whole-run watchdog), service-ready, per-phase drains (absorbs DrainConfig), metrics drain (0-sentinel killed; None = unlimited), and the worker lifecycle waits (moved off settings.client; carriers renamed *_s, excluded from dumps and CLI). - --timeout was consumed nowhere; it now aborts the run: session.stop() then SIGTERM the aggregator (INTERRUPTED final snapshot, first-wins), ExecutionError after finalization - a fired watchdog can never yield a COMPLETE result_summary.json. Deadline is captured before setup; the timer stays armed through the metrics drain. Timed-out runs skip accuracy scoring; audit phases map a fired watchdog to ExecutionError. - publish_final serialized with an asyncio.Lock: a SIGTERM racing the ENDED-driven finalize can no longer abandon a half-written snapshot. - runtime.min_duration_ms/--duration deleted: sample count is explicit (--num-samples) or the dataset issued once. max_duration_ms stays in runtime as the perf-phase workload cap (int|None, gt 0); reaching it is a normal end. MLPerf ruleset path (RuntimeSettings/UserConfig) keeps its internal duration fields. - ServiceLauncher.terminate(module): exact-match SIGTERM; MetricsPipeline.terminate_metrics_aggregator() is the narrow public face. - config/schema.py split into enums/audit/model_params/datasets/settings/ timeouts modules; schema.py keeps the root aggregate + re-export hub. SystemDefaults and TEMPLATE_TYPE_MAP deleted. - Examples, templates, and docs migrated; docs gain a YAML<->CLI time-knob table. Stale inert timeout: values dropped, warmup drain removed from examples. Breaking: bare configs (no --num-samples) now run the dataset once instead of deriving QPS x 10min samples; old YAML keys hard-error via extra=forbid.
d6ac8ac to
02fca9d
Compare
…s; make --timeout a real run watchdog Reworked from PR #409 review feedback, rebuilt on latest main: - New frozen Timeouts model at settings.timeouts holds every give-up deadline: run_timeout_s (--timeout, whole-run watchdog), service-ready, per-phase drains (absorbs DrainConfig), metrics drain (0-sentinel killed; None = unlimited), and the worker lifecycle waits (moved off settings.client; carriers renamed *_s, excluded from dumps and CLI). - --timeout was consumed nowhere; it now aborts the run: session.stop() then SIGTERM the aggregator (INTERRUPTED final snapshot, first-wins), ExecutionError after finalization - a fired watchdog can never yield a COMPLETE result_summary.json. Deadline is captured before setup; the timer stays armed through the metrics drain. Timed-out runs skip accuracy scoring; audit phases map a fired watchdog to ExecutionError. - publish_final serialized with an asyncio.Lock: a SIGTERM racing the ENDED-driven finalize can no longer abandon a half-written snapshot. - runtime.min_duration_ms/--duration deleted: sample count is explicit (--num-samples) or the dataset issued once. max_duration_ms stays in runtime as the perf-phase workload cap (int|None, gt 0); reaching it is a normal end. MLPerf ruleset path (RuntimeSettings/UserConfig) keeps its internal duration fields. - ServiceLauncher.terminate(module): exact-match SIGTERM; MetricsPipeline.terminate_metrics_aggregator() is the narrow public face. - config/schema.py split into enums/audit/model_params/datasets/settings/ timeouts modules; schema.py keeps the root aggregate + re-export hub. SystemDefaults and TEMPLATE_TYPE_MAP deleted. - Examples, templates, and docs migrated; docs gain a YAML<->CLI time-knob table. Stale inert timeout: values dropped, warmup drain removed from examples. Breaking: bare configs (no --num-samples) now run the dataset once instead of deriving QPS x 10min samples; old YAML keys hard-error via extra=forbid.
02fca9d to
0ff8514
Compare
…s; make --timeout a real run watchdog Reworked from PR #409 review feedback, rebuilt on latest main: - New frozen Timeouts model at settings.timeouts holds every give-up deadline: run_timeout_s (--timeout, whole-run watchdog), service-ready, per-phase drains (absorbs DrainConfig), metrics drain (0-sentinel killed; None = unlimited), and the worker lifecycle waits (moved off settings.client; carriers renamed *_s, excluded from dumps and CLI). - --timeout was consumed nowhere; it now aborts the run: session.stop() then SIGTERM the aggregator (INTERRUPTED final snapshot, first-wins), ExecutionError after finalization - a fired watchdog can never yield a COMPLETE result_summary.json. Deadline is captured before setup; the timer stays armed through the metrics drain. Timed-out runs skip accuracy scoring; audit phases map a fired watchdog to ExecutionError. - publish_final serialized with an asyncio.Lock: a SIGTERM racing the ENDED-driven finalize can no longer abandon a half-written snapshot. - runtime.min_duration_ms/--duration deleted: sample count is explicit (--num-samples) or the dataset issued once. max_duration_ms stays in runtime as the perf-phase workload cap (int|None, gt 0); reaching it is a normal end. MLPerf ruleset path (RuntimeSettings/UserConfig) keeps its internal duration fields. - ServiceLauncher.terminate(module): exact-match SIGTERM; MetricsPipeline.terminate_metrics_aggregator() is the narrow public face. - config/schema.py split into enums/audit/model_params/datasets/settings/ timeouts modules; schema.py keeps the root aggregate + re-export hub. SystemDefaults and TEMPLATE_TYPE_MAP deleted. - Examples, templates, and docs migrated; docs gain a YAML<->CLI time-knob table. Stale inert timeout: values dropped, warmup drain removed from examples. Breaking: bare configs (no --num-samples) now run the dataset once instead of deriving QPS x 10min samples; old YAML keys hard-error via extra=forbid.
28ca7a6 to
5db7537
Compare
b6165a1 to
7935df4
Compare
…s; make --timeout a real run watchdog Reworked from PR #409 review feedback, rebuilt on latest main: - New frozen Timeouts model at settings.timeouts holds every give-up deadline: run_timeout_s (--timeout, whole-run watchdog), service-ready, per-phase drains (absorbs DrainConfig), metrics drain (0-sentinel killed; None = unlimited), and the worker lifecycle waits (moved off settings.client; carriers renamed *_s, excluded from dumps and CLI). - --timeout was consumed nowhere; it now aborts the run: session.stop() then SIGTERM the aggregator (INTERRUPTED final snapshot, first-wins), ExecutionError after finalization - a fired watchdog can never yield a COMPLETE result_summary.json. Deadline is captured before setup; the timer stays armed through the metrics drain. Timed-out runs skip accuracy scoring; audit phases map a fired watchdog to ExecutionError. - publish_final serialized with an asyncio.Lock: a SIGTERM racing the ENDED-driven finalize can no longer abandon a half-written snapshot. - runtime.min_duration_ms/--duration deleted: sample count is explicit (--num-samples) or the dataset issued once. max_duration_ms stays in runtime as the perf-phase workload cap (int|None, gt 0); reaching it is a normal end. MLPerf ruleset path (RuntimeSettings/UserConfig) keeps its internal duration fields. - ServiceLauncher.terminate(module): exact-match SIGTERM; MetricsPipeline.terminate_metrics_aggregator() is the narrow public face. - config/schema.py split into enums/audit/model_params/datasets/settings/ timeouts modules; schema.py keeps the root aggregate + re-export hub. SystemDefaults and TEMPLATE_TYPE_MAP deleted. - Examples, templates, and docs migrated; docs gain a YAML<->CLI time-knob table. Stale inert timeout: values dropped, warmup drain removed from examples. Breaking: bare configs (no --num-samples) now run the dataset once instead of deriving QPS x 10min samples; old YAML keys hard-error via extra=forbid.
|
Thanks everyone for the detailed review and discussions! this PR ships the global-timeout half and consolidates the per-phase drains as flat, consistently named knobs in one block. Full phase modularization (explicit phase types, dependencies, per-phase config blocks) is a bigger structural change — filed #449 to track it. The MR ready to finalize |
…s; make --timeout a real run watchdog Reworked from PR #409 review feedback, rebuilt on latest main: - New frozen Timeouts model at settings.timeouts holds every give-up deadline: run_timeout_s (--timeout, whole-run watchdog), service-ready, per-phase drains (absorbs DrainConfig), metrics drain (0-sentinel killed; None = unlimited), and the worker lifecycle waits (moved off settings.client; carriers renamed *_s, excluded from dumps and CLI). - --timeout was consumed nowhere; it now aborts the run: session.stop() then SIGTERM the aggregator (INTERRUPTED final snapshot, first-wins), ExecutionError after finalization - a fired watchdog can never yield a COMPLETE result_summary.json. Deadline is captured before setup; the timer stays armed through the metrics drain. Timed-out runs skip accuracy scoring; audit phases map a fired watchdog to ExecutionError. - publish_final serialized with an asyncio.Lock: a SIGTERM racing the ENDED-driven finalize can no longer abandon a half-written snapshot. - runtime.min_duration_ms/--duration deleted: sample count is explicit (--num-samples) or the dataset issued once. max_duration_ms stays in runtime as the perf-phase workload cap (int|None, gt 0); reaching it is a normal end. MLPerf ruleset path (RuntimeSettings/UserConfig) keeps its internal duration fields. - ServiceLauncher.terminate(module): exact-match SIGTERM; MetricsPipeline.terminate_metrics_aggregator() is the narrow public face. - config/schema.py split into enums/audit/model_params/datasets/settings/ timeouts modules; schema.py keeps the root aggregate + re-export hub. SystemDefaults and TEMPLATE_TYPE_MAP deleted. - Examples, templates, and docs migrated; docs gain a YAML<->CLI time-knob table. Stale inert timeout: values dropped, warmup drain removed from examples. Breaking: bare configs (no --num-samples) now run the dataset once instead of deriving QPS x 10min samples; old YAML keys hard-error via extra=forbid.
…ding tokenization An expired metrics_drain_timeout_s finalizes the aggregator as COMPLETE with n_pending_tasks > 0; previously that exited 0 with complete: false buried in result_summary.json. run_benchmark now raises ExecutionError after the artifacts are written, so partial ISL/OSL/TPOT stats can never look like a clean run. (The audit path already refused to certify these.)
Every enum had exactly one consumer module, so the kind-based enums.py bought no cycle-breaking and no sharing: LoadPatternType/ProfilerEngine now live in settings.py beside LoadPattern/ProfilingConfig, OSLDistributionType/StreamingMode in model_params.py, DatasetType/EvalMethod/ScorerMethod in datasets.py, and the root-level TestType/TestMode in schema.py. The split criterion is now uniform: one module per config domain, every name beside its owner, schema.py = root aggregate + cross-domain validation + re-export hub (import sites unchanged).
…mple Examples set only non-default overrides; the null drain deadlines equal the schema defaults.
The three endpoint-client worker waits (init, graceful shutdown, force kill) are client internals, not global run deadlines — restore them on HTTPClientConfig under their original names and drop them from Timeouts. CLI_QUICK_REFERENCE gains a run-lifetime timeline visualizing where every time knob acts.
test_metrics_preflight_tap.py references a local scratchpad path and test_protocol.py targets transport features that do not exist on main; neither belongs to this PR.
One object owns the whole-run deadline: timer handle, fired flag, and the late-bound session, replacing the nonlocal flag + mutable-holder closure threaded through _run_benchmark_async. Behavior unchanged.
Fix references left behind by the consolidation: renamed drain knob in session.py docstring, argv-vs-schema 0-sentinel wording in the aggregator snapshot/help text, config module pointers after the schema split, the from-config flag surface in CLI_QUICK_REFERENCE, regenerate-templates trigger lists, the config DESIGN nested-model table, and the compliance plan's duration-floor note. schema.py re-exports HTTPClientConfig again.
2a4e153 to
7a49605
Compare
What does this PR do?
Unifies every global time knob into one place — the frozen
Timeoutsmodel in the newsrc/inference_endpoint/config/timeouts.py, mounted atsettings.timeouts— gives the previously dead--timeoutflag real whole-run-watchdog semantics, deletes the--durationknob, and splits the ~1700-lineconfig/schema.pyinto focused domain modules.The headline bug this fixes
BenchmarkConfig.timeout(--timeout, top-leveltimeout:YAML key) was consumed nowhere — a silent no-op. It is nowsettings.timeouts.run_timeout_s: a real whole-run watchdog whose deadline is captured atrun_benchmarkentry (setup counts against it) and stays armed through every phase and the metrics drain. When it fires: the session stops (ENDED still flows, buffered tokenizer-drain samples are recorded), the metrics aggregator is SIGTERMed (its handler writes an INTERRUPTEDfinal_snapshot.json), scoring is skipped, andrun_benchmarkraisesExecutionErrorafter artifacts are written — a fired watchdog can never yieldcomplete: trueand always exits non-zero. Locked bytests/integration/commands/test_run_timeout.py.The one block
Deleted outright (hard cutover,
extra=forbidmakes stale YAML keys error loudly): top-leveltimeout:, thesettings.drainblock (fields absorbed intotimeoutswith consistent*_drain_timeout_snames),settings.service_ready_timeout_s(moved),runtime.min_duration_ms/--duration, and every0 = unlimitedsentinel (gt=0; unlimited isnull).docs/CLI_QUICK_REFERENCE.md gains a run-lifetime timeline showing where every knob acts, a YAML-path <-> CLI-flag table, and composition rules. All CLI aliases unchanged.
Sample count: explicit or dataset-once (breaking)
--duration/min_duration_msis deleted (review consensus in the duration thread): the sample count is explicitruntime.n_samples_to_issue(--num-samples) or, when omitted, one pass over the dataset.target_qps x 10 minworth of samples; it now runs the dataset once. Example YAMLs that relied on derivation carry explicit counts. The MLPerf ruleset path (RuntimeSettings/UserConfig) keeps its internal duration fields, so submission validity rules are unaffected.Metrics drain gets teeth
An expired
metrics_drain_timeout_s(aggregator finalizes COMPLETE withn_pending_tasks > 0) previously exited 0 withcomplete: falseburied in the summary. It now fails the run: artifacts are written first, thenExecutionError. Partial ISL/OSL/TPOT stats can never look like a clean run. (The audit path already refused to certify these.)schema.py split
config/schema.pynow owns only the root aggregates (BenchmarkConfig/EndpointConfig), cross-field validation, the root-levelTestType/TestMode, and the re-export hub (import sites unchanged). Domains moved tosettings.py,timeouts.py,model_params.py,datasets.py,audit.py— each module owns its models AND enums (no shared enums file; every enum had exactly one consumer). Dead code deleted:SystemDefaults,TEMPLATE_TYPE_MAP.Design invariants
run_timeout_snever derives per-stage deadlines; it is the only total-wall-time bound.max_duration_msandperformance_drain_timeout_snever run concurrently: the cap bounds issuing and skips the drain; the drain bounds post-issuing waiting after a natural end.settings.client(unchanged from main).turn_timeout_sstays dataset-scoped.Robustness fixes found during review hardening
publish_finalis serialized with anasyncio.Lock: a watchdog SIGTERM racing the ENDED-driven finalize can no longer abandon a half-writtenfinal_snapshot.json.finalize_benchmarkforcescomplete: falsewhen the aggregator finalized COMPLETE just before the SIGTERM landed — timed-out artifacts are never split-brained.Follow-up filed: #449 (promote warmup to a first-class phase type with per-phase config).
Type of change
Testing
tests/unit: 1865 passed; integration command suites green--helpverified aliases unchangedChecklist
🤖 Generated with Claude Code