Make the SLURM Preemption/Timeout Signal Configurable #14626

Queuecumber · 2022-09-09T16:44:59Z

What does this PR do?

This PR adds a flag to the SLURMEnvironment class which allows users to change which signal will be watched for preemtion/timeout.

Although SIGUSR1 is fairly standard, some libraries (e.g., submitit) which submit to SLURM use a different signal or users may have another compatibility issue with SIGUSR1 and therefore require the use of another signal.

Fixes #14610

Does your PR introduce any breaking changes? If yes, please list them.

None

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

Queuecumber · 2022-09-10T14:37:34Z

@awaelchli This is ready for review, I have two failing tests but I'm not sure whats causing it

Besides the test cases I added I also confirmed that it works on our SLURM cluster with a real job

Queuecumber · 2022-09-10T14:41:46Z

One thing to discuss:

The way I implemented this if the user specifies an invalid signal, we emit a warning message and the SignalConnector falls back to using SIGUSR1

This could also be a fatal error of some sort (either exception and crash or error log and no signal listening)

awaelchli

Overall approach and implementation looks good to me. Minor comments. Thanks for this contribution, and the testing on the cluster!

docs/source-pytorch/clouds/cluster_advanced.rst

src/pytorch_lightning/plugins/environments/slurm_environment.py

src/pytorch_lightning/trainer/connectors/signal_connector.py

awaelchli · 2022-09-10T16:30:06Z

The way I implemented this if the user specifies an invalid signal, we emit a warning message and the SignalConnector falls back to using SIGUSR1

I think we should immediately check this in the init of SLURMEnvironment. Did you choose a warning for a reason? I.e., long waiting job in the queue and erroring out can be annoying to the user? If there is a concern, I think that's fine. Otherwise I would error out, as warnings could be easily overlooked or lost in logs.

awaelchli · 2022-09-10T16:33:29Z

I have two failing tests but I'm not sure whats causing it

Will look at it once we have resolved the discussions here. Probably a difference in the signal library between Python versions.

Queuecumber · 2022-09-11T01:27:13Z

The way I implemented this if the user specifies an invalid signal, we emit a warning message and the SignalConnector falls back to using SIGUSR1

I think we should immediately check this in the init of SLURMEnvironment. Did you choose a warning for a reason? I.e., long waiting job in the queue and erroring out can be annoying to the user? If there is a concern, I think that's fine. Otherwise I would error out, as warnings could be easily overlooked or lost in logs.

I'm fine with erroring out, I think this is probably safer since a user passing an invalid signal would likely not want SIGUSR1 anyway so we probably shouldnt assume it.

Although forcing them to pass an actual signal instead of a string sort of solves this problem for us

tests/tests_pytorch/trainer/connectors/test_signal_connector.py

docs/source-pytorch/clouds/cluster_advanced.rst

tests/tests_pytorch/trainer/connectors/test_signal_connector.py

Signed-off-by: Max Ehrlich <[email protected]>

Co-authored-by: Adrian Wälchli <[email protected]>

Signed-off-by: Max Ehrlich <[email protected]>

Queuecumber · 2022-09-12T13:19:38Z

This is rebased on latest master and ready to go unless there is more feedback

src/pytorch_lightning/trainer/connectors/signal_connector.py

Borda · 2022-09-12T17:21:44Z

@awaelchli seems to be ready... :)

Co-authored-by: Jirka Borovec <[email protected]>

Queuecumber · 2022-09-12T17:57:22Z

Just for my own curiosity can you tell me what those changes are for?

awaelchli · 2022-09-12T18:02:56Z

Earlier I promised I would take care of the failing Windows tests: https://github.com/Lightning-AI/lightning/actions/runs/3039218876/jobs/4893893890

Which is what I'm doing now. Otherwise we can't merge with failing tests.

Queuecumber · 2022-09-12T18:05:36Z

Ok I understand now, because python on windows doesn't even define those constants for the signals

awaelchli · 2022-09-12T19:26:45Z

@Queuecumber Looks like we got the Windows cases figured out.
Great work, thanks for contributing this improvement!

Queuecumber · 2022-09-12T19:26:55Z

Thanks for merging, do you guys know what the ETA is for it showing up in an installable package (pre-release or full release)

I'm eager to use this in semi-production

awaelchli · 2022-09-12T19:29:51Z

This feature will go in to 1.8, we are targeting end of September / beginning October.

To use this immediately, you can install the build from the master branch:
pip install https://github.com/Lightning-AI/lightning/archive/refs/heads/master.zip -U

Queuecumber requested review from tchaton, carmocca, Borda, awaelchli, justusschock, williamFalcon, kaushikb11 and rohitgr7 as code owners September 9, 2022 16:45

github-actions bot added the pl Generic label for PyTorch Lightning package label Sep 9, 2022

Queuecumber marked this pull request as draft September 9, 2022 16:46

Queuecumber changed the title ~~Draft: Make the SLURM Preemption/Timeout Signal Configurable~~ Make the SLURM Preemption/Timeout Signal Configurable Sep 9, 2022

Queuecumber marked this pull request as ready for review September 10, 2022 14:36

Queuecumber requested review from otaj, edenlightning and Felonious-Spellfire as code owners September 10, 2022 14:36

awaelchli reviewed Sep 10, 2022

View reviewed changes

carmocca approved these changes Sep 11, 2022

View reviewed changes

tests/tests_pytorch/trainer/connectors/test_signal_connector.py Outdated Show resolved Hide resolved

carmocca added feature Is an improvement or enhancement environment: slurm labels Sep 11, 2022

carmocca added this to the pl:1.8 milestone Sep 11, 2022

carmocca added the community This PR is from the community label Sep 11, 2022

awaelchli approved these changes Sep 12, 2022

View reviewed changes

docs/source-pytorch/clouds/cluster_advanced.rst Outdated Show resolved Hide resolved

mergify bot added the ready PRs ready to be merged label Sep 12, 2022

awaelchli reviewed Sep 12, 2022

View reviewed changes

tests/tests_pytorch/trainer/connectors/test_signal_connector.py Outdated Show resolved Hide resolved

mergify bot added the has conflicts label Sep 12, 2022

Queuecumber and others added 7 commits September 12, 2022 09:17

Change environment signal to be a real signal

1b4f5ac

Signed-off-by: Max Ehrlich <[email protected]>

Simplified logic by using a real signal object in the environment class

e71a113

Signed-off-by: Max Ehrlich <[email protected]>

Updated docs

06b6ef3

Signed-off-by: Max Ehrlich <[email protected]>

Update tests with new format

2460055

Signed-off-by: Max Ehrlich <[email protected]>

Remove mostly duplicated test

2583017

Signed-off-by: Max Ehrlich <[email protected]>

Fix incorrect tuple in test case

a0eca33

Co-authored-by: Adrian Wälchli <[email protected]>

Move documentation of new flag

91e69a9

Signed-off-by: Max Ehrlich <[email protected]>

Queuecumber force-pushed the master branch from fdb03b2 to 91e69a9 Compare September 12, 2022 13:19

mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Sep 12, 2022

Borda enabled auto-merge (squash) September 12, 2022 17:15

Merge branch 'master' into master

da5cb59

Borda approved these changes Sep 12, 2022

View reviewed changes

src/pytorch_lightning/trainer/connectors/signal_connector.py Outdated Show resolved Hide resolved

awaelchli and others added 2 commits September 12, 2022 19:40

handle windows not supporting sigusr1

3f77bde

Update src/pytorch_lightning/trainer/connectors/signal_connector.py

cac6893

Co-authored-by: Jirka Borovec <[email protected]>

fix broken GH suggestion

d9dd583

skip access to SIGUSR1 on windows in test

2abe4a8

rohitgr7 approved these changes Sep 12, 2022

View reviewed changes

Borda merged commit e5998e6 into Lightning-AI:master Sep 12, 2022

Queuecumber mentioned this pull request Sep 26, 2022

Lightning sends SIGTERM when using other SLURM manager #14893

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the SLURM Preemption/Timeout Signal Configurable #14626

Make the SLURM Preemption/Timeout Signal Configurable #14626

Queuecumber commented Sep 9, 2022 •

edited

Loading

Queuecumber commented Sep 10, 2022 •

edited

Loading

Queuecumber commented Sep 10, 2022 •

edited

Loading

awaelchli left a comment

awaelchli commented Sep 10, 2022

awaelchli commented Sep 10, 2022

Queuecumber commented Sep 11, 2022

Queuecumber commented Sep 12, 2022

Borda commented Sep 12, 2022

Queuecumber commented Sep 12, 2022

awaelchli commented Sep 12, 2022

Queuecumber commented Sep 12, 2022

awaelchli commented Sep 12, 2022

Queuecumber commented Sep 12, 2022

awaelchli commented Sep 12, 2022

Make the SLURM Preemption/Timeout Signal Configurable #14626

Make the SLURM Preemption/Timeout Signal Configurable #14626

Conversation

Queuecumber commented Sep 9, 2022 • edited Loading

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Queuecumber commented Sep 10, 2022 • edited Loading

Queuecumber commented Sep 10, 2022 • edited Loading

awaelchli left a comment

Choose a reason for hiding this comment

awaelchli commented Sep 10, 2022

awaelchli commented Sep 10, 2022

Queuecumber commented Sep 11, 2022

Queuecumber commented Sep 12, 2022

Borda commented Sep 12, 2022

Queuecumber commented Sep 12, 2022

awaelchli commented Sep 12, 2022

Queuecumber commented Sep 12, 2022

awaelchli commented Sep 12, 2022

Queuecumber commented Sep 12, 2022

awaelchli commented Sep 12, 2022

Queuecumber commented Sep 9, 2022 •

edited

Loading

Queuecumber commented Sep 10, 2022 •

edited

Loading

Queuecumber commented Sep 10, 2022 •

edited

Loading