feat: added requeue option to client #136
Conversation
WalkthroughThe recent changes introduce a Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/init.py (2 hunks)
Additional comments not posted (2)
snakemake_executor_plugin_slurm/__init__.py (2)
43-54: LGTM! Therequeuefield is well-defined.The
requeuefield is correctly added with a default value ofFalseand includes helpful metadata for documentation.
157-158: LGTM! The--requeueflag integration is correct.The
run_jobmethod correctly appends the--requeueflag based on therequeuesetting, enhancing job management flexibility.
|
@johanneskoester I requested your review, because locally testworkflows run just fine and our change yields valid Python types, where the CI thinks a None-Type is given. Ideas? |
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/init.py (2 hunks)
Additional comments not posted (2)
snakemake_executor_plugin_slurm/__init__.py (2)
45-56: LGTM!The code changes are approved. The new
requeuefield is correctly added to theExecutorSettingsclass with appropriate metadata.
160-162: LGTM!The code changes are approved. The
--requeueargument is correctly appended to thesbatchcommand based on therequeuesetting.To verify the impact of the
requeuesetting on the job execution process, please run the following script:Verification successful
Verification Successful
The code correctly appends the
--requeueflag to thesbatchcommand when therequeuesetting is enabled. This implementation is verified and aligns with the expected behavior.
- Location:
snakemake_executor_plugin_slurm/__init__.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the `requeue` setting on the job execution process. # Test: Search for the `--requeue` flag in the `sbatch` command. Expect: The flag is present only when the `requeue` setting is enabled. rg --type python -A 5 $'if self.settings.requeue:\n\s+call \+= " --requeue"'Length of output: 268
Script:
#!/bin/bash # Description: Verify the impact of the `requeue` setting on the job execution process. # Test: Search for the `--requeue` flag in the `sbatch` command. Expect: The flag is present only when the `requeue` setting is enabled. rg --type python --multiline -A 5 $'if self.settings.requeue:\n\s+call \+= " --requeue"'Length of output: 119
Script:
#!/bin/bash # Description: Verify the impact of the `requeue` setting on the job execution process. # Test: Search for the `--requeue` flag in the `sbatch` command. Expect: The flag is present only when the `requeue` setting is enabled. rg --multiline -g '*.py' -A 5 $'if self.settings.requeue:\n\s+call \+= " --requeue"'Length of output: 604
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/init.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_slurm/init.py
|
The tests still fail - while working locally, the CI claims
|
|
ah, wanted to look into this CI issue - turns out, I cannot find this branch. WTF??? |
…tings are put into the workflow)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
Line range hint
45-160: Consider adding a test for the newrequeuefeatureThe implementation of the new
requeueoption looks good and addresses the PR objective. To ensure the feature works as expected and to prevent potential regressions in the future, consider adding a test case that verifies the correct behavior of therequeueoption.This test could:
- Create a mock
ExecutorSettingsinstance withrequeue=True.- Call the
run_jobmethod with these settings.- Assert that the generated
sbatchcommand includes the--requeueoption.Adding such a test would increase the robustness of this new feature.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- snakemake_executor_plugin_slurm/init.py (2 hunks)
🔇 Additional comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
45-56: LGTM: Newrequeueoption added correctlyThe new
requeueboolean field in theExecutorSettingsclass is well-implemented. It follows the existing pattern for other fields, has a clear help text explaining its purpose and effect, and uses a default value ofFalseto ensure backward compatibility.
158-160: LGTM:requeueoption integrated correctlyThe
requeueoption is properly integrated into therun_jobmethod. It correctly checks the workflow settings before adding the--requeueoption to thesbatchcommand. The placement and implementation of this new feature are appropriate and align with the existing code structure.
Line range hint
1-1000: Summary: Newrequeuefeature implemented correctlyThe changes in this file successfully implement the new
requeuefeature for the SLURM executor plugin. The implementation is correct, minimal, and consistent with the existing codebase. It addresses the PR objective of allowing users to specify the SBATCH requeue flag.Key points:
- The
requeueoption is added to theExecutorSettingsclass with appropriate metadata.- The
run_jobmethod is updated to include the--requeueflag when the option is enabled.- The changes are backward-compatible and do not introduce any apparent issues.
A suggestion has been made to add a test for this new feature to ensure its correct behavior and prevent future regressions.
Note: The CI issues mentioned in the PR comments do not appear to be directly related to these changes and may require separate investigation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/tests.py (3)
8-15: LGTM: TestWorkflows class changes are appropriate.The renaming of the class from
TestWorkflowsBasetoTestWorkflowsand the update toget_executor_settingsmethod are consistent with the PR objectives. The method now returns a concreteExecutorSettingsinstance, which is an improvement over returningNone.Consider adding a comment explaining why the default
ExecutorSettings()is used here, to provide context for future developers.
18-20: LGTM: TestWorkflowsRequeue class is well-implemented.The new
TestWorkflowsRequeueclass correctly inherits fromTestWorkflowsand overrides theget_executor_settingsmethod to test the new requeue functionality. This aligns well with the PR objectives.Consider adding a docstring to the
TestWorkflowsRequeueclass explaining its purpose and how it differs from the baseTestWorkflowsclass. This would improve code documentation and make the intent clearer for future developers.
Line range hint
1-20: Consider expanding test coverage for requeue functionality.The changes in this file provide a good foundation for testing the new requeue option. However, to ensure comprehensive coverage of the new functionality, consider adding the following:
- Test cases that specifically verify the behavior when requeue is set to True vs False.
- Edge cases, such as testing with multiple job submissions or with different job statuses (e.g., preempted, failed).
- Integration tests that verify the correct flags are passed to the SLURM scheduler when requeue is enabled.
These additional tests would help ensure the robustness of the new feature and catch potential issues early in the development process.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- tests/tests.py (1 hunks)
🔇 Additional comments (1)
tests/tests.py (1)
5-5: LGTM: Import statement for ExecutorSettings is correct.The import of
ExecutorSettingsfromsnakemake_executor_plugin_slurmis necessary for the changes in theget_executor_settingsmethods and is placed correctly in the import section.
🤖 I have created a release *beep* *boop* --- ## [0.11.0](v0.10.2...v0.11.0) (2024-09-27) ### Features * added requeue option to client ([#136](#136)) ([b0ff160](b0ff160)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
I see. Cool! Thanks! |
added option for users to indicate the sbatch requeue flag if the cluster configuration does no re-queuing on default
this is a boolean flag. We wait for the interface_plugin to be fixed.
Summary by CodeRabbit
--requeueflag to the job submission command for enhanced control over job handling.