Skip to content

fix: log exceptions instead of silently swallowing them in Job/JobGroup.status - #569

Open
andrewwhitecdw wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
andrewwhitecdw:andrewwhitecdw/fix/job-status-exception-swallowing
Open

fix: log exceptions instead of silently swallowing them in Job/JobGroup.status#569
andrewwhitecdw wants to merge 2 commits into
NVIDIA-NeMo:mainfrom
andrewwhitecdw:andrewwhitecdw/fix/job-status-exception-swallowing

Conversation

@andrewwhitecdw

Copy link
Copy Markdown

Bug

Job.status and JobGroup.status used except Exception: ... with a finally: return, which swallowed status-check failures silently and made debugging hard.

Fix

Preserve the existing fallback behavior (return last known state / UNKNOWN) but log the exception at ERROR level so operators can see when the runner is failing.

Test

Updated test_job_status_exception and test_job_group_status_exception to assert the error is logged using caplog.

Verification

uv run pytest test/run/test_job.py::test_job_status_exception test/run/test_job.py::test_job_group_status_exception -v passes. uv run --group lint ruff check ... and ruff format --check ... pass.

…up.status

Job.status and JobGroup.status used  with a

, which swallowed status-check failures silently and made

debugging hard. Preserve the existing fallback behavior (return last known

state / UNKNOWN) but log the exception at ERROR level so operators can see

when the runner is failing.

Updates existing exception tests to assert the error is logged.

Signed-off-by: Andrew White <andrewh@cdw.com>

Copy link
Copy Markdown
Contributor

Review verdict: no blocking findings for #569.

Blocking findings: None.

What I reviewed
nemo_run/run/job.py:96-107Job.status() now logs the active exception and then returns the same last-known-state fallback as before. Moving the return out of finally also avoids suppressing exceptions outside the caught Exception class.
nemo_run/run/job.py:312-330JobGroup.status() logs per-handle failures, preserves UNKNOWN for a failed or missing status, updates self.states, and continues processing remaining handles.
test/run/test_job.py:105-119 and test/run/test_job.py:443-459 — tests cover both fallback results and the exception/message in the nemo_run.run.job log.
• I traced the consumers in nemo_run/run/experiment.py and compared the PR head with current main; the touched files have not diverged on main since the branch's merge base.

Non-blocking follow-up: Experiment.status() calls job.status() twice while building one row (nemo_run/run/experiment.py:913 and :930). A backend failure will therefore now produce two identical ERROR tracebacks per status request. Consider polling once and reusing the result for both the display text and returned dictionary; this is existing duplicate polling, not a correctness regression in this patch.

Checks: DCO passes. The exact head has nine GitHub Actions workflows in action_required with zero jobs, including tests and Ruff lint/format, so automated validation has not run. git diff --check succeeded, but the sandbox has no uv, so I could not independently execute the focused pytest cases. CI should run before merge.

Copy link
Copy Markdown
Contributor

LGTM — #569

Blocking findings: None.

Job.status() (nemo_run/run/job.py:96-107) now logs runner exceptions while preserving the previous last-known-state fallback. JobGroup.status() (nemo_run/run/job.py:312-330) logs failures per handle, substitutes AppState.UNKNOWN, and continues processing the group. Moving both returns out of finally also avoids unintentionally suppressing exceptions outside the caught Exception class.

The updated tests (test/run/test_job.py:105-119 and :443-459) verify both fallback behavior and traceback logging. I reviewed both changed files, their surrounding implementation, and the callers in Experiment.status().

Non-blocking observation: Experiment.status() polls each job twice at nemo_run/run/experiment.py:913 and :930, so one backend failure can now produce two identical error tracebacks. That duplicate polling predates this PR and is not a correctness regression.

Validation: DCO passes. The nine GitHub Actions workflows for the exact head are action_required, so tests and lint have not executed. git diff --check passed; this sandbox lacks uv and Python, so I could not independently run the focused tests.

Copy link
Copy Markdown
Contributor

LGTM — #569

Blocking findings: None.

nemo_run/run/job.py:96-107Job.status() now records runner failures with logger.exception() while preserving the prior last-known-state fallback. Moving the return outside finally also avoids suppressing exceptions outside the caught Exception class.
nemo_run/run/job.py:312-330JobGroup.status() logs failures per handle, substitutes AppState.UNKNOWN, continues checking remaining handles, and updates self.states as before.
test/run/test_job.py:105-119 and :443-459 — the updated tests cover both fallback behavior and the emitted exception/message.
• I reviewed both changed files, surrounding implementation, and callers in nemo_run/run/experiment.py.

Non-blocking observation: Experiment.status() polls each job twice at nemo_run/run/experiment.py:913 and :930, so one backend failure can produce two identical error tracebacks. This duplicate polling predates the PR and is not a correctness regression.

Validation: DCO passes. The exact head has no GitHub Actions runs beyond DCO, so pytest and Ruff have not run. git diff --check passed; the sandbox lacks uv and Python, so I could not independently execute the focused tests. CI should run before merge.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants