Synchronize with submitit Slurmexecutor python arg addition - #2830
Conversation
facebook-github-bot
added
the
CLA Signed
|
This would be an amazing addition. I cannot use the Submitit plugin without it on our cluster. |
lauritsf
left a comment
lauritsf
left a comment
There was a problem hiding this comment.
This contribution is of high value to people working on larger HPC clusters, where the preferred approach is to use singularity containers.
The naming and default match the existing init keys.
omry
left a comment
omry
left a comment
There was a problem hiding this comment.
Thanks for the PR. This looks like a useful addition, but it needs a bit more before I can take it:
- Add tests covering the new
hydra.launcher.python/slurm_pythonbehavior. - Add a plugin news fragment for
hydra-submitit-launcher.
Please also make sure the implementation handles the Submitit version requirement cleanly, since Hydra currently allows older Submitit versions that may not support this constructor argument.
Require Submitit 1.4.6 and verify that hydra.launcher.python is forwarded to Submitit as slurm_python. Document the launcher option and add the plugin news fragment. Closes facebookresearch#3309.
|
Thanks @berombau for the original implementation. I’ve taken over finalizing this PR while preserving your two implementation commits and attribution. I completed the remaining review work tracked in issue #3309: required Submitit 1.4.6, added structured configuration coverage for forwarding The plugin tests, minimum-version tests, lint checks, and full Hydra core suite pass. |
I took over this PR to complete the remaining work. Commit 3cfc07b addresses the requested tests, news fragment, Submitit minimum version, and documentation.
|
@codex, comments? |
|
To use Codex here, create an environment for this repo. |
5 participants
Motivation
Synchronizing with now merged submitit change (facebookincubator/submitit#1729) that allows to pass in a "python" variable to SlurmExecutor.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
This minimal added optional variable works for my setup.
Related Issues and PRs
The previous PR #2654 does the same, but the PR was closed before it could be merged.