Skip to content

feat(config): consolidate all time knobs into settings.timeouts; make --timeout a real run watchdog - #409

Open
viraatc wants to merge 45 commits into
mainfrom
timeouts-consolidation
Open

feat(config): consolidate all time knobs into settings.timeouts; make --timeout a real run watchdog#409
viraatc wants to merge 45 commits into
mainfrom
timeouts-consolidation

Conversation

@viraatc

@viraatc viraatc commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Consolidates every global time knob into settings.timeouts and makes --timeout a real whole-run watchdog.

settings:
  runtime:
    min_duration_ms: null            # ONLY allowed for num-samples-to-issue calculation in POISSON mode 
    max_duration_ms: null            # workload cap: bounds perf-phase ISSUING; normal end, valid report
  timeouts:                          # every global wait/deadline; null = unlimited (0 never means unlimited;
                                     # drain budgets accept an explicit 0 = give up immediately)
    run_timeout_s: null              # --timeout; whole-run watchdog -> INTERRUPTED artifacts + non-zero exit
    service_ready_timeout_s: 30.0    # metrics/event-logger startup
    warmup_drain_timeout_s: 240.0    # per-phase post-issuing drains
    performance_drain_timeout_s: null
    accuracy_drain_timeout_s: null
    metrics_drain_timeout_s: null    # tokenization drain; expiry FAILS the run (complete: false + non-zero exit)
  client:
    worker_initialization_timeout: 60.0   # endpoint-client internals stay on client (unchanged)

Changes

  • --timeout / top-level timeout: was consumed nowhere (silent no-op) -> now settings.timeouts.run_timeout_s, a whole-run watchdog armed from setup through the metrics drain; firing writes INTERRUPTED artifacts, skips scoring, exits non-zero.
  • settings.drain block, flat settings.service_ready_timeout_s, and every 0 = unlimited sentinel deleted; hard cutover (extra=forbid), no back-compat shims; templates/examples/docs/tests migrated. One convention everywhere: null = unlimited; drain budgets additionally accept an honest 0 (zero budget, give up immediately); run_timeout_s rejects 0.
  • min_duration_ms/max_duration_ms are workload durations on settings.runtime, not timeouts. The --duration alias is deleted — --runtime.min-duration-ms is the one spelling (poisson only, requires an explicit target_qps; explicit --num-samples wins; both unset = one dataset pass). The MLPerf ruleset path keeps its internal duration fields.
  • Expired metrics_drain_timeout_s now fails the run instead of exiting 0 with partial ISL/OSL buried in the summary; artifacts written first.
  • Ctrl-C is part of the same contract, with one behavior: a single process-level SIGINT handler (SigintGovernor, shared by compliance-audit runs) stops the session gracefully — the aggregator drains, artifacts land state: interrupted / complete: false, events.jsonl flushed, exit 130. Teardown is bounded by a fixed 30 s grace: if the metrics drain has not finished, the service children are SIGTERMed (the aggregator writes a best-effort INTERRUPTED snapshot) then SIGKILLed — a wedged drain can never hang the abort, and no second keystroke is needed. Repeat ^C is a logged no-op, so runners that forward the terminal's group SIGINT (uv run delivers a single ^C twice) need no special handling. Escalation is timeout-driven, not keystroke-driven (design cue: aiperf).
  • Every ^C delivery window is defined: during setup (immediate abort, exit 130, no artifacts), mid-run (graceful), during the metrics drain (graceful, grace-bounded), during sync finalization (raises immediately — never silently swallowed), between audit phases (refuses to start the next phase), and during post-measurement accuracy scoring (the report is rewritten interrupted/complete: false before it is persisted — an interrupted run is an invalid run; its artifacts only expose the partial metrics).
  • Artifact precedence, documented and tested: result_summary.json + the exit code are the run-level truth; metrics/final_snapshot.json records what the aggregator itself observed and can legitimately read state: complete when the abort lands after the session's terminal ENDED (post-ENDED drain window). The aggregator ignores SIGINT (the parent's ENDED path is authoritative); INTERRUPTED is entered via the session's marker event or SIGTERM (run watchdog / teardown grace).
  • A run whose measurement was aborted never exits 0 and its result_summary.json is never complete: true (split-brain rewrite guard keyed on report.state, covering the drain-timeout subcase too).
  • Docs: run-lifetime timeline of every knob, YAML<->CLI table, Ctrl-C contract + artifact precedence in CLI_QUICK_REFERENCE.md; AGENTS.md aggregator lifecycle updated. CLI aliases otherwise unchanged (only --duration deleted, above).
  • Tests: watchdog e2e (mid-run / pre-session / during-drain fire, no-fire), real-subprocess CLI SIGINT (graceful mid-run, grace expiry against a wedged drain, pre-session, one-keystroke-under-uv run, drain-window artifact divergence — exit 130, honest artifacts, no orphans), governor unit suite (graceful + grace arming/expiry/disarm, repeat no-op, no-live-run raise), drain-timeout failure, finalization-window regression, config validation + removed-key tripwires.

Exit codes (interrupt/timeout contract)

exit meaning artifacts
0 clean run state: complete, complete: true
2 / 3 input validation / setup error none
4 ExecutionError: watchdog (run_timeout_s) fired, metrics_drain_timeout_s expired, or session aborted (e.g. transport closure) written first; state: interrupted or complete: false
130 user ^C (graceful; teardown bounded by a 30 s grace) full interrupted set incl. events.jsonl; grace expiry: whatever was already written + best-effort interrupted snapshot

Invariant: an aborted run never exits 0 and never ships a complete: true summary — an interrupted run is an invalid run; its artifacts exist only to expose the partial metrics. One documented, tested edge: final_snapshot.json may read state: complete for a post-ENDED-drain-window abort (the summary stays authoritative).

Follow-ups: #449 (promote warmup to a first-class phase type), #459 (consolidate run-outcome state flags into one abort/outcome model), profiling lifecycle hardening (explicit line-profiler shutdown instead of atexit; non-blocking profile POSTs) as a separate PR after this merges. schema.py module split also deferred to a follow-up MR.

Type of change

  • Bug fix
  • New feature
  • Refactor/cleanup

Testing

  • Tests added/updated
  • Full unit suite + interrupt/timeout integration suites pass locally
  • Manual testing completed (live watchdog + ^C runs at every phase window, incl. real uv run keystroke forwarding)

Checklist

  • Code follows project style
  • Pre-commit hooks pass

@viraatc
viraatc requested a review from a team July 13, 2026 20:20
@github-actions

Copy link
Copy Markdown

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@github-actions
github-actions Bot requested review from arekay-nv and nvzhihanj July 13, 2026 20:21

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

viraatc added a commit that referenced this pull request Jul 13, 2026
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>
Comment thread src/inference_endpoint/config/model_params.py Fixed
@viraatc viraatc changed the title feat(config): consolidate all durations and deadlines into one Timeouts model DRAFT: feat(config): consolidate all durations and deadlines into one Timeouts model Jul 13, 2026
@viraatc
viraatc marked this pull request as draft July 13, 2026 21:30
Comment thread docs/CLI_QUICK_REFERENCE.md Outdated
Comment thread docs/CLI_QUICK_REFERENCE.md Outdated
Comment thread docs/LOCAL_TESTING.md Outdated
Comment thread examples/02_ServerBenchmarking/offline_llama3_8b_cnn.yaml Outdated
Comment thread examples/08_Qwen3-VL-235B-A22B_Example/offline_qwen3_vl_235b_a22b_shopify.yaml Outdated

@arekay-nv arekay-nv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the schema breakdown make sense and is a lot cleaner.
Regarding the timeouts - two suggestions, and feedback is welcome:

  1. Remove the duration field - makes it simpler especially since we are mostly going to be doing concurrency based runs.
  2. 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 as warmup always goes before performance, reporting comes after accuracy etc.

Comment thread docs/CLI_QUICK_REFERENCE.md Outdated
Comment thread docs/CLI_QUICK_REFERENCE.md Outdated
Comment thread examples/08_Qwen3-VL-235B-A22B_Example/offline_qwen3_vl_235b_a22b_shopify.yaml Outdated
@arekay-nv
arekay-nv requested a review from roborluo August 6, 2026 19:17
Comment thread docs/CLI_QUICK_REFERENCE.md
Comment thread docs/CLI_QUICK_REFERENCE.md Outdated
Comment thread docs/config/DESIGN.md Outdated
Comment thread examples/03_BenchmarkComparison/compare_with_vllm.py Outdated
Comment thread examples/08_Qwen3-VL-235B-A22B_Example/offline_qwen3_vl_235b_a22b_shopify.yaml Outdated
Comment thread src/inference_endpoint/async_utils/services/launcher.py Outdated
Comment thread src/inference_endpoint/config/templates/offline_template_full.yaml Outdated
Comment thread src/inference_endpoint/config/datasets.py Outdated
Comment thread src/inference_endpoint/commands/benchmark/execute.py Outdated
Comment thread src/inference_endpoint/config/timeouts.py Outdated
@viraatc
viraatc marked this pull request as ready for review August 13, 2026 18:04
@viraatc viraatc changed the title DRAFT: feat(config): consolidate all durations and deadlines into one Timeouts model feat(config): consolidate all durations and deadlines into one Timeouts model Aug 13, 2026
viraatc added a commit that referenced this pull request Aug 13, 2026
…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.
…n spellings

Real-world report: a run with a 2.2M-item tokenization backlog wedged in
the unbounded metrics drain after ^C, and every further ^C was suppressed.
Second distinct ^C now cancels the run task — the unwind kills the service
children (the SIGTERMed aggregator writes a best-effort INTERRUPTED
snapshot without finishing its drain), salvages tmpfs, exits 130.
Re-deliveries within 1s remain one keystroke (uv run & co. forward the
group SIGINT), so graceful ^C keeps working under wrappers.

Cleanups: warmup/accuracy phase settings use the canonical
min_duration_ms=None; the e2e concurrency perf test no longer passes the
poisson-only min-duration flag; the standalone httpclient bench renames
its unrelated --duration to --duration-s.
…euristic

First ^C stops gracefully; every further SIGINT cancels the run task
(teardown/metrics drain abandoned, service children killed, best-effort
INTERRUPTED snapshot, exit 130). No timing state. Known caveat, documented:
wrappers that forward the terminal's group SIGINT (uv run) deliver one
keystroke twice, so a single ^C under them force-quits immediately.
Line-profiler stats are dumped by shutdown() called from the CLI run()
finally, worker_main's finally, and pytest_sessionfinish. The plugin's
stderr-suppression hook is gone too: with the profiler torn down before
interpreter exit there are no shutdown errors to hide.
…review-round fixes

Force-quit (second distinct ^C) now abandons everything without grace:
the pipeline teardown SIGKILLs the service children on every unwind path
(force_quit predicate), HTTP workers are SIGKILLed with no graceful wait,
and the tmpfs salvage is skipped. First ^C keeps the graceful path:
report for whatever is already computed, normal drain budget, exit 130.
Runners that forward the terminal's group SIGINT (uv run) deliver a
single ^C twice, the forwarded copy ~200ms later; deliveries within 1s of
the last accepted one are dropped as duplicates so one keystroke never
force-quits. Locked by integration tests: wedged-aggregator force quit,
pre-session ^C, and a real uv-run one-keystroke run.

The audit runner installs its own SigintGovernor around the phase loop
and passes it to every phase, so a ^C during a long audit phase takes the
graceful path instead of a raw KeyboardInterrupt that skips finalize; a
^C between phases refuses to start another phase.

Review-round fixes: split-brain guard keys on report.state (catches the
drain-timeout subcase of an aborted run); governor binds beside the
watchdog at session creation; SIGINT-handler restore uses a sentinel
(None is a restorable C-installed handler); whole-run deadline derivation
shared via _run_deadline; run_benchmark fails loudly when no usable
report exists; the metrics-drain failure message names the
tokenizer-failure cause; signal frame typed FrameType | None;
run_timeout_s description states the setup-boundary limitation;
tokenizer-workers doc default is 4.
… the loop thread; profiler teardown unconditional

Three correctness holes from review:

1. After run_until_complete returns, the SIGINT governor's
session/task/loop stay bound but the loop is stopped - a first ^C
during sync finalization (or audit inter-phase setup) queued
session.stop on the dead loop and was silently swallowed. The graceful
path now requires a live task on a running loop; otherwise
KeyboardInterrupt is raised immediately (salvage still runs via
run_benchmark's finally, exit 130).

2. ProfileController.start/stop ran sequential blocking urlopen calls
(2s each) on the event-loop thread, so a queued force-quit task.cancel
could not land until every POST finished. The POSTs now run in worker
threads (asyncio.to_thread), awaited sequentially - identical ordering
and records, but cancellation lands between POSTs. The session's
on_phase_start hook is awaitable to carry this, and the perf cap is
armed AFTER the awaited profile arming: _run_phase clears the
phase-stop flag at entry, so a one-shot cap firing during the await
would be erased and the phase would run uncapped.

3. Line-profiler shutdown() early-returned when print_stats() had
already run, and a failing output destination skipped teardown - both
left the C profiler enabled at exit. Teardown now runs unconditionally
in a finally; _stats_printed only suppresses a duplicate dump.

Regression tests: SigintGovernor state machine (no-live-run raise,
duplicate-delivery drop, force cancel), hook awaited before issuing,
cap-shorter-than-hook still bounds the phase, print_stats-then-shutdown
and failing-destination teardown.
…ion matrix

Test-quality pass over the PR's test surface:

- SigintGovernor: replace overlapping named scenario tests with one
exhaustive delivery-sequence matrix (every bind-state x distinct/dup
sequence up to length 3, against a dedicated child task), keeping only
the finished-loop cases the matrix cannot express. PerfPhaseTimeout
tests move next to the governor and run against the real event loop
(cap fires, accuracy start disarms, never-armed cases parametrized,
idempotent cancel) instead of a fake-loop scheduling recorder.
- test_sigint.py: launch/readiness/teardown/leftover-scan boilerplate
extracted into helpers; four distinct scenarios stay.
- test_run_timeout.py: shared _make_config builder + big-prompts
dataset writer replace five hand-rolled config blocks.
- test_schema.py: drop max_duration 0/negative rejection duplicates
(covered parametrized in test_timeouts.py, same validator).
- test_benchmark.py: audit-dispatch tests get a shared config-double
helper and hoisted module imports; the two audit-FAIL tests collapse
into one parametrized case; class renamed TestRunBenchmarkAuditDispatch.
- test_profiler.py: file-level unit pytestmark (was invisible to
-m unit), enabled_profiler fixture replaces repeated env/singleton
setup, singleton restored after every test; drop the conditional no-op
prefix test.
- line_profiler._teardown_profiler tolerates an already-disabled
profiler (line_profiler raises ValueError on double sys.monitoring
release) so shutdown teardown is truly unconditional.
…dow divergence

nv-alicheng's review flagged that a SIGINT reaching only the main process
can leave final_snapshot.json COMPLETE while result_summary.json is
corrected to INTERRUPTED. This is deliberate - the snapshot is the
aggregator's own observation, the summary is run-level truth - but the
contract was undocumented and the divergence window untested, and three
docs still claimed a ^C never leaves COMPLETE artifacts.

- CLI_QUICK_REFERENCE, AGENTS.md, snapshot.py SessionState docstring, and
the test_sigint module contract now state the precedence: result_summary
+ exit code are authoritative; final_snapshot can legitimately read
state=complete when the abort lands in the post-ENDED metrics-drain
window (and the aggregator ignores SIGINT - the parent's ENDED path is
authoritative for ^C, with SIGTERM/marker-event as the INTERRUPTED
entries).
- New integration test pins the window deterministically: session ends,
tokenization backlog drains, ^C to the main process only -> exit 130,
summary state=interrupted complete=false, snapshot state=complete, no
orphaned children.
- execute.py's phase-start hook extracted to _make_phase_start_hook and
its profiler-before-perf-cap ordering asserted directly (a production
reorder now fails the test); the timing-based session test that
re-implemented the ordering locally is deleted.
A ^C landing during post-measurement accuracy scoring (run task done,
governor raises KeyboardInterrupt mid-score_accuracy) exits 130, and
finalize_benchmark's finally still writes the genuinely completed perf
report as complete:true. That is the honest outcome - the measurement
finished; only post-measurement scoring was aborted. Documented as the
one exception to 'a ^C'd run's summary is never complete' in
CLI_QUICK_REFERENCE, the finalize comment, and the sigint test module
contract; pinned by a unit regression (score_accuracy raising
KeyboardInterrupt -> summary complete:true, interrupt propagates).
Shared complete-report builder extracted for the two finalize guards.
Single import style for the cli module (string-target monkeypatch, _run
imported directly); explicit awaited cancellation via wait_for in the
governor matrix teardown.
@viraatc
viraatc force-pushed the timeouts-consolidation branch from a71a187 to bffa94d Compare August 20, 2026 22:09
…rdown grace

Simplification pass modeled on aiperf's interrupt design: a ^C has one
meaning. It stops the session gracefully and arms a 30s teardown grace;
if the metrics drain has not finished when the grace expires, the
service children are SIGTERMed (the aggregator's handler writes a
best-effort INTERRUPTED snapshot) then SIGKILLed, so a wedged drain can
never hang the abort - escalation is timeout-driven, not
keystroke-driven. Repeat ^C is a logged no-op, which also makes
group-SIGINT-forwarding runners (uv run) need no special handling.

Deleted with the second-^C force-quit semantics: the duplicate-delivery
window and forced state in SigintGovernor, the force_quit predicate and
kill_now fast path in MetricsPipeline, the forced-quit exception branch
in _run_benchmark_async, and http_client.kill_workers /
worker_manager.kill_now (endpoint_client files are back to main
verbatim; main's worker teardown is already bounded).

Also deferred to a follow-up (files back to main verbatim): the
line-profiler explicit-shutdown refactor and the threaded profile POSTs
- with no force-cancel to unblock, the sync POSTs are fine again, and
the session's on_phase_start hook returns to a plain sync callback
(no await window, so the perf-cap arming order is no longer load-bearing).

A ^C during post-measurement accuracy scoring now rewrites the report
to interrupted/complete:false before it is persisted (an interrupted
run is an invalid run; artifacts only expose the partial metrics) and
the interrupt propagates for exit 130.

Tests follow the model: governor suite covers graceful+grace arming,
repeat no-op, grace expiry, disarm-on-drain, and the no-live-run raise;
the wedged-drain integration test now needs only a single ^C (grace
shrunk to 3s via the class constant); the uv one-keystroke and
drain-window divergence tests are unchanged in spirit.
…GINT restore

Two holes found by post-simplification review:

1. Grace expiry SIGKILLs a wedged aggregator, so the report is built
from the subscriber's last LIVE pub/sub snapshot - and the split-brain
guard only rewrote state=="complete", letting an aborted run persist
result_summary.json with state:"live". The guard now rewrites any
aborted non-interrupted state; the wedge integration test asserts the
summary lands interrupted/complete:false and the unit guard test is
parametrized over both snapshot states.

2. signal.signal(SIGINT, None) raises TypeError, so the sentinel-based
restore was broken for exactly the case it existed for (a C-installed
previous handler, getsignal()->None). Restoring SIG_DFL instead would
destroy the host's handler, so the governor now probes getsignal()
first and refuses to install over an unrepresentable C handler (stays
passive, same stance as off-main-thread); the restore path only ever
sees Python-representable handlers. The _SIGINT_NOT_INSTALLED sentinel,
its object-typed local, and the type:ignore are gone (both run_benchmark
and run_audit).
…edence, shard reaping

Council findings (two parallel reviewers, addressed locally):

Correctness:
- finalize_benchmark writes the split-brain-rewritten report back onto
BenchmarkResult, so the audit runner's post-phase state check sees the
honest interrupted state instead of the aggregator's stale COMPLETE (a
drain-window ^C during the final audit phase could previously certify
a PASS).
- run_benchmark refreshes user_interrupted from the governor after the
loop returns: a ^C landing between the coroutine's flag snapshot and
task completion can no longer produce COMPLETE artifacts on an exit-130
run.
- A drain/report failure on an interrupted run no longer re-raises as
ExecutionError (exit 4): the user's abort outranks a drain error its
own grace escalation may have caused - exit stays 130.
- The finalization KeyboardInterrupt handler now covers every
post-measurement write (scoring, summary log, profiling.json,
accuracy_results), rewriting and RE-persisting the summary as
interrupted if it was already written COMPLETE.
- Tokenizer shard workers arm PR_SET_PDEATHSIG=SIGKILL in their
initializer: a SIGKILLed aggregator (watchdog / teardown-grace
escalation) can no longer orphan the non-daemon ProcessPool shards
that are outside every launcher PID list. Behavioral test reads
PR_GET_PDEATHSIG back from a real pool worker.
- SIGINT install/restore is one shared context manager
(watchdog.sigint_policy) used by run_benchmark and run_audit: probes
getsignal() first (an unrepresentable C handler stays untouched,
governor passive), restores after the caller's finally so a repeat ^C
during tmpfs salvage still hits the governor's no-op.

Quality: _on_global_timeout renamed _on_perf_phase_timeout (it only
caps the perf phase), Timeouts docstring no longer promises None for
service_ready_timeout_s, sigint test helpers deduplicate the /proc
scan and name timeout_s, grace-expiry unit test asserts exactly one
fire, timeouts validation tests pin the failing field (extra=forbid
would mask typos), dict annotations typed.
Comment thread tests/unit/commands/test_watchdog.py Dismissed
Comment thread tests/unit/commands/test_watchdog.py Fixed

def test_restores_on_exception(self):
gov = SigintGovernor()
prev = signal.getsignal(signal.SIGINT)
Cut the whole-finalize KeyboardInterrupt re-persist wrapper (guarded a
millisecond window between artifact writes; the exit code is the
documented run-outcome verdict) and the tokenizer-shard pdeathsig guard
(pre-existing risk - main's terminate_all already SIGKILLs - and CF
workers self-reap on queue EOF; follow-up material). The scoring-window
invalidation, split-brain write-back, user_interrupted refresh, and
exit-130 precedence stay: each enforces the documented contract in
1-3 lines.
…s a config knob

Size pass: PerfPhaseTimeout returns to its original execute.py location
(verbatim, as _PerfPhaseTimeout) and SigintGovernor/sigint_policy/
RunWatchdog live beside it - the separate watchdog.py module double-
counted the moved code in the diff. Multi-line rationale comments
compressed to their load-bearing core.

The teardown grace is now settings.timeouts.teardown_grace_s
(default 30; None = never abandon; 0 = abandon immediately) consumed by
both the ^C governor and the run watchdog; the wedged-drain integration
test shrinks it via YAML instead of a python -c constant override. The
five per-drain CLI alias flags are dropped - the auto-generated dotted
flags remain and the docs table now shows those spellings; --timeout
stays.

Non-test churn: 1694 -> 1514.
Comment thread src/inference_endpoint/config/schema.py Outdated
Comment thread src/inference_endpoint/config/schema.py Outdated
Comment thread src/inference_endpoint/config/schema.py Outdated
Comment thread src/inference_endpoint/commands/benchmark/execute.py Outdated
Comment thread src/inference_endpoint/commands/benchmark/execute.py Outdated
) from e
raise
finally:
watchdog.cancel()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[review-council: Claude+Codex+Grok] medium — accuracy scoring / finalize is not covered by run_timeout_s; a hung external scorer hangs an otherwise-clean run. RunWatchdog is cancelled here (watchdog.cancel()), then run_benchmark calls finalize_benchmarkscore_accuracyscorer.score() with no live deadline. For SKIP_ENDPOINT_PHASE scorers that delegates whole agent execution + grading to an external service (SWE-bench swebench_service_url), and VBench/LCB spawn uv run subprocesses — any can block indefinitely on a clean run, and only a manual ^C escapes. Acknowledged in CLI_QUICK_REFERENCE.md:166 ("not deadline-bounded") but contradicted by this field's own docstring (schema.py:838 "through every phase and drain") and CLI_QUICK_REFERENCE.md:188 ("the only total-wall-time bound"). Either bound scoring with a dedicated scorer timeout, or reconcile the three statements.

Priority:
1. If `n_samples_to_issue` is set, return it (explicit override)
2. If min_duration_ms=0, return all dataset samples (new CLI default)
2. If no duration target is set, return all dataset samples

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: min_duration to make clear where it should be (duration now applies to

Every bare 'duration' the MR added is now explicit: the sizing input is
min_duration_ms (poisson-only, target_qps x min_duration_ms, sizing not
a timer), the runtime cap is max_duration_ms, and the give-up deadline
is run_timeout_s. total_samples_to_issue's priority docstring names the
actual rule per step; offline/concurrency runs are called out as purely
count-driven (min_duration_ms with them is a config error).
The grace applies exactly to runs being marked INTERRUPTED (^C or the
run watchdog); the name now says so. Config field, constructor params,
docs table, and templates renamed together.
…_duration_ms docstring

The interrupt machinery (SigintGovernor, sigint_policy, RunWatchdog)
returns to commands/benchmark/watchdog.py - the module reviewers
reviewed; the perf-phase cap (_PerfPhaseTimeout) stays at its original
execute.py location. RuntimeSettings.min_duration_ms now documents the
full contract: sizing input (target_qps x min_duration_ms), never a
runtime timer, n_samples_to_issue wins, populated from
settings.runtime.min_duration_ms or directly by rulesets. Reverts the
gratuitous 'duration floor' -> 'min_duration_ms floor' renames in the
compliance docs (the original text was already precise).
… public boundary

run_benchmark_async gives callers without a governor (audit phases get
one from run_audit; embedded/test callers) a passive SigintGovernor -
never installed as a signal handler, interrupted stays False - so
_run_benchmark_async requires it and the five scattered
'sigint is not None' guards collapse into plain attribute access. The
adjacent report-persist guards in finalize merge into one block.

Also restores the compliance-plan min_duration blockquote verbatim from
main (the branch's condensed rewrite had dropped the 'merely derives a
count' explanation; the file is now untouched by this MR).
…run_timeout_s is a hard bound

arekay's review round:

- HIGH: a ^C during service launch / endpoint connect (task bound,
session not yet) previously raised raw KeyboardInterrupt out of the
loop, skipping the pipeline __aexit__ and orphaning the
aggregator/event-logger children. The governor now cancels the run task
- exactly like the watchdog's pre-session fire - so the unwind kills
the children; _run_benchmark_async maps the cancellation back to
KeyboardInterrupt for exit 130. Unit test pins the cancel.

- MED: interrupted_teardown_grace_s: null no longer softens
run_timeout_s - the watchdog's SIGKILL escalation always arms (null
grace applies to the ^C path only, as documented).

- nits: redundant cyclopts help= strings that duplicated the field
description are dropped (cyclopts falls back to the description);
templates regenerated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: P0 Critical — blocks release or users size/very-large PR Review Policy: >1500 lines or >50 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants