Fix: Makes worker shutdown timeout configurable in torchrun#172596
Fix: Makes worker shutdown timeout configurable in torchrun#172596efazal wants to merge 2 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/172596
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New Failures, 1 Unrelated FailureAs of commit 6981ecb with merge base 3688cec ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
zou3519
added
the
triaged
efazal
force-pushed
the
fix-torchrun-timeout
branch
2 times, most recently
from
dd64b36 to
8b3ec62
Compare
aorenste
left a comment
aorenste
left a comment
There was a problem hiding this comment.
Sorry - I was out last week. LGTM.
|
I think this has been approved already, do you just need help landing? |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / macos-py3-arm64 / build Details for Dev Infra teamRaised by workflow job |
|
cc @efazal would you mind rebasing the PR? I think that should make the lintfailure go away (pkg_resources not found) The other faillures look unrelated to me. |
efazal
force-pushed
the
fix-torchrun-timeout
branch
from
8b3ec62 to
317ecdc
Compare
|
Hi @jcaip, yes, I just need help in merging this PR. Thanks for your response. I've just rebased the PR. I hope this fixes the lint issue. Please let me know if anything is required from my end. Thanks. |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: Not merging any PRs at the moment because there is a merge blocking https://github.com/pytorch/pytorch/labels/ci:%20sev issue open at: Details for Dev Infra teamRaised by workflow job |
efazal
force-pushed
the
fix-torchrun-timeout
branch
from
317ecdc to
6981ecb
Compare
|
@pytorchbot merge CI failure looks unrelated |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
|
Hi @jcaip, I'm not sure why the pre-merge CI checks keeps failing for this PR. They all seem unrelated to this. I wonder what can be done to resolve this? Please let me know if there is something I can do to resolve. Thank you. |
|
@efazal I don't think it's anything broken in your PR, it looks like a flaky test. Let me try force merging. Sorry about the delay! |
|
@pytorchbot merge -f CI failure looks unrelated, merge was reverted a couple times. |
|
❌ 🤖 pytorchbot command failed: Try |
|
@pytorchbot merge -f "CI failure looks unrelated, merge was reverted a couple times" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
…172596) Fixes [pytorch#119856](pytorch#119856) Previously, torchrun had a hardcoded 30s timeout between SIGTERM and SIGKILL. This was an issue for large model training where checkpoint saving exceeds 30s, k8s env's with longer `terminationGracePeriodSeconds`, and in SLURM preemption scenarios. The hardcoded timeout caused torchrun to send SIGKILL before workers completed graceful shutdown, even when the orchestrator allowed more time. The fix adds a new configurable param "--shutdown-timeout", that takes timeout value via CLI args or falls back to `TORCH_ELASTIC_SHUTDOWN_TIMEOUT` env variable. Defaults to 30s otherwise, as it is currently. Flows through: CLI → LaunchConfig → LocalElasticAgent → SimpleElasticAgent → PContext.close() Added unit tests. Please review and add anyone else who might be relevant @aivanou @Unturned3 @SandishKumarHN Pull Request resolved: pytorch#172596 Approved by: https://github.com/aorenste
6 participants
Fixes #119856
Previously, torchrun had a hardcoded 30s timeout between SIGTERM and SIGKILL. This was an issue for large model training where checkpoint saving exceeds 30s, k8s env's with longer
terminationGracePeriodSeconds, and in SLURM preemption scenarios. The hardcoded timeout caused torchrun to send SIGKILL before workers completed graceful shutdown, even when the orchestrator allowed more time.The fix adds a new configurable param "--shutdown-timeout", that takes timeout value via CLI args or falls back to
TORCH_ELASTIC_SHUTDOWN_TIMEOUTenv variable. Defaults to 30s otherwise, as it is currently.Flows through: CLI → LaunchConfig → LocalElasticAgent → SimpleElasticAgent → PContext.close()
Added unit tests.
Please review and add anyone else who might be relevant @aivanou @Unturned3 @SandishKumarHN