docs: review and edit new docs #237
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 2 minutes and 40 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. 📒 Files selected for processing (1)
WalkthroughThis update revises the documentation for the Snakemake SLURM executor plugin, expanding and reorganizing sections on installation, resource specification, retry mechanisms, and advanced usage. Additionally, the GitHub Actions workflow for Mastodon release announcements is updated to use a newer version of the Changes
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
…-executor-plugin-slurm into docs/review-new-docs
|
@dlaehnemann please mark the PR as 'ready for review' once done. |
|
Jup, but currently I'm still working on this. Hope to get through it today! |
|
@dlaehnemann hi, updated the branch a couple of times. So far, only minor updates. I would like to update main from this branch soon-ish as some work needs documentation. (I'm on it.) Yet, I understand that your changes are substantial. |
…TODOs for clarification
…on for regular jobs, as this is covered in the where to do configuration section and the main snakemake docs
…seful for a user -- just explain special cases MPI and GPU below
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
docs/further.md (4)
17-23: Ensure consistency in list punctuation
In the Contributions list, consider adding an Oxford comma for clarity and ensuring bullet items consistently end with a period:- We welcome bug reports, feature requests and pull requests! + We welcome bug reports, feature requests, and pull requests!Also, add a trailing period to each repository bullet:
* [`snakemake`](https://github.com/snakemake/snakemake) itself.🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...(UNLIKELY_OPENING_PUNCTUATION)
30-39: Shell example: move comments off continuation lines
Placing comments after the backslash can be confusing in shell usage. It’s safer to have the backslash at the very end, then comment on the next line or inline without breaking the continuation. For example:$ snakemake --executor slurm \ -> -j unlimited \ # assuming an unlimited number of jobs + -j unlimited \ # assuming an unlimited number of jobsAlternatively:
$ snakemake --executor slurm \ -j unlimited \ # assuming an unlimited number of jobs
83-86: Add missing standard resource:mem_mb_per_cpu
mem_mb_per_cpuis a standard Snakemake resource that maps to SLURM's--mem-per-cpu. Consider adding it for completeness:| `runtime` | the walltime per job in minutes | `--time` | +| `mem_mb_per_cpu` | memory per CPU in MB | `--mem-per-cpu` |
658-658: Link to issue tracker for interactive-job edge case
Consider hyperlinking “issues on the plugin's GitHub repository” directly to the issues page for convenience:- report the specific problems as issues on the plugin's GitHub repository. + report the specific problems as [issues on the plugin's GitHub repository](https://github.com/snakemake/snakemake-executor-plugin-slurm/issues).🧰 Tools
🪛 LanguageTool
[uncategorized] ~658-~658: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/further.md (5)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-02-18T14:40:27.064Z
Learning: In the Snakemake executor plugin for SLURM, the GPU resource is specified using the "gpus" (plural) resource name, not "gpu" (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/utils.py:75-100
Timestamp: 2025-02-18T14:49:42.624Z
Learning: In the Snakemake SLURM executor plugin, users must specify either `gres` or `gpus` for GPU resources, but not both simultaneously, as these are mutually exclusive ways to request GPU resources.
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
🪛 LanguageTool
docs/further.md
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~66-~66: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them. Using [dynamic resource speci...
(BE_ABLE_TO)
[formatting] ~143-~143: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... their account or partition; for example, because they have a pre-defined default per use...
(COMMA_BEFORE_BECAUSE)
[typographical] ~146-~146: It seems that a comma is missing.
Context: ...no-account` flag to turn it off. ##### Wait Times and Frequencies There are a numb...
(WAIT_COMMA)
[formatting] ~171-~171: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... resource requirements with each attempt, because jobs will rarely exceed an otherwise se...
(COMMA_BEFORE_BECAUSE)
[locale-violation] ~172-~172: In American English, “take a look” is more commonly used.
Context: ... an otherwise sensible limit. For this, have a look at the [documentation of the snakemake ...
(HAVE_A_LOOK)
[typographical] ~210-~210: Usually, there’s no comma before “if”.
Context: ...his can be added to the srun statement, if required. Here's an example of definin...
(IF_NO_COMMA)
[uncategorized] ~658-~658: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...
(COMMA_COMPOUND_SENTENCE)
[style] ~673-~673: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ractive job in the same terminal, you need to type exit. Even if your job has ended...
(REP_NEED_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (8)
docs/further.md (8)
10-13: Installation section is clear and concise
The instructions correctly note that installing via conda brings in the jobstep plugin and recommend the storage plugin for data staging.
25-28: Issue filing guidance reads well
The instructions for where to file issues or pull requests are clear and correctly link to the appropriate repositories.
42-44: Configuration introduction is well-structured
The “Configuration” header and its lead-in make it easy for users to find how to set resources at different levels.
181-182: SLURM native requeue explanation is clear
The description of theJobRequeueparameter and its effect is accurate and helpful.
186-186: Plugin requeue flag usage is correct
The guidance for--slurm-requeueis clear. No changes needed.
271-274: GPU job guidance is comprehensive
The explanation of bothgpuandgresmethods, including manufacturer notes, provides users the necessary context to choose the right approach.
495-495:slurm_extraexample is illustrative
Demonstrating the use ofslurm_extrafor custom flags is useful.
384-385: Local execution instructions are spot on
Thelocalrulesdirective is correctly introduced with a helpful link.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/further.md (4)
10-13: Enhance installation instructions with a concrete example
The guide notes installation via conda but omits a direct command. Consider adding a snippet like:conda install -c conda-forge snakemake-executor-plugin-slurmThis makes it easier for users to get started without searching.
30-39: Fix shell example line continuation formatting
Line 36 has an extra space before the backslash, which can break copy-paste. Also, consider keeping the prompt marker (>) consistent:- > -j unlimited \ + > -j unlimited \Ensuring clean backslashes and prompt markers improves usability.
665-670: Correct typo in interactive job example
The word “chosing” should be “choosing” on line 673. Also, maintain consistent backslashes:- > --time <enough time> \ + > --time <enough time> \ - > <further arguments, e.g. memory> + > <further arguments, e.g. memory>This ensures the example works as intended.
673-675: Refine sub-shell warning for clarity
The current phrasing is slightly wordy. Consider:- Consider that `salloc` will start a particular subshell. Before you start another interactive job in the _same_ terminal, you need to type `exit`. Even if your job has ended already. + Note that `salloc` opens a subshell. Exit it before starting another interactive job in the same terminal—even if the previous job has completed.This streamlines the guidance.
🧰 Tools
🪛 LanguageTool
[style] ~675-~675: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ractive job in the same terminal, you need to typeexit. Even if your job has ended...(REP_NEED_TO_VB)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/further.md (5)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-02-18T14:40:27.064Z
Learning: In the Snakemake executor plugin for SLURM, the GPU resource is specified using the "gpus" (plural) resource name, not "gpu" (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/utils.py:75-100
Timestamp: 2025-02-18T14:49:42.624Z
Learning: In the Snakemake SLURM executor plugin, users must specify either `gres` or `gpus` for GPU resources, but not both simultaneously, as these are mutually exclusive ways to request GPU resources.
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
🪛 LanguageTool
docs/further.md
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~42-~42: Consider using the singular form after the singular determiner “This”.
Context: ...file system than the workflow ``` This examples assumes no limit for submitted jobs (`-...
(AGREEMENT_SENT_START)
[style] ~68-~68: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them. Using [dynamic resource speci...
(BE_ABLE_TO)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...n account can be used for [collection users' resource usage](https://slurm.schedmd....
(AI_HYDRA_LEO_MISSING_OF)
[formatting] ~145-~145: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... their account or partition; for example, because they have a pre-defined default per use...
(COMMA_BEFORE_BECAUSE)
[typographical] ~148-~148: It seems that a comma is missing.
Context: ...no-account` flag to turn it off. ##### Wait Times and Frequencies There are a numb...
(WAIT_COMMA)
[uncategorized] ~151-~151: Possible missing comma found.
Context: ...lugin try to provide sensible defaults. But especially if you become aware of your ...
(AI_HYDRA_LEO_MISSING_COMMA)
[formatting] ~173-~173: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... resource requirements with each attempt, because jobs will rarely exceed an otherwise se...
(COMMA_BEFORE_BECAUSE)
[locale-violation] ~174-~174: In American English, “take a look” is more commonly used.
Context: ... an otherwise sensible limit. For this, have a look at the [documentation of the snakemake ...
(HAVE_A_LOOK)
[uncategorized] ~201-~201: The verb “is” doesn’t seem to fit in this context, “are” is probably more formally correct.
Context: ...ted by the maximum number of cores that is available on any machine in the cluster...
(AI_HYDRA_LEO_CPT_IS_ARE)
[uncategorized] ~202-~202: “you” seems less likely than “your” (belonging to you).
Context: ...izing to more cores than any machine in you cluster can offer. [See the main docume...
(AI_HYDRA_LEO_CP_YOU_YOUR)
[typographical] ~212-~212: Usually, there’s no comma before “if”.
Context: ...his can be added to the srun statement, if required. Here's an example of definin...
(IF_NO_COMMA)
[uncategorized] ~660-~660: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...
(COMMA_COMPOUND_SENTENCE)
[style] ~675-~675: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...ractive job in the same terminal, you need to type exit. Even if your job has ended...
(REP_NEED_TO_VB)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (12)
docs/further.md (12)
15-28: Clarify contribution paths and issue templates
This section does a great job listing where bugs may originate. To further streamline contributions, you might:
- Link to example issue/PR templates.
- Briefly describe when to file in each of the three repos.
Overall, the expanded guidance significantly improves the contributor experience.🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...(UNLIKELY_OPENING_PUNCTUATION)
44-49: Approve Configuration section intro
The new “Configuration” heading and overview clearly set up the detailed subsections. It accurately frames how standard and SLURM-specific resources are handled.
51-55: Approve resource specification locations
Listing rule definitions, profiles, and CLI flags as the three configuration layers is spot on. This clarity will help users pick the right approach.
66-73: Approve dynamic resource specification details
Good linking to Snakemake’s dynamic resources examples, highlighting input-based and retry-based configurations. This empowers users to write adaptable rules.🧰 Tools
🪛 LanguageTool
[style] ~68-~68: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them. Using [dynamic resource speci...(BE_ABLE_TO)
76-82: Approve Standard Resources overview
The heading and explanation correctly introduce Snakemake’s standard resources and their mapping. Emphasizing dynamic resources here reinforces best practices.
83-88: Approve standard resources mapping table
The table accurately mapsgpu→--gpus,mem/mem_mb→--mem, andruntime→--time. This aligns with the plugin’s behavior.
93-97: Approve SLURM-specific Resources introduction
The text clearly explains that these options are platform-specific and belong in profiles. This distinction aids workflow portability.
99-113: Approve SLURM-specific resources mapping table
All entries (clusters,constraint,cpus_per_task,mem_mb_per_cpu,nodes,slurm_account,slurm_partition,slurm_requeue,tasks) map correctly to theirsbatchflags including--requeueforslurm_requeue.
272-277: Approve GPU job configuration guidance
The section on choosing betweengpuvs.gres, model specification, and mapping to--gpusis clear and accurate. Noting SLURM’s lack of manufacturer distinction is an important caveat.
386-387: Approvelocalrulessection
Introducing thelocalrulesdirective with a link helps users offload quick tasks to the head node. This hybrid execution tip is a valuable addition.
497-498: Approveslurm_extraexample
Theslurm_extrasnippet demonstrates how to pass custom flags clearly. Reinforcing profile use over hardcoding is a good best practice.
660-662: Approve cluster-specific support note
Guidance to file specialized cluster-setup issues in the plugin repo directs users to the correct venue. This avoids confusion about where to report.🧰 Tools
🪛 LanguageTool
[uncategorized] ~660-~660: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...(COMMA_COMPOUND_SENTENCE)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
docs/further.md (7)
12-13: Clarify Plugin Names and Links
The text refers to the'jobstep' plugin—consider linking to or using its full package name (snakemake-executor-plugin-slurm-jobstep) for clarity. Also, the recommendation to installsnakemake-storage-plugin-fsis great; perhaps add a link to its repository or PyPI page.
19-23: Improve Bug Origin List Phrasing
The phrase “Additionally, bugs related to the plugin can originate in the:” is a bit abrupt. It would read more naturally as:Additionally, bugs related to the plugin can originate in the following repositories:This makes the list items that follow clearer.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...(UNLIKELY_OPENING_PUNCTUATION)
25-26: Streamline Issue Filing Instructions
The instructions are helpful, but could be tightened for consistency and tone. For example:If you can pinpoint the exact repository your issue pertains to, please file it there. If you're unsure, create a new issue here and we will direct you to the correct repo.
32-39: Separate Comments from Code Snippet
Embedding the explanatory comment on the same line as the final command may break the copy-paste experience. Consider moving “# assuming a data path…” outside the snippet, e.g.:```console $ snakemake --executor slurm \ -j unlimited \ --workflow-profile <profile directory with a `config.yaml`> \ --configfile config/config.yaml \ --directory <path>
assuming a data path on a different file system than the workflow
This keeps the snippet clean. --- `42-42`: **Fix Grammar in Example Description** Change “This examples assumes” to “This example assumes” to match singular usage: ```diff - This examples assumes no limit for submitted jobs (`-j unlimited`). + This example assumes no limit for submitted jobs (`-j unlimited`).🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: Consider using the singular form after the singular determiner “This”.
Context: ...file system than the workflow ``` This examples assumes no limit for submitted jobs (`-...(AGREEMENT_SENT_START)
660-662: Add Comma for Compound Sentence
The sentence combines two independent clauses; add a comma after “job” for correct punctuation:- If your administrators require running Snakemake within a job and you encounter issues, please report… + If your administrators require running Snakemake within a job, and you encounter issues, please report…🧰 Tools
🪛 LanguageTool
[uncategorized] ~660-~660: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...(COMMA_COMPOUND_SENTENCE)
665-665: Remove Unnecessary Comma
In “Doing so, will preserve…”, the comma after “so” should be removed:- Doing so, will preserve Snakemake's output and ease debugging. + Doing so will preserve Snakemake's output and ease debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/further.md (5)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-02-18T14:40:27.064Z
Learning: In the Snakemake executor plugin for SLURM, the GPU resource is specified using the "gpus" (plural) resource name, not "gpu" (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/utils.py:75-100
Timestamp: 2025-02-18T14:49:42.624Z
Learning: In the Snakemake SLURM executor plugin, users must specify either `gres` or `gpus` for GPU resources, but not both simultaneously, as these are mutually exclusive ways to request GPU resources.
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
🪛 LanguageTool
docs/further.md
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~42-~42: Consider using the singular form after the singular determiner “This”.
Context: ...file system than the workflow ``` This examples assumes no limit for submitted jobs (`-...
(AGREEMENT_SENT_START)
[style] ~68-~68: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them. Using [dynamic resource speci...
(BE_ABLE_TO)
[uncategorized] ~138-~138: Possible missing preposition found.
Context: ...n account can be used for [collection users' resource usage](https://slurm.schedmd....
(AI_HYDRA_LEO_MISSING_OF)
[formatting] ~145-~145: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... their account or partition; for example, because they have a pre-defined default per use...
(COMMA_BEFORE_BECAUSE)
[typographical] ~148-~148: It seems that a comma is missing.
Context: ...no-account` flag to turn it off. ##### Wait Times and Frequencies There are a numb...
(WAIT_COMMA)
[uncategorized] ~151-~151: Possible missing comma found.
Context: ...lugin try to provide sensible defaults. But especially if you become aware of your ...
(AI_HYDRA_LEO_MISSING_COMMA)
[formatting] ~173-~173: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... resource requirements with each attempt, because jobs will rarely exceed an otherwise se...
(COMMA_BEFORE_BECAUSE)
[locale-violation] ~174-~174: In American English, “take a look” is more commonly used.
Context: ... an otherwise sensible limit. For this, have a look at the [documentation of the snakemake ...
(HAVE_A_LOOK)
[uncategorized] ~201-~201: The verb “is” doesn’t seem to fit in this context, “are” is probably more formally correct.
Context: ...ted by the maximum number of cores that is available on any machine in the cluster...
(AI_HYDRA_LEO_CPT_IS_ARE)
[uncategorized] ~202-~202: “you” seems less likely than “your” (belonging to you).
Context: ...izing to more cores than any machine in you cluster can offer. [See the main docume...
(AI_HYDRA_LEO_CP_YOU_YOUR)
[typographical] ~212-~212: Usually, there’s no comma before “if”.
Context: ...his can be added to the srun statement, if required. Here's an example of definin...
(IF_NO_COMMA)
[uncategorized] ~660-~660: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (5)
docs/further.md (5)
10-10: Installation Section Heading Looks Good
The new “### Installation” heading is clear and consistent with the rest of the document.
15-15: Contributions Section Heading
The “### Contributions” heading cleanly introduces the contribution guidelines. No issues here.
386-387: Localrules Directive Explanation
The addition of thelocalrulesdirective and link to the official docs is spot on. This will help users run specific rules locally.
497-497:slurm_extraExample
Great inclusion of an example forslurm_extra. Quoting the flags correctly illustrates how to pass multiple options.
668-672: Interactive Job Snippet Formatting
The interactive job example is helpful. To improve readability, you might:
- Ensure the final line does not include a prompt continuation arrow (
>).- Optionally, move generic placeholders (e.g.,
<further arguments…>) into a separate explanatory note below the snippet.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/further.md (3)
10-13: Nit: Capitalize SLURM and clarify installation methods
To maintain consistency with the rest of the document, please change “slurm execution nodes” to “SLURM execution nodes.”
Optionally, you may also mention alternative installation via pip (e.g.pip install snakemake-executor-plugin-slurm) or specify the recommended conda channel (-c conda-forge).
25-26: Improve hyperlink and punctuation in issue filing instructions
- Add a comma before “and” in line 26 (“…here, and we will…”).
- Hyperlink the word “here” to the same
/issuesURL to make it easy for users to file an issue.🧰 Tools
🪛 LanguageTool
[uncategorized] ~26-~26: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... you are unsure, create a new issue here and we will direct you to the correct repo....(COMMA_COMPOUND_SENTENCE)
661-661: Minor stylistic suggestion for clarity
Consider rephrasing to something like:If your administrators require that you run Snakemake within a job and you encounter issues, please file them in the plugin’s GitHub repository.
This makes the sentence structure a bit smoother.🧰 Tools
🪛 LanguageTool
[uncategorized] ~661-~661: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/further.md (5)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-02-18T14:40:27.064Z
Learning: In the Snakemake executor plugin for SLURM, the GPU resource is specified using the "gpus" (plural) resource name, not "gpu" (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/utils.py:75-100
Timestamp: 2025-02-18T14:49:42.624Z
Learning: In the Snakemake SLURM executor plugin, users must specify either `gres` or `gpus` for GPU resources, but not both simultaneously, as these are mutually exclusive ways to request GPU resources.
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
🪛 LanguageTool
docs/further.md
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~26-~26: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... you are unsure, create a new issue here and we will direct you to the correct repo....
(COMMA_COMPOUND_SENTENCE)
[grammar] ~42-~42: Consider using the singular form after the singular determiner “This”.
Context: ...g.yaml \ > --directory ``` This examples assumes no limit for submitted jobs (`-...
(AGREEMENT_SENT_START)
[style] ~69-~69: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them. Using [dynamic resource speci...
(BE_ABLE_TO)
[formatting] ~146-~146: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... their account or partition; for example, because they have a pre-defined default per use...
(COMMA_BEFORE_BECAUSE)
[typographical] ~149-~149: It seems that a comma is missing.
Context: ...no-account` flag to turn it off. ##### Wait Times and Frequencies There are a numb...
(WAIT_COMMA)
[formatting] ~174-~174: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... resource requirements with each attempt, because jobs will rarely exceed an otherwise se...
(COMMA_BEFORE_BECAUSE)
[locale-violation] ~175-~175: In American English, “take a look” is more commonly used.
Context: ... an otherwise sensible limit. For this, have a look at the [documentation of the snakemake ...
(HAVE_A_LOOK)
[typographical] ~213-~213: Usually, there’s no comma before “if”.
Context: ...his can be added to the srun statement, if required. Here's an example of definin...
(IF_NO_COMMA)
[uncategorized] ~661-~661: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...s require running Snakemake within a job and you encounter issues, please report the...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (4)
docs/further.md (4)
32-39: Example invocation snippet is clear and correct
The multi-linesnakemake --executor slurm …example uses proper backslashes and prompts. No changes needed.
387-388: Local execution directive is well described
The addition of thelocalrulesdirective and the link to the official Snakemake docs is clear and accurate.
498-498:slurm_extraresource example is accurate
The Python-style assignment forslurm_extracorrectly demonstrates how to pass arbitrary sbatch flags.
668-672: Interactive SLURM job snippet is correct
Thesallocmulti-line example uses proper backslashes and clearly illustrates how to start an interactive SLURM session for debugging. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
docs/further.md (1)
203-205: Duplicate comment: MPI launcher phrasing
A previous review noted the typo “launcer” → “launcher” and suggested removing the comma before “if required.” That feedback remains applicable here.🧰 Tools
🪛 LanguageTool
[typographical] ~204-~204: Usually, there’s no comma before “if”.
Context: ...his can be added to thesrunstatement, if required. Here's an example of definin...(IF_NO_COMMA)
🧹 Nitpick comments (2)
docs/further.md (2)
10-13: Enhance installation clarity with example command
Adding an explicit install example will help users get started more quickly. Consider appending a snippet like:conda install -c conda-forge snakemake-executor-plugin-slurm
32-39: Ensure CLI example formatting is consistent
The block mixes>prefixes with backslashes, which can be confusing. Consider removing the>markers and indenting continuation lines uniformly:- $ snakemake --executor slurm \ - > -j unlimited \ - > --workflow-profile <profile directory with a `config.yaml`> \ - > --configfile config/config.yaml \ - > --directory <path> + $ snakemake --executor slurm \ + -j unlimited \ + --workflow-profile <profile directory with a `config.yaml`> \ + --configfile config/config.yaml \ + --directory <path>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/further.md (5)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-02-18T14:40:27.064Z
Learning: In the Snakemake executor plugin for SLURM, the GPU resource is specified using the "gpus" (plural) resource name, not "gpu" (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/utils.py:75-100
Timestamp: 2025-02-18T14:49:42.624Z
Learning: In the Snakemake SLURM executor plugin, users must specify either `gres` or `gpus` for GPU resources, but not both simultaneously, as these are mutually exclusive ways to request GPU resources.
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
🪛 LanguageTool
docs/further.md
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~42-~42: Consider using the singular form after the singular determiner “This”.
Context: ...g.yaml \ > --directory ``` This examples assumes no limit for submitted jobs (`-...
(AGREEMENT_SENT_START)
[style] ~64-~64: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them.administrators based on the si...
(BE_ABLE_TO)
[formatting] ~137-~137: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... their account or partition; for example, because they have a pre-defined default per use...
(COMMA_BEFORE_BECAUSE)
[typographical] ~140-~140: It seems that a comma is missing.
Context: ...no-account` flag to turn it off. ##### Wait Times and Frequencies There are a numb...
(WAIT_COMMA)
[formatting] ~165-~165: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... resource requirements with each attempt, because jobs will rarely exceed an otherwise se...
(COMMA_BEFORE_BECAUSE)
[locale-violation] ~166-~166: In American English, “take a look” is more commonly used.
Context: ... an otherwise sensible limit. For this, have a look at the [documentation of the snakemake ...
(HAVE_A_LOOK)
[typographical] ~204-~204: Usually, there’s no comma before “if”.
Context: ...his can be added to the srun statement, if required. Here's an example of definin...
(IF_NO_COMMA)
[uncategorized] ~652-~652: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...uire that you run Snakemake within a job and you encounter issues, please report the...
(COMMA_COMPOUND_SENTENCE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (3)
docs/further.md (3)
68-80: Standard Resources section looks correct
The header and mapping table forgpu,mem, andruntimealign with Slurm flags and Snakemake conventions. No changes needed.
85-107: SLURM-specific Resources mapping is accurate
All Slurm-specific plugin options are correctly mapped tosbatcharguments (--clusters,--constraint/-C,--cpus-per-task, etc.). Excellent clarity.
110-668: Skip remaining documentation changes
The rest of the updated sections—detailed resource descriptions, GPU examples, local execution, profiles, advanced configs, software recommendations, job nesting, and interactive usage—are well-written, comprehensive, and accurate.🧰 Tools
🪛 LanguageTool
[formatting] ~137-~137: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... their account or partition; for example, because they have a pre-defined default per use...(COMMA_BEFORE_BECAUSE)
[typographical] ~140-~140: It seems that a comma is missing.
Context: ...no-account` flag to turn it off. ##### Wait Times and Frequencies There are a numb...(WAIT_COMMA)
[formatting] ~165-~165: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... resource requirements with each attempt, because jobs will rarely exceed an otherwise se...(COMMA_BEFORE_BECAUSE)
[locale-violation] ~166-~166: In American English, “take a look” is more commonly used.
Context: ... an otherwise sensible limit. For this, have a look at the [documentation of the snakemake ...(HAVE_A_LOOK)
[typographical] ~204-~204: Usually, there’s no comma before “if”.
Context: ...his can be added to thesrunstatement, if required. Here's an example of definin...(IF_NO_COMMA)
[typographical] ~299-~299: Two consecutive dots
Context: ...ust be specified as a numerical value. .. note:: Internally, Snakemake knows the ...(DOUBLE_PUNCTUATION)
[typographical] ~344-~344: Two consecutive dots
Context: ...four CPU cores for each GPU requested. .. note:: Ifcpus_per_gpuis set to a va...(DOUBLE_PUNCTUATION)
[uncategorized] ~414-~414: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...ifications A more flexible approach to manage resource specifications is by using Sna...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[uncategorized] ~499-~499: A comma might be missing here.
Context: ...ml#integrated-package-management). On a cluster sometimes a file system other than `HOM...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[formatting] ~499-~499: Consider inserting a comma after an introductory phrase for better readability.
Context: ...icated (for example because of quotas). In this case pointing the installation to different ...(IN_THAT_CASE_COMMA)
[uncategorized] ~500-~500: You might be missing the article “a” here.
Context: ... this case pointing the installation to different file system with `--conda-prefix /other...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[misspelling] ~525-~525: This word is normally spelled with a hyphen.
Context: ... strain on the batch system's database. Human readable information is stored in the comment of...(EN_COMPOUNDS_HUMAN_READABLE)
[uncategorized] ~580-~580: Loose punctuation mark.
Context: ... not needed anymore. -latency-wait: 5: Sets a 5-second latency wait to accommo...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~581-~581: Loose punctuation mark.
Context: ...delays. -default-storage-provider: fs: Utilizes the file system storage plugin...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~627-~627: This phrase is redundant. Consider writing “consult”.
Context: ... computational tasks. It's advisable to consult with your cluster's support team to understa...(CONSULT_WITH)
[uncategorized] ~652-~652: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...uire that you run Snakemake within a job and you encounter issues, please report the...(COMMA_COMPOUND_SENTENCE)
🪛 markdownlint-cli2 (0.17.2)
223-223: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
224-224: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
225-225: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
235-235: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
256-256: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
257-257: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
294-294: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
295-295: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
369-369: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
370-370: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
371-371: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
435-435: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
436-436: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
508-508: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
519-519: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
520-520: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
553-553: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
555-555: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
557-557: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
578-578: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
580-580: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
581-581: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
582-582: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
583-583: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
584-584: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…-executor-plugin-slurm into docs/review-new-docs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/further.md (2)
18-19:⚠️ Potential issueFix GitHub issues link to
/issues.
The Contributions section currently links to/issue, which is invalid. Update it to point at/issuesfor the plugin’s issue tracker:- https://github.com/snakemake/snakemake-executor-plugin-slurm/issue + https://github.com/snakemake/snakemake-executor-plugin-slurm/issues
42-43:⚠️ Potential issueCorrect typographical and grammatical errors.
- “This examples assumes” → “This example assumes”
- “Furtheremore” → “Furthermore”
- “seperation” → “separation”
- Split into two sentences for readability:
- This examples assumes no limit for submitted jobs (`-j unlimited`). Any number, e.g. `-j 150`, will throttle a workflow to this number of concurrent jobs. - Furtheremore we on many clusters we have to assume seperation from workflows and data, hence the flag `--directory <path>` which can point to a different file system, containing your data. + This example assumes no limit for submitted jobs (`-j unlimited`). Any number, e.g., `-j 150`, will throttle the workflow to that limit. + Furthermore, on many clusters, workflows and data are separated. Use the `--directory <path>` flag to point to the data file system.🧰 Tools
🪛 LanguageTool
[grammar] ~42-~42: Consider using the singular form after the singular determiner “This”.
Context: ...g.yaml \ > --directory ``` This examples assumes no limit for submitted jobs (`-...(AGREEMENT_SENT_START)
🧹 Nitpick comments (5)
docs/further.md (5)
25-26: Make “here” a clickable hyperlink.
In “If you are unsure, create a new issue here…”, the word “here” should link directly to the issues page:- create a new issue here + [create a new issue here](https://github.com/snakemake/snakemake-executor-plugin-slurm/issues)
35-39: **Remove literal “>prompts in code block for clarity.** The example console block uses>` markers on continuation lines, which can confuse readers. Prefer consistent indentation:- $ snakemake --executor slurm \ - > -j unlimited \ - > --workflow-profile <profile directory with a `config.yaml`> \ + $ snakemake --executor slurm \ + -j unlimited \ + --workflow-profile <profile directory with a `config.yaml`> \Alternatively, clarify that
>is the shell prompt.
64-65: Fix garbled sentence in Dynamic Resource Specification.
The sentence contains a stray token and missing spacing. Consider rephrasing:- Where to set configurations can also depend on how generically we are able to set them.administrators based on the size of input files, or increasing the runtime with every `attempt` of running a job ... + How and where you set configurations can depend on factors such as input file size or increasing runtime with each `attempt` of a job ...🧰 Tools
🪛 LanguageTool
[style] ~64-~64: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them.administrators based on the si...(BE_ABLE_TO)
265-268: Clarify GPU jobs introductory guidance.
It may help users to note that SLURM’s--gpusand--gresflags are mutually exclusive. Example addition:- Additionally, while Snakemake internally recognizes the `gpu_manufacturer` resource, SLURM does not distinguish between GPU model and manufacturer. + Note: SLURM allows requesting GPUs via either `--gpus` or `--gres` (generic resources) but not both simultaneously. Consult your cluster configuration for the preferred method.
657-658: Remove unnecessary comma in interactive job recommendation.
The comma after “Doing so” should be omitted:- Doing so, will preserve Snakemake's output and ease debugging. + Doing so will preserve Snakemake's output and ease debugging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/further.md (5)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (even simple numeric ones) should be translated to the `--gpus` flag (plural) to match SLURM's expectations, not `--gpu` (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: tests/tests.py:79-86
Timestamp: 2025-03-10T13:34:59.270Z
Learning: In the Snakemake executor plugin for SLURM, all GPU resources (both simple numeric values and model:number specifications) should be translated to the `--gpus` flag (plural form) to match SLURM's command line interface expectations.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-02-18T14:40:27.064Z
Learning: In the Snakemake executor plugin for SLURM, the GPU resource is specified using the "gpus" (plural) resource name, not "gpu" (singular).
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/utils.py:75-100
Timestamp: 2025-02-18T14:49:42.624Z
Learning: In the Snakemake SLURM executor plugin, users must specify either `gres` or `gpus` for GPU resources, but not both simultaneously, as these are mutually exclusive ways to request GPU resources.
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
🪛 LanguageTool
docs/further.md
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...snakemake-executor-plugin-slurm-jobstep), which runs snakemake within slurm jobs ...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~42-~42: Consider using the singular form after the singular determiner “This”.
Context: ...g.yaml \ > --directory ``` This examples assumes no limit for submitted jobs (`-...
(AGREEMENT_SENT_START)
[style] ~64-~64: As a shorter alternative for ‘able to’, consider using “can”.
Context: ...s can also depend on how generically we are able to set them.administrators based on the si...
(BE_ABLE_TO)
[formatting] ~137-~137: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... their account or partition; for example, because they have a pre-defined default per use...
(COMMA_BEFORE_BECAUSE)
[typographical] ~140-~140: It seems that a comma is missing.
Context: ...no-account` flag to turn it off. ##### Wait Times and Frequencies There are a numb...
(WAIT_COMMA)
[formatting] ~165-~165: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... resource requirements with each attempt, because jobs will rarely exceed an otherwise se...
(COMMA_BEFORE_BECAUSE)
[locale-violation] ~166-~166: In American English, “take a look” is more commonly used.
Context: ... an otherwise sensible limit. For this, have a look at the [documentation of the snakemake ...
(HAVE_A_LOOK)
[typographical] ~204-~204: Usually, there’s no comma before “if”.
Context: ...his can be added to the srun statement, if required. Here's an example of definin...
(IF_NO_COMMA)
[uncategorized] ~652-~652: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...uire that you run Snakemake within a job and you encounter issues, please report the...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (4)
docs/further.md (4)
10-13: Installation section is clear and accurate.
The heading and instructions correctly describe how to install the plugin (including thejobstepplugin) and recommend thesnakemake-storage-plugin-fs. No issues detected.
75-80: Standard resources table mapping is correct.
Thegpu,mem, andruntimeresources are accurately mapped to SLURM’s--gpus,--mem, and--timeflags.
103-106: SLURM-specific resources table is accurate.
The entries forslurm_requeue→--requeue,clusters,constraint, and others correctly reflect the plugin’s behavior.
378-379: “Running Jobs locally” section is well phrased.
The use of thelocalrulesdirective is correct and properly linked.
…-executor-plugin-slurm into docs/review-new-docs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…-executor-plugin-slurm into docs/review-new-docs
Summary by CodeRabbit
Documentation
docs/further.mdwith extensive restructuring and detailed guidance on installation, resource configuration, MPI, GPU, local execution, advanced settings, and job nesting.Chores