Skip to content

fix: rescue lingering finished SWE episodes and mask infra failures - #2410

Open
tdene wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
tdene:tde/swe_rescue_and_masking
Open

fix: rescue lingering finished SWE episodes and mask infra failures#2410
tdene wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
tdene:tde/swe_rescue_and_masking

Conversation

@tdene

@tdene tdene commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

This PR contains two fixes.

For the first, the harness will commonly be left running despite the model having finished the episode. This usually happens due to lingering commands. Currently this leads to a long timeout and failure. Rescuing these episodes from the harness allows them to complete early and retrieve rewards for what would otherwise be marked as a failure.

For the second, infra failures are now tagged so that the training backend can choose to mask out their gradients instead of training with them as 0 reward.

@tdene

tdene commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

SHIP — solid, well-tested reliability fix. One NOTE inline; nothing blocking.

The core change is correct: _finish_container_command now polls the OpenHands process instead of a single blocking communicate(), so a finished-but-lingering container (a shell command the model left running) is rescued and scored on its valid output files rather than burning the full timeout and being reported as a failure. What I verified:

  • Timeout semantics preserved. The deadline-based loop and the remaining <= 0raise TimeoutError reproduce the old wall-clock behavior; the min(_CONTAINER_COMMAND_POLL_INTERVAL_S, remaining) bound prevents overshooting the deadline. Non-agent modes fall through to continue unchanged.
  • Rescue path is safe. terminated_after_completion correctly suppresses the SIGKILL-induced non-zero return code at the returncode != 0 check (2556), so a rescued episode isn't misreported as a command failure. Output completeness is gated on full-file json.loads, so a mid-flush partial file waits out the grace window instead of being killed early.
  • Fail-loud on empty output. Marker-seen-but-no-output after the grace raises rather than hanging — the right call.
  • Masking plumbing is consistent. mask_sample/failure_reason are set on the infra-failure path and OR'd into the downstream mask decision in responses(), then persisted via metrics_to_update. Reward stays 0.0 and mask_sample propagates into SWEBenchVerifyResponse (covered by the updated wrapper test).
  • Tests exercise all three branches (missing/complete/late output) with monkeypatched intervals — good coverage of the new logic.

No async-HTTP, ray.get-in-async, or public-API concerns. The one NOTE (inline) is that this new infra-failure path stamps failure_reason = "agent_command_failure" even for timeouts/OOMs, and the downstream if not failure_reason: guard then won't refine it — so the more specific agent_timeout/agent_oom reasons this PR adds are shadowed for those cases. Telemetry granularity only; masking is unaffected.

Comment thread responses_api_agents/swe_agents/app.py Outdated
# An agent command that died without producing output is an infrastructure failure,
# not a model failure; allow for it to be masked out of the gradient.
metrics.mask_sample = True
metrics.failure_reason = "agent_command_failure"

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.

NOTE — failure_reason taxonomy is coarsened for timeouts/OOMs that flow through this path.

WHAT: This except block catches every agent-command failure, including wall-clock timeouts (ValueError("Command timed out")) and watchdog OOMs (the OOM RuntimeError at ~2550). It unconditionally sets metrics.failure_reason = "agent_command_failure" and persists it. Downstream in responses() (~3653), the specific-reason logic is guarded by if not failure_reason:, so it never overwrites — a genuine agent timeout now reports agent_command_failure instead of agent_timeout, and an agent OOM instead of agent_oom.

BLAST RADIUS: Observability only — mask_sample is still correctly True, and agent_timed_out/oom_killed bools remain set, so masking and the boolean signals are unaffected. But the failure_reason taxonomy this PR introduces is partly self-defeating for the two most common failure modes.

FIX: Either set the reason more specifically here (compute after agent_timed_out/watchdog stats), or set it to None/leave unset and let the downstream if not failure_reason: block classify it. Your call — telemetry granularity, not correctness.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! Fixed.

@tdene

tdene commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test bbc003e

tdene and others added 2 commits August 25, 2026 03:39
Co-Authored-By: Jorge Albericio <jalbericiola@nvidia.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
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