docs: requeue #153
docs: requeue #153
Conversation
WalkthroughThe pull request introduces new command line flags for the SLURM executor plugin, specifically Changes
Possibly related PRs
Suggested reviewers
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
🧹 Outside diff range and nitpick comments (4)
docs/further.md (4)
163-170: Minor improvements needed in the new command line flags section.The new section on additional command line flags is informative and well-structured. However, there are a couple of minor issues to address:
- On line 169, "usefull" should be corrected to "useful".
- The sentence on line 165 could be improved for clarity.
Consider applying these changes:
-This plugin defines additional command line flags. As always the can be used on the command line or in a profile. +This plugin defines additional command line flags. As always, they can be used on the command line or in a profile. -| `--slurm_init_seconds_before_status_checks`| will modify the default time (40 seconds) before the initial status check - usefull for development purposes| +| `--slurm_init_seconds_before_status_checks`| will modify the default time (40 seconds) before the initial status check - useful for development purposes|🧰 Tools
🪛 LanguageTool
[typographical] ~165-~165: Consider adding a comma here.
Context: ...fines additional command line flags. As always the can be used on the command line or ...(AS_ALWAYS_COMMA)
[grammar] ~165-~165: Did you mean “they”, “that” or “there”?
Context: ...dditional command line flags. As always the can be used on the command line or in a...(THE_THEY)
Line range hint
215-283: Approve changes to "Using Profiles" section with a minor suggestion.The expanded "Using Profiles" section provides valuable information about configuration options, storage prefixes, and file system usage. The examples for different cluster setups are particularly helpful.
Consider formatting the note about ongoing development differently for better visibility:
-==This is ongoing development. Eventually you will be able to annotate different file access patterns.== +> **Note:** This is ongoing development. Eventually, you will be able to annotate different file access patterns.This change will make the note stand out more clearly as a separate piece of information.
🧰 Tools
🪛 LanguageTool
[style] ~215-~215: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
Line range hint
283-308: Approve changes to "Retries" section with a minor formatting suggestion.The expanded "Retries" section now provides comprehensive information about handling job failures, requeuing, and resuming workflows. The addition of code examples for checking cluster requeue settings and using the
--slurm-requeueflag is particularly helpful.For consistency with the rest of the document, consider using backticks for the
--rerun-incompleteflag in the code example:-snakemake ... --rerun-incomplete +snakemake ... `--rerun-incomplete`This change will maintain a consistent style for command-line flags throughout the documentation.
Line range hint
309-370: Approve changes to "Dynamic Parameterization" section with a minor wording suggestion.The expanded "Dynamic Parameterization" section now provides excellent guidance on dynamically adjusting job parameters, including memory requirements and runtime. The code examples for both Snakefile and workflow profile configurations are clear and illustrative.
Consider rewording the sentence about using sensible settings for improved clarity:
-Be sure to use sensible settings for your cluster and make use of parallel execution (e.g. threads) and [global profiles](#using-profiles) to avoid I/O contention. +Be sure to use appropriate settings for your cluster, utilize parallel execution (e.g., threads), and implement [global profiles](#using-profiles) to optimize performance and avoid I/O contention.This change provides a more specific and actionable recommendation for users.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- docs/further.md (4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/further.md
[typographical] ~165-~165: Consider adding a comma here.
Context: ...fines additional command line flags. As always the can be used on the command line or ...(AS_ALWAYS_COMMA)
[grammar] ~165-~165: Did you mean “they”, “that” or “there”?
Context: ...dditional command line flags. As always the can be used on the command line or in a...(THE_THEY)
[style] ~215-~215: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
🔇 Additional comments (1)
docs/further.md (1)
Line range hint
371-383: Approve the "Summary" section.The "Summary" section effectively concludes the document by providing a concise and well-commented example of a typical command line for using the SLURM executor plugin. It successfully ties together the various concepts and options discussed throughout the documentation.
The command line example is clear, comprehensive, and includes helpful comments, making it an excellent reference for users.
🧰 Tools
🪛 LanguageTool
[typographical] ~165-~165: Consider adding a comma here.
Context: ...fines additional command line flags. As always the can be used on the command line or ...(AS_ALWAYS_COMMA)
[grammar] ~165-~165: Did you mean “they”, “that” or “there”?
Context: ...dditional command line flags. As always the can be used on the command line or in a...(THE_THEY)
[uncategorized] ~174-~174: Possible missing comma found.
Context: ...## Multicluster Support For reasons of scheduling multicluster support is provided by the...(AI_HYDRA_LEO_MISSING_COMMA)
|
Would be nice if one of you would find the time to review. Thank you! |
Co-authored-by: David Laehnemann <david.laehnemann@hhu.de>
Co-authored-by: David Laehnemann <david.laehnemann@hhu.de>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
docs/further.md (4)
163-172: LGTM! Clear and concise presentation of new command line flags.The new section effectively introduces the additional command line flags for the SLURM executor plugin. The table format is appropriate and easy to read.
Consider using a relative link for the "retries" section reference in line 171 to ensure it works correctly across different versions of the documentation. For example:
See the [section "retries" for details](#retries)
216-216: LGTM! Minor wording improvement.The change enhances the clarity of the sentence.
Optional improvement: Consider replacing "prior to" with "before" to make the sentence slightly more concise, as suggested by the static analysis tool. However, the current wording is acceptable in technical documentation.
🧰 Tools
🪛 LanguageTool
[style] ~216-~216: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
295-309: LGTM! Comprehensive update on retry and requeue options.The new information about the "requeue" option and the
--slurm-requeueflag is valuable for users. The added command for checking cluster requeue settings is particularly helpful.For consistency with the rest of the document, consider using backticks (
) to format the--requeue` parameter in line 306:This requeue feature is integrated into the SLURM submission command, adding the `--requeue` parameter to allow requeuing after node failure or preemption using:
284-284: Consider using Markdown-style anchors for better compatibility.While the HTML anchor tag is functional, many Markdown processors automatically generate anchors for headings. Consider using a Markdown-style approach for better compatibility and readability:
## Retries - Or Trying again when a Job failed {#retries}This approach creates an anchor that can be linked to using
[link text](#retries)elsewhere in the document.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- docs/further.md (4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/further.md
[style] ~216-~216: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
🔇 Additional comments (1)
docs/further.md (1)
Line range hint
1-1: Overall, excellent updates to the SLURM executor plugin documentation!The changes provide valuable information about new features, command line flags, and options for the SLURM executor plugin. The additions are well-structured, clear, and informative. The minor suggestions for improvement are aimed at enhancing consistency and following Markdown best practices, but they don't detract from the overall quality of the updates.
Great job on keeping the documentation up-to-date and user-friendly!
🧰 Tools
🪛 LanguageTool
[uncategorized] ~175-~175: Possible missing comma found.
Context: ...## Multicluster Support For reasons of scheduling multicluster support is provided by the...(AI_HYDRA_LEO_MISSING_COMMA)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
docs/further.md (4)
163-172: LGTM! Consider minor formatting adjustment for consistency.The new section on additional command line flags is clear and informative. It provides valuable information about the new features introduced in this version.
For consistency with the rest of the document, consider changing the header to use Title Case:
-### Additional Command Line Flags +### Additional Command Line Flags
216-216: LGTM! Consider minor wording adjustment.The added information clarifies when the module load command is executed, which is helpful for users.
Consider rephrasing to avoid wordiness:
-This will, internally, trigger a `module load bio VinaLC` immediately prior to execution. +This will, internally, trigger a `module load bio VinaLC` just before execution.🧰 Tools
🪛 LanguageTool
[style] ~216-~216: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
295-309: LGTM! Consider adding a brief explanation of preemption.The new information about the requeue option is valuable and well-explained. It provides a clear distinction between Snakemake's
--retriesand SLURM's requeue feature.Consider adding a brief explanation of what preemption means in the context of SLURM, as some users might not be familiar with the term. For example:
-The "requeue" option allows jobs to be resubmitted automatically if they fail or are preempted. +The "requeue" option allows jobs to be resubmitted automatically if they fail or are preempted (i.e., interrupted by higher-priority jobs).
Line range hint
384-391: LGTM! Consider adding a note about-j unlimited.The updated summary section provides a comprehensive example of a typical Snakemake command line with the SLURM executor. The addition of the
--workflow-profileoption and explanatory comments is helpful.Consider adding a brief note explaining the implications of using
-j unlimited:$ snakemake --workflow-profile <path> \ -> -j unlimited \ # assuming an unlimited number of jobs +> -j unlimited \ # allows Snakemake to submit as many jobs as possible, limited by cluster resources > --default-resources slurm_account=<account> slurm_partition=<default partition> \ > --configfile config/config.yaml \ > --directory <path> # assuming a data path not relative to the workflowThis clarification helps users understand that while Snakemake can submit unlimited jobs, the actual number of running jobs will still be limited by the cluster's resources and policies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- docs/further.md (4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/further.md
[style] ~216-~216: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
docs/further.md (4)
163-172: LGTM! Clear and informative addition of new command line flags.The new section on additional command line flags is well-structured and provides valuable information. The table format is clear and concise.
Consider adding a brief explanation of what "requeue" means in the context of SLURM jobs in the table description for
--slurm_requeue. This would provide immediate context for users who might not be familiar with the concept.🧰 Tools
🪛 Markdownlint
171-171: null
Link fragments should be valid(MD051, link-fragments)
216-216: Approved with a minor suggestion for improvement.The addition of "immediately prior to execution" provides more precise timing information, which is helpful.
Consider simplifying "immediately prior to execution" to "just before execution" to make the sentence more concise while maintaining clarity.
🧰 Tools
🪛 LanguageTool
[style] ~216-~216: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
295-309: Excellent addition of information about the requeue option!The new content provides valuable information about SLURM's requeue feature, its comparison with Snakemake's
--retries, and how to use it. The added commands for checking and enabling requeue are particularly helpful.Consider adding a brief explanation of the potential benefits of using
--slurm-requeueover--retries, such as maintaining job priority or any other SLURM-specific advantages. This would help users make an informed decision about which option to use.
Line range hint
1-400: Overall, excellent updates to the documentation!The new additions about command line flags and requeue options integrate seamlessly with the existing content. The document maintains a good balance between technical details and user-friendly explanations, and the use of code blocks and tables enhances readability.
Consider adding a table of contents at the beginning of the document to help users navigate the various sections more easily, especially as new content is added. This would improve the overall user experience and make it easier to find specific information quickly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- docs/further.md (3 hunks)
🧰 Additional context used
🪛 Markdownlint
docs/further.md
171-171: null
Link fragments should be valid(MD051, link-fragments)
🪛 LanguageTool
docs/further.md
[style] ~216-~216: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... amodule load bio VinaLCimmediately prior to execution. Note, that - environment ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
🔇 Additional comments (1)
docs/further.md (1)
171-171: Verify the internal link to the "retries" section.The link fragment
#retriesin the table might not correspond to an existing section in the document. This could result in a broken internal link.Please check if the "retries" section has the correct ID in the Markdown. If needed, update the link or the section ID to ensure proper navigation. You can use the following script to verify the existence of the section:
✅ Verification successful
Internal link to the "retries" section is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the existence of the "retries" section # Test: Search for a heading with "retries" (case-insensitive) if rg -i '^#+\s+.*retries' docs/further.md; then echo "Section 'retries' found." else echo "Section 'retries' not found. Please check the heading or update the link." fiLength of output: 200
🧰 Tools
🪛 Markdownlint
171-171: null
Link fragments should be valid(MD051, link-fragments)
Thank you for your prompt review and the lectoring! |
🤖 I have created a release *beep* *boop* --- ## [0.11.1](v0.11.0...v0.11.1) (2024-10-21) ### Documentation * requeue ([#153](#153)) ([d91ee5f](d91ee5f)) --- 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>
only documenting what is new in v0.11.0 (see #152 )
Summary by CodeRabbit
--slurm_init_seconds_before_status_checksand--slurm_requeue.--retriesand--slurm-requeue.