feat: adding reservation flag #323
Conversation
WalkthroughA new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor
participant SLURM
User->>Executor: Submit job (with optional reservation)
Executor->>Executor: Check if reservation is set
alt Reservation is set
Executor->>SLURM: Submit job with --reservation argument
else Reservation not set
Executor->>SLURM: Submit job without --reservation
end
SLURM-->>Executor: Job submission response
Executor-->>User: Return submission result
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
|
![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
🧹 Nitpick comments (2)
docs/further.md (2)
623-627: Refine wording for clarity and grammar.The current sentences are a bit awkward and contain a preposition issue.
Apply this diff:- This scenario is mostly done when testing or teaching. + This is primarily used for testing or teaching scenarios. - It does not validate the spelling nor eligibility to this reservation + It does not validate the spelling or eligibility for this reservation.🧰 Tools
🪛 LanguageTool
[uncategorized] ~625-~625: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation For teaching, it is re...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
621-628: Include a CLI usage example.Readers would benefit from a concrete snippet demonstrating how to pass the reservation flag. For example, add:
### Running in SLURM reservations This is primarily used for testing or teaching scenarios. The plugin allows specifying a flag `--reservation=<name>` to use a particular reservation. It does not validate the spelling or eligibility for this reservation. For teaching, it is recommended that you ask your administrators to set up a reservation with the SLURM `magnetic` flag. This will ensure that the `--reservation` flag is not necessary, e.g. if the reservation is associated with a particular course account. + ```console + $ snakemake --executor slurm --reservation my_course_reservation \ + -j 100 --workflow-profile path/to/profile --directory /path/to/workdir + ```🧰 Tools
🪛 LanguageTool
[uncategorized] ~625-~625: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation For teaching, it is re...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/further.md
[uncategorized] ~625-~625: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation For teaching, it is re...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/further.md (1)
621-628: Minor grammar and clarity improvement in reservation subsection
The sentence “It does not validate the spelling nor eligibility to this reservation” reads more naturally as “eligibility for this reservation.” Also, add a period to separate it from the next clause.- It does not validate the spelling nor eligibility to this reservation - For teaching, it is recommended that you ask your administrators to set up a reservation with the SLURM `magnetic` flag. + It does not validate the spelling nor eligibility for this reservation. + For teaching, it is recommended that you ask your administrators to set up a reservation with the SLURM `magnetic` flag.🧰 Tools
🪛 LanguageTool
[uncategorized] ~625-~625: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation For teaching, it is re...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/further.md
[uncategorized] ~625-~625: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation For teaching, it is re...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
Co-authored-by: Filipe G. Vieira <1151762+fgvieira@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/further.md (1)
623-623: Duplicate of earlier suggestion
This description matches the feedback from fgvieira’s previous review.
🧹 Nitpick comments (2)
docs/further.md (2)
624-624: Grammar refinement
Change “nor eligibility to this reservation” to “or eligibility of this reservation” for clarity.-It does not validate the spelling nor eligibility to this reservation. +It does not validate the spelling or eligibility of this reservation.🧰 Tools
🪛 LanguageTool
[uncategorized] ~624-~624: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation. ### Frequently Asked...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
621-625: Relocate section for better context
Consider moving “Running in SLURM reservations” out of the nesting-jobs section and into the SLURM-specific resources area under Configuration for improved discoverability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~624-~624: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation. ### Frequently Asked...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/further.md
[uncategorized] ~624-~624: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation. ### Frequently Asked...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (1)
docs/further.md (1)
623-623: Verify flag naming consistency
Ensure that the CLI flag named here (--slurm-reservation) aligns with the actual implementation (e.g., argument parser definition and the sbatch option).Search the codebase for occurrences of both
--slurm-reservationand--reservationto confirm the correct flag is documented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/further.md (1)
621-625: Fix heading spelling and improve clarity in documentationThe heading “Submittings” is misspelled and can be simplified for consistency. Also, rephrase the second sentence to clearly state what isn’t validated.
-### Submittings Jobs into SLURM reservations +# Submitting Jobs into SLURM Reservations -The plugin allows specifying a flag `--slurm-reservation=<name>` to use a particular reservation. -It does not validate the spelling nor eligibility to this reservation. +The plugin allows specifying a flag `--slurm-reservation=<name>` to use a particular reservation. +It does not validate the reservation name or check user eligibility.🧰 Tools
🪛 LanguageTool
[uncategorized] ~624-~624: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation. ### Frequently Asked...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/further.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/further.md
[uncategorized] ~624-~624: The preposition “for” seems more likely in this position than the preposition “to”.
Context: ...t validate the spelling nor eligibility to this reservation. ### Frequently Asked...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION_TO_FOR)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🤖 I have created a release *beep* *boop* --- ## [1.4.0](v1.3.6...v1.4.0) (2025-06-12) ### Features * adding reservation flag ([#323](#323)) ([d4e0a0f](d4e0a0f)) ### Documentation * review and edit new docs ([#237](#237)) ([ec82a70](ec82a70)) --- 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 a reservation flag feature. - **Documentation** - Updated changelog with version 1.4.0 details and refined documentation. - **Chores** - Bumped application version to 1.4.0. - Updated release announcement workflow to use latest Mastodon posting action. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Christian Meesters <cmeesters@users.noreply.github.com>
This PR adds a
--reservationflag. Usually, for teaching, we recommend relying on SLURM'smagneticflag. This way, users will not need to specify a reservation. However, not all reservations can be made magnetic nor for all courses. Hence, the introduction of a new flag.Supersedes PR #285
Summary by CodeRabbit
New Features
Documentation
--slurm-reservationflag and noted that reservation validation and eligibility checks are not performed by the plugin.