fix(trainer): don't report Complete when a container status is Unknown - #788
Conversation
aggregate_status_from_containers() filtered Unknown out of the set before
checking for completion, so ["Complete", "Unknown"] aggregated to Complete.
Unknown means "not determined", not "no signal to weigh": get_container_status()
returns it whenever the adapter raises during inspect, and
container_status_to_trainjob_status() falls through to it for paused, dead and
restarting. Dropping it from the check treated an undetermined container as if
it agreed with the others, so on a two-node job where node-0 exited 0 and node-1
could not be inspected, wait_for_job_status(status={"Complete"}) returned on that
poll with node-1's outcome never determined.
Check every status instead. The empty-list guard from kubeflow#562 is preserved, and
Failed and Running still short-circuit first, so ["Created", "Unknown"] remains
Created and the only behavior that changes is the mixed Complete case.
Fixes kubeflow#787
Signed-off-by: brightyorcerf <thetejaansh@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Thanks for the update. I tested PR #788 locally. All 696 tests passed, including 56 container backend tests, and Ruff checks passed. The status aggregation fix looks correct from my testing. |
appreciate your help! |
2 participants
What this PR does / why we need it:
aggregate_status_from_containers()filteredUnknownout of the status set before checking for completion, so["Complete", "Unknown"]aggregated toComplete.Unknownisn't an absent signal —get_container_status()returns it whenever the adapter raises during inspect, andcontainer_status_to_trainjob_status()falls through to it for paused/dead/restarting. Filtering it out treated an undetermined container as if it agreed with the others: on a two-node job where node-0 exited 0 and node-1 couldn't be inspected,wait_for_job_status(status={"Complete"})returned on that poll with node-1's outcome never determined.This checks every status instead. Scoped to
Completeonly — the empty-list guard from #562 is preserved,FailedandRunningstill short-circuit above it, and["Created", "Unknown"]still returnsCreated. Enumerating every multiset of{Failed, Running, Complete, Created, Unknown}up to size 4: 120 combinations unchanged, 6 changed, all of them sets drawn from{Complete, Unknown}with at least one of each.Follow-up to #562
One note on cross-backend consistency, since this makes the container backend stricter than its siblings:
kubernetesreads the TrainJob CR's conditions (kubernetes/backend.py:764-772) so an unreadable pod never blocksCompletethere, andlocalprocessfalls through toCOMPLETEfor anything unrecognized (localprocess/backend.py:268-281). The container backend is the only one that synthesizes job status from raw container inspects, so it's the only one where "couldn't determine" is a real signal rather than an artifact of a controller that already decided. Happy to align the others in a follow-up if you'd prefer uniformity.Which issue(s) this PR fixes:
Fixes #787
Verification:
uv run pytest kubeflow/ -q→ 696 passed;ruff check/ruff format --checkclean.wait_for_job_statuscase is a real regression test, verified both ways: revertingaggregate_status_from_containersto its currentmainbody fails it, with the fix it passes.MockContainerAdapteronly.Checklist: