-
Notifications
You must be signed in to change notification settings - Fork 38
feat: new flag for SLURM qos #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds optional SLURM QoS support: introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Snakemake
participant SLURM as "SLURM (sbatch)"
User->>Snakemake: Configure executor (may set qos)
Snakemake->>Snakemake: Build sbatch args (reservation?, qos?)
alt QoS provided
Note over Snakemake: Append --qos=<value>
else QoS not provided
Note over Snakemake: No --qos flag
end
Snakemake->>SLURM: sbatch [--reservation ...] [--qos <qos>] job.sh
SLURM-->>Snakemake: Submission response (job ID)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/further.md (1)
630-633
: Polish QoS section: capitalization, grammar, and add a concrete CLI example.Apply:
-### Using SLURM QoS - -To use SLURM's quality of service flags (`--qos` to `sbatch`) the plugin allows specifying `--slurm-qos=<qos-string>`, too. Both, the reservation and the qos may be particularly useful in a course setting. +### Using SLURM QoS + +To use SLURM Quality of Service, set `--slurm-qos=<qos>`. The plugin passes this to `sbatch` as `--qos=<qos>`. Both reservation and QoS can be useful in course settings. + +Example: +```console +snakemake --executor slurm --slurm-qos=short ... +```snakemake_executor_plugin_slurm/__init__.py (1)
143-150
: QoS setting addition looks good; consider parity with per-rule resources.
Field definition is consistent with existing settings. Optionally, allow a per-rule resource (e.g.,slurm_qos
) similar toslurm_partition
/slurm_account
, falling back to the global--slurm-qos
when unset. This would let users vary QoS across rules withoutslurm_extra
.I can sketch the lookup order and a small helper to resolve per-job QoS if you want.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/further.md
(1 hunks)snakemake_executor_plugin_slurm/__init__.py
(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
@coderabbitai - I noticed, your walkthrough states a |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 15806 @cmeesters Thank you for the correction! You're absolutely right. I apologize for the error in my walkthrough. Looking at the actual implementation, I can see that the SLURM executor uses the call += f' --wrap="{exec_job}"' where This is an important distinction about how the plugin actually operates - it submits the Snakemake process directly rather than generating and submitting a separate job script. Thank you for keeping the technical details accurate! ✏️ Learnings added
🧠 Learnings used
|
🤖 I have created a release *beep* *boop* --- ## [1.7.0](v1.6.1...v1.7.0) (2025-09-09) ### Features * new flag for SLURM qos ([#351](#351)) ([55068ae](55068ae)) --- 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 - Documentation - Added 1.7.0 release entry to the changelog, highlighting a new flag for SLURM QoS. - Chores - Bumped package version to 1.7.0 to reflect the latest release. No functional changes are included in this PR; it documents the release and updates version metadata only. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
a new flag
--slurm-qos
has been added to conform with reservations requiring a particular qos upon job submission.Summary by CodeRabbit
New Features
Documentation