Skip to content

fix: harden FPM collector completion and failure handling - #379

Draft
Arsene12358 wants to merge 3 commits into
mainfrom
fix/fpm-finalization-followup-20261003
Draft

Arsene12358 wants to merge 3 commits into
mainfrom
fix/fpm-finalization-followup-20261003

Conversation

@Arsene12358

@Arsene12358 Arsene12358 commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Why and what changed

Newer Dynamo self-benchmark writes a worker-probe sidecar alongside its rank timing files. AISimulate could treat that sidecar as a rank measurement. On full grids, it could also stop the engines after rank files appeared but before local merged results and the advertised probe finished writing. Separately, a later Slurm node could fail during startup while the collector waited on an earlier node’s unfinished future.

Classify probe sidecars separately and wait for correctly bound local merged/probe artifacts before teardown. A separate 120-second finalization window starts after local rank measurements complete, does not reset during polling, and remains capped by the collection deadline. Recorded partial or failed probe observations remain visible to downstream validation. Observe Slurm node futures in completion order so the first observed failure promptly triggers existing cancellation of owned child processes and preserves both nodes’ logs and the original failure.

Review map

  • Risk level: medium.
  • Start with collector/fpm_forward/runtime/fpm_exec.sh for lifecycle ordering and artifact binding, then collector/fpm_forward/native_artifact.py for rank-file discovery and collector/fpm_forward/slurm.py for failure propagation.
  • Public or serialized contract changed: optional FPM_FINALIZATION_TIMEOUT_SECONDS override, default 120 and range 1–3600 seconds. Existing artifact schemas are unchanged.
  • Compatibility or rollback concern: older producers whose merged artifacts do not advertise a probe remain supported. No stored timing data needs migration.

Evidence

  • Tests: 394 collector tests passed on 8bc44d6d4993efc0ecf1260acb507aa57821ca0d, independently rerun by task and whole-branch reviewers. Coverage includes runtime wrapper, artifact parsing, runner, Slurm, and measurement evidence.
  • Fast CI: passed on 8bc44d6d4993efc0ecf1260acb507aa57821ca0d.
  • Full CI: not run locally.
  • CodeRabbit reviewed commit: none; automatic review was skipped because this is a draft.
  • Codex reviewed commit: 8bc44d6d4993efc0ecf1260acb507aa57821ca0d; independent task and fresh whole-branch reviews, no findings.
  • Negative or boundary cases: delayed merged/probe output, both local DP completion orders, stale or mismatched bindings, invalid timeout overrides, separate finalization timeout, collection-deadline cap, legacy producers, partial probe observations, a failed second node while the first remains alive, retained stdout/stderr, and existing interruption/admission races. Ruff lint/format, shell syntax, and whitespace checks passed.
  • Expected-value derivation: artifact fixtures follow the actual producer schema and bind to their known run, grid, mode, paths, and local source ranks. Pending/complete assertions follow those producer states.
  • Before/after: preserved full-grid artifacts contain all eight ranks' completed measurements but lack advertised probes. The old checker returned complete; the new checker returns finalization-pending. Four complete prefill/decode smoke shards still pass. A fresh eight-GPU GB300 prefill/decode smoke with the patched collector completed successfully; the larger replication pilot was cancelled after an engine-startup port bind failure and produced no measurements. Full-grid replication with the patched collector remains unvalidated.

Reproduce the collector test subset from python/aisimulate (the timeout plugin is disabled for macOS):

PYTHONPATH=src:. PYTHONDONTWRITEBYTECODE=1 python -m pytest -p no:timeout -p no:cacheprovider -o addopts= -q tests/unit/collector/test_fpm_exec.py tests/unit/collector/test_fpm_runtime_wrapper.py tests/unit/collector/test_fpm_worker_probe_artifact.py tests/unit/collector/test_fpm_runner.py tests/unit/collector/test_fpm_slurm.py tests/unit/collector/test_fpm_measurement_evidence.py

Modeling or data provenance

This changes collector control flow and artifact classification. Validation uses synthetic boundary fixtures plus preserved GB300 DEP8 artifacts from pinned vLLM 0.27.0 and current Dynamo PR source revisions. Timing values, collection grids, aggregation, and quality thresholds are unchanged. Probe-file completion does not establish measurement quality or worker-restoration success. Slurm failure handling cancels owned child commands; it does not cancel the caller’s allocation.

Tracking

Signed-off-by: Yiming Liu <yimingl@nvidia.com>
Keep each collector engine alive until its local merged benchmark artifact and any advertised worker-probe sidecar are recorded. Bind completion artifacts to the run and local DP shard, retain legacy producers without sidecars, and preserve recorded partial or failed probe evidence for quality validation.

Bound post-rank finalization independently of the collection budget, preserve raw timings on failure, and cover delayed merge/probe output, both two-node completion orders, stale artifacts, and deadline failures. Validation: 350 collector tests passed; independently rechecked against failed full-grid and successful P/D smoke artifacts.

Signed-off-by: Yiming Liu <yimingl@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Oct 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Observe failed node futures immediately so existing command-scope cancellation stops live peer steps before pool shutdown. Preserve the original error and every node's output logs.

Add a real-process regression with a failing second node and live first node, plus multi-node success coverage. All 67 Slurm unit tests and Ruff checks pass.

Signed-off-by: Yiming Liu <yimingl@nvidia.com>
@Arsene12358 Arsene12358 changed the title fix: preserve FPM artifacts before collector teardown fix: harden FPM collector completion and failure handling Oct 3, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant