Skip to content

Conversation

cmeesters
Copy link
Member

@cmeesters cmeesters commented Sep 9, 2025

a new flag --slurm-qos has been added to conform with reservations requiring a particular qos upon job submission.

Summary by CodeRabbit

  • New Features

    • Added optional SLURM QoS support for job submissions. Users can specify a QoS via a new --slurm-qos= CLI option or corresponding executor setting; no change occurs when unset.
  • Documentation

    • Added “Using SLURM QoS” under SLURM reservations, explaining how to configure QoS alongside reservations and the new option.

Copy link
Contributor

coderabbitai bot commented Sep 9, 2025

Walkthrough

Adds optional SLURM QoS support: introduces qos in ExecutorSettings and appends --qos=<value> to sbatch submissions when set. Also updates documentation with a "Using SLURM QoS" section describing a --slurm-qos=<qos-string> option. No other API changes.

Changes

Cohort / File(s) Summary
Documentation: SLURM QoS usage
docs/further.md
Added a section describing SLURM QoS support and the --slurm-qos=<qos-string> option alongside reservations.
Executor settings and submission logic
snakemake_executor_plugin_slurm/__init__.py
Added ExecutorSettings.qos: Optional[str] and updated sbatch command assembly to append --qos=<value> when workflow.executor_settings.qos is provided.

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)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • dlaehnemann
  • fgvieira

Pre-merge checks (2 passed, 1 warning)

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The current title clearly summarizes the main change by stating the addition of a new SLURM QoS flag and directly reflects the primary feature introduced in the pull request.
Description Check ✅ Passed The description accurately references the addition of the --slurm-qos flag to support QoS requirements in SLURM reservations, directly aligning with the documented changes in the pull request.

Poem

I nibble docs and code with glee,
A QoS flag for SLURM, whee! 🐇
Reservations hum, queues align,
sbatch hops on the dotted line.
Small changes, big hops — celebrate with me!

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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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 details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fea54a9 and 99279d7.

📒 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
⏰ 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
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/qos-flag

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 to slurm_partition/slurm_account, falling back to the global --slurm-qos when unset. This would let users vary QoS across rules without slurm_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

📥 Commits

Reviewing files that changed from the base of the PR and between 28924a7 and fea54a9.

📒 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

@cmeesters
Copy link
Member Author

@coderabbitai - I noticed, your walkthrough states a job.sh, where in fact, Snakemake submits itself.

Copy link
Contributor

coderabbitai bot commented Sep 9, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Let's examine the SLURM executor code to understand how job submission actually works
cat snakemake_executor_plugin_slurm/__init__.py | grep -A 20 -B 5 "sbatch"

# Also check the documentation to see if there are any references to job.sh or submission mechanism
cat docs/further.md | grep -i "job\|submit"

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 --wrap parameter in the sbatch command to directly execute the Snakemake command itself, rather than creating a separate job.sh script file. The code shows:

call += f' --wrap="{exec_job}"'

where exec_job is the formatted Snakemake execution command. The documentation also clearly states that "Snakemake's SLURM executor submits itself as a job, ensuring that all features function correctly within the job context."

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
Learnt from: cmeesters
URL: https://github.com/snakemake/snakemake-executor-plugin-slurm/pull/351

Timestamp: 2025-09-09T15:55:55.636Z
Learning: In the Snakemake SLURM executor plugin, Snakemake submits itself as a job using the --wrap parameter in the sbatch command, rather than creating or submitting a separate job.sh script file. The actual implementation uses `call += f' --wrap="{exec_job}"'` where exec_job is the formatted snakemake execution command.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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.

Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.

@cmeesters cmeesters merged commit 55068ae into main Sep 9, 2025
6 checks passed
@cmeesters cmeesters deleted the feat/qos-flag branch September 9, 2025 17:48
cmeesters pushed a commit that referenced this pull request Sep 9, 2025
🤖 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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant