fix: rerun-trigger regression#3492
Conversation
fixes 3491
📝 WalkthroughWalkthroughThe change updates the default value for the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches
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:
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake/cli.py (1)
724-724: Approving the fix for rerun-trigger default valueThis change restores the correct default value for the
--rerun-triggersargument, ensuring that all available rerun triggers are enabled by default. This aligns with the help text description at line 726 which states "By default, all triggers are used..."The change fixes the regression mentioned in PR title by replacing
RerunTrigger.choices()withRerunTrigger.all().Consider adding a unit test that verifies the default value of
--rerun-triggersto prevent similar regressions in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/cli.py
⏰ Context from checks skipped due to timeout of 90000ms (36)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
- GitHub Check: formatting
|
But why doesn't |
.choices() returns list[str] whereas .all() returns list[RerunTrigger]. See this comment #3491 (comment) |
|
But that is a string when set by the user, so a list[str] should also work as snakemake/src/snakemake/cli.py Line 725 in 17f0d9b and the defaults are not... As for the docs, what I discussed with @johanneskoester at the time, was that the proper way would actually by to fix the parsing of the options by |
|
That’s a good question. User supplied strings are parsed into RerunTrigger.
I guess the defaults are not?
…On Sun, Mar 30, 2025 at 11:20 Filipe G. Vieira ***@***.***> wrote:
But that is a string when set by the user, so a list[str] should also work
as choices, no? Unless it is parsed by
https://github.com/snakemake/snakemake/blob/17f0d9b1edd321a8470d1552ead314532f2b3910/src/snakemake/cli.py#L725
and the defaults are not...
—
Reply to this email directly, view it on GitHub
<#3492 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVQJ4UBAKZ7DWKA5SSZQB32XAYWTAVCNFSM6AAAAAB2APCM6OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRUGY4DINZQGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
[image: fgvieira]*fgvieira* left a comment (snakemake/snakemake#3492)
<#3492 (comment)>
But that is a string when set by the user, so a list[str] should also work
as choices, no? Unless it is parsed by
https://github.com/snakemake/snakemake/blob/17f0d9b1edd321a8470d1552ead314532f2b3910/src/snakemake/cli.py#L725
and the defaults are not...
—
Reply to this email directly, view it on GitHub
<#3492 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKVQJ4UBAKZ7DWKA5SSZQB32XAYWTAVCNFSM6AAAAAB2APCM6OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRUGY4DINZQGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Merging this for now to avoid the regression |
🤖 I have created a release *beep* *boop* --- ## [9.1.4](v9.1.3...v9.1.4) (2025-04-01) ### Bug Fixes * Fix map call in report creation ([#3503](#3503)) ([44754cc](44754cc)) * in the report, do not render toggle labels if there is more than one label eligible for a toggle ([#3502](#3502)) ([3be8ca9](3be8ca9)) * only inform about storage cleanup in case of --verbose mode ([#3494](#3494)) ([62bbbf5](62bbbf5)) * provide causing rule to workflow error during checkpoint handling ([#3499](#3499)) ([b4cbe5d](b4cbe5d)) * rerun-trigger regression ([#3492](#3492)) ([811742d](811742d)) ### Documentation * note about parameter inheritance when using the "use rule with" pattern ([#3500](#3500)) ([8f6a8eb](8f6a8eb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
fixes snakemake#3491 Edit: This is reverting 4430e23, which changed the default for rerun-trigger for the docs. Presumably because `RerunTrigger.all()` gives the actual enum values which don't render nicely. Will need to find a better solution. Any thoughts @fgvieira? <!--Add a description of your PR here--> ### QC <!-- Make sure that you can tick the boxes below. --> * [ ] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [ ] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Updated the CLI default so that all available triggers for job reruns are enabled by default, ensuring a more comprehensive and consistent behavior that aligns with workflow configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [9.1.4](snakemake/snakemake@v9.1.3...v9.1.4) (2025-04-01) ### Bug Fixes * Fix map call in report creation ([snakemake#3503](snakemake#3503)) ([44754cc](snakemake@44754cc)) * in the report, do not render toggle labels if there is more than one label eligible for a toggle ([snakemake#3502](snakemake#3502)) ([3be8ca9](snakemake@3be8ca9)) * only inform about storage cleanup in case of --verbose mode ([snakemake#3494](snakemake#3494)) ([62bbbf5](snakemake@62bbbf5)) * provide causing rule to workflow error during checkpoint handling ([snakemake#3499](snakemake#3499)) ([b4cbe5d](snakemake@b4cbe5d)) * rerun-trigger regression ([snakemake#3492](snakemake#3492)) ([811742d](snakemake@811742d)) ### Documentation * note about parameter inheritance when using the "use rule with" pattern ([snakemake#3500](snakemake#3500)) ([8f6a8eb](snakemake@8f6a8eb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
fixes #3491
Edit: This is reverting 4430e23, which changed the default for rerun-trigger for the docs. Presumably because
RerunTrigger.all()gives the actual enum values which don't render nicely.Will need to find a better solution. Any thoughts @fgvieira?
QC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).Summary by CodeRabbit