feat: in job stability #137
Conversation
|
Warning Rate limit exceeded@cmeesters has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 43 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe updates include the addition of a utility function, Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- snakemake_executor_plugin_slurm/init.py (2 hunks)
- snakemake_executor_plugin_slurm/utils.py (1 hunks)
Additional comments not posted (1)
snakemake_executor_plugin_slurm/__init__.py (1)
29-30: LGTM! Verify the impact ofdelete_slurm_environment.The import and usage of
delete_slurm_environmentinwarn_on_jobcontextare approved. This change aims to prevent conflicts by cleaning up SLURM-related environment variables.Ensure that this change does not inadvertently affect other parts of the system that rely on SLURM environment variables.
Run the following script to verify the impact of
delete_slurm_environment:Also applies to: 89-94
Verification successful
Verification Successful: No Impact Found from
delete_slurm_environmentThe searches for SLURM-related environment variables returned no results, indicating that there are no critical dependencies on these variables in the codebase. The
delete_slurm_environmentfunction does not inadvertently affect other parts of the system.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of `delete_slurm_environment` on the codebase. # Test: Search for the usage of SLURM-related environment variables. Expect: No critical dependencies on these variables. rg --type python -A 5 $'os.environ.get("SLURM_")'Length of output: 81
Script:
#!/bin/bash # Description: Verify the impact of `delete_slurm_environment` on the codebase. # Test: Search for the usage of SLURM-related environment variables. Expect: No critical dependencies on these variables. rg -A 5 'os.environ.get("SLURM_")'Length of output: 36
Script:
#!/bin/bash # Description: Verify the impact of `delete_slurm_environment` on the codebase by checking for other patterns. # Test: Search for any direct access to SLURM-related environment variables. rg -A 5 'os.environ\["SLURM_"'Length of output: 32
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- snakemake_executor_plugin_slurm/utils.py (1 hunks)
Files skipped from review due to trivial changes (1)
- snakemake_executor_plugin_slurm/utils.py
|
@fgvieira would you have time to review this PR? Alas, we cannot test this within the CI (due to lack of resources). Seems to work fine for me and others. |
|
Why is it not recommended to run |
|
Thing is: The executor exports the environment. This has to be done, because otherwise the base environment might not be present. Within a SLURM job the I am not sure the current PR will fix all issues. I am even not sure, that 100 % stability can be achieved at all - apparently HPC admins made akward configuration a hobby ... But removing the scheduler env variables certainly is a step forward. |
|
So the idea would be to remove all |
|
In fact, you cannot remove environment variables from a parent shell. Hence, python can only remove within the current environment on a node, not its jobscript. Within the Snakemake process on that node, you would not see the If you use (host based) logins to that node, you too do not see these environment variables (because: new shell). Howver, if your run $ srun -A <account> -p <partition> --pty -t 10 bash -i
<node>:<path>$ python3
>>> import os
>>> print(os.environ["SLURM_JOB_ID"])
376803
>>> del os.environ["SLURM_JOB_ID"]
>>> print(os.environ["SLURM_JOB_ID"])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<frozen os>", line 714, in __getitem__
KeyError: 'SLURM_JOB_ID'
>>>
<node>:<path>$ echo $SLURM_JOB_ID
376803you will see, that you are able to tinker with env vars within your process, not in the parent shell. |
🤖 I have created a release *beep* *boop* --- ## [0.10.0](v0.9.0...v0.10.0) (2024-08-23) ### Features * in job stability ([#137](#137)) ([c27f5f8](c27f5f8)) ### Bug Fixes * add --parsable to sbatch call for a more robust output parsing ([#125](#125)) ([5e41d05](5e41d05)) * issue [#109](#109) - preemption is no longer considered a failed status ([#132](#132)) ([6dad273](6dad273)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced the `--parsable` option to the `sbatch` command for improved output parsing. - **Bug Fixes** - Revised job preemption handling to prevent it from being marked as a failed status, enhancing job stability. - **Chores** - Updated the version number of the Snakemake Executor Plugin for Slurm to 0.10.0 in preparation for the release. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
might increase stability of in-job submissions, might fix #113
Summary by CodeRabbit
New Features
Bug Fixes