fix: only trigger script with CODE label#3707
Conversation
📝 WalkthroughWalkthroughCompute CODE-based Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WF as Workflow
participant DAG as DAG.update_needrun
participant RT as RerunTriggers
participant P as Persistence
participant R as Reason
WF->>DAG: evaluate need-run for job
DAG->>RT: check enabled rerun triggers
alt CODE enabled
DAG->>DAG: async iterate outputs_older_than_script_or_notebook() (short-circuit on first older)
DAG->>R: set reason.code_changed = True on first older output
DAG->>P: call persistence.code_changed(job)
P-->>DAG: boolean results
DAG->>R: reason.code_changed |= any(persistence results)
else CODE not enabled
DAG->>R: skip CODE-based checks
end
DAG-->>WF: return need-run decisions with reasons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (55)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/snakemake/dag.py (1)
1450-1458: Gate script/notebook mtime-based code-change detection behind RerunTrigger.CODE; minor efficiency and comment clarity nits.The guard fixes unintended reruns and matches the PR goal. Two small follow-ups:
- Avoid materializing a list for the async generator; short-circuit on first match.
- Tighten the comment for clarity.
Apply:
- if RerunTrigger.CODE in self.workflow.rerun_triggers: - # job metadata can be missed, but the separate scripts don't. - # but it should not work if unwanted - reason.code_changed = any( - [ - f - async for f in job.outputs_older_than_script_or_notebook() - ] - ) + if RerunTrigger.CODE in self.workflow.rerun_triggers: + # Prefer script/notebook mtime over metadata when CODE trigger is enabled. + # Short-circuit on first older output to avoid building a list. + reason.code_changed = False + async for _ in job.outputs_older_than_script_or_notebook(): + reason.code_changed = True + break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/snakemake/dag.py(1 hunks)tests/common.py(1 hunks)tests/tests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
tests/common.pysrc/snakemake/dag.pytests/tests.py
🧠 Learnings (2)
📚 Learning: 2024-11-12T12:08:20.342Z
Learnt from: johanneskoester
PR: snakemake/snakemake#3148
File: snakemake/dag.py:1332-1336
Timestamp: 2024-11-12T12:08:20.342Z
Learning: In `snakemake/dag.py`, when code is outdated and will disappear upon resolving merge conflicts, avoid making code review suggestions on that code.
Applied to files:
src/snakemake/dag.py
📚 Learning: 2024-10-14T09:42:11.571Z
Learnt from: johanneskoester
PR: snakemake/snakemake#3140
File: snakemake/dag.py:1308-1308
Timestamp: 2024-10-14T09:42:11.571Z
Learning: In `snakemake/dag.py`, avoid flagging missing lines or indentation issues when there is no clear syntax or logical error to prevent false positives.
Applied to files:
src/snakemake/dag.py
🧬 Code graph analysis (2)
src/snakemake/dag.py (4)
src/snakemake/settings/enums.py (1)
RerunTrigger(5-10)src/snakemake/workflow.py (1)
rerun_triggers(524-526)src/snakemake/persistence.py (1)
code_changed(462-464)src/snakemake/jobs.py (1)
outputs_older_than_script_or_notebook(402-417)
tests/tests.py (3)
tests/common.py (2)
run(152-496)dpath(33-36)src/snakemake/workflow.py (2)
run(2301-2302)rerun_triggers(524-526)src/snakemake/settings/enums.py (1)
RerunTrigger(5-10)
⏰ 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). (54)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-latest, py313)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (9, windows-latest, py313)
- GitHub Check: tests (4, windows-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, windows-latest, py313)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-latest, py313)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (8, macos-latest, py312)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py313)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (1)
tests/common.py (1)
16-16: Import aligns with usage of urllib.request symbols.Switching to
import urllib.requestmatches the references tourllib.request.urlopenandurllib.request.URLErrorbelow. Good catch.
bd2c611 to
702f5e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/snakemake/dag.py (3)
1451-1453: Tighten the comment to match behavior.Current phrasing is slightly ambiguous. Suggest making it explicit that the mtime check only runs when CODE is enabled.
Apply this diff:
- # Always check script/notebook mtime unless CODE trigger is disabled. - # Short-circuit on first older output to avoid building a list. + # Check script/notebook mtime only when RerunTrigger.CODE is enabled. + # Short-circuit on first older output to avoid building a list.
1473-1475: Avoid unnecessary persistence work; short-circuit when mtime already flagged code changes.If the earlier script/notebook mtime check already set reason.code_changed=True, we can skip evaluating persistence.code_changed(job). Minor perf win on large DAGs.
Apply this diff:
- if RerunTrigger.CODE in self.workflow.rerun_triggers: - reason.code_changed |= any( - self.workflow.persistence.code_changed(job) - ) + if RerunTrigger.CODE in self.workflow.rerun_triggers: + if not reason.code_changed: + reason.code_changed = any( + self.workflow.persistence.code_changed(job) + )
1454-1458: Optional: use anext(...) for clarity if supported Python >= 3.10.Using anext to grab a single element reads a bit clearer and avoids entering a loop. Only worth it if the project’s minimum Python version permits.
Proposed alternative (replace the loop body):
- reason.code_changed = False - async for ( - _ - ) in job.outputs_older_than_script_or_notebook(): - reason.code_changed = True - break + reason.code_changed = ( + await anext( + job.outputs_older_than_script_or_notebook(), + None, + ) + is not None + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/snakemake/dag.py(1 hunks)tests/tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tests.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.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.
Files:
src/snakemake/dag.py
🧠 Learnings (1)
📚 Learning: 2024-11-12T12:08:20.342Z
Learnt from: johanneskoester
PR: snakemake/snakemake#3148
File: snakemake/dag.py:1332-1336
Timestamp: 2024-11-12T12:08:20.342Z
Learning: In `snakemake/dag.py`, when code is outdated and will disappear upon resolving merge conflicts, avoid making code review suggestions on that code.
Applied to files:
src/snakemake/dag.py
🧬 Code graph analysis (1)
src/snakemake/dag.py (4)
src/snakemake/settings/enums.py (1)
RerunTrigger(5-10)src/snakemake/workflow.py (1)
rerun_triggers(524-526)src/snakemake/persistence.py (1)
code_changed(462-464)src/snakemake/jobs.py (1)
outputs_older_than_script_or_notebook(402-417)
⏰ 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). (55)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (7, windows-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (8, windows-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py313)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, windows-latest, py313)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, windows-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: apidocs
🔇 Additional comments (2)
src/snakemake/dag.py (2)
1450-1458: Correctly gate script/notebook mtime checks behind RerunTrigger.CODE and short-circuit on first hit.This aligns behavior with the PR goal: code-mtime-induced reruns only when CODE is enabled. The async iteration with early break avoids constructing an intermediate list. LGTM.
1450-1475: Alloutputs_older_than_script_or_notebookchecks are properly scoped to CODE or for reporting
- The only other occurrence of
at src/snakemake/dag.py:3249 lives inside the[f async for f in job.outputs_older_than_script_or_notebook()]DAG.changed()method, which is used only to assemble a list of changed files for CLI/status output—not to decide reruns.- Both assignments to
reason.code_changed(around lines 1453–1458 and 1473–1475) are guarded by
if RerunTrigger.CODE in self.workflow.rerun_triggers.- The only other call to
persistence.code_changed(job)at line 2950 is in the status‐message builder and does not affect rerun logic.No unconditional script/notebook‐mtime checks remain that would trigger reruns when CODE is disabled. Everything is correctly gated.
702f5e0 to
c03fbff
Compare
|
So sad for many fails when pixi install |
|
Your approach is better than mine in #3706 as it handled ignoring code change when the file don't have metadata as well! I am gonna close my pull request. |
|
Thanks a lot! |
🤖 I have created a release *beep* *boop* --- ## [9.11.0](v9.10.1...v9.11.0) (2025-09-05) ### Features * add landing page to the report containing custom metadata defined with a YTE yaml template ([#3452](#3452)) ([c754d80](c754d80)) * Issue a warning when unsupported characters are used in wildcard names ([#1587](#1587)) ([fa57355](fa57355)) ### Bug Fixes * avoid checking output files in immediate-submit mode ([#3569](#3569)) ([58c42cf](58c42cf)) * clarify --keep-going flag help text to distinguish runtime vs DAG construction errors ([#3705](#3705)) ([a3a8ef4](a3a8ef4)) * enable docstring assignment in `use rule ... with:` block ([#3710](#3710)) ([2191962](2191962)) * fix missing attribute error in greedy scheduler settings when using touch, dryrun or immediate submit. ([6471004](6471004)) * only trigger script with CODE label ([#3707](#3707)) ([2d01f8c](2d01f8c)) * parser.py incorrectly parsing multiline f-strings ([#3701](#3701)) ([06ece76](06ece76)) * parsing ordinary yaml strings ([#3506](#3506)) ([ddf334e](ddf334e)) * remove temp files when using checkpoints ([#3716](#3716)) ([5ff3e20](5ff3e20)) * Restore python rules changes triggering reruns. (GH: [#3213](#3213)) ([#3485](#3485)) ([2f663be](2f663be)) * unit testing ([#3680](#3680)) ([06ba7e6](06ba7e6)) * use queue_input_wait_time when updating queue input jobs ([#3712](#3712)) ([a945a19](a945a19)) ### Documentation * output files within output directories is an error ([#2848](#2848)) ([#2913](#2913)) ([37272e5](37272e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR is inspired by snakemake#3706, resolves issues snakemake#3456 and snakemake#3474, and addresses the concern raised in snakemake#3148 (comment) In brief, commit e502312 made changes so that the modification time (mtime) of script and notebook files no longer triggers a rerun of the workflow, later, commit e8a0b83 reversed this, making mtime changes always trigger reruns -- even when RerunTrigger.CODE is not specified in `--rerun-triggers`. This created an undesirable situation: legitimate and common scenarios (e.g., copying scripts or pulling them from a Git repository) could unnecessarily trigger reruns — as reported in snakemake#3474. To make the behavior more intuitive, the `outputs_older_than_script_or_notebook` condition should take precedence over metadata checks. However, this condition should still be governed by the presence of `RerunTrigger.CODE`. ### QC <!-- Make sure that you can tick the boxes below. --> * [x] 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 * **Bug Fixes** * Code-change reruns now only run when the CODE rerun trigger is enabled; code-change detection correctly accumulates signals from multiple sources to avoid missed or spurious reruns. * **Tests** * Added an MTIME-triggered rerun test validating that outputs retain timestamps when only input mtimes change. * **Chores** * Fixed a test import path and broadened connectivity error handling to include timeouts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [9.11.0](snakemake/snakemake@v9.10.1...v9.11.0) (2025-09-05) ### Features * add landing page to the report containing custom metadata defined with a YTE yaml template ([snakemake#3452](snakemake#3452)) ([c754d80](snakemake@c754d80)) * Issue a warning when unsupported characters are used in wildcard names ([snakemake#1587](snakemake#1587)) ([fa57355](snakemake@fa57355)) ### Bug Fixes * avoid checking output files in immediate-submit mode ([snakemake#3569](snakemake#3569)) ([58c42cf](snakemake@58c42cf)) * clarify --keep-going flag help text to distinguish runtime vs DAG construction errors ([snakemake#3705](snakemake#3705)) ([a3a8ef4](snakemake@a3a8ef4)) * enable docstring assignment in `use rule ... with:` block ([snakemake#3710](snakemake#3710)) ([2191962](snakemake@2191962)) * fix missing attribute error in greedy scheduler settings when using touch, dryrun or immediate submit. ([6471004](snakemake@6471004)) * only trigger script with CODE label ([snakemake#3707](snakemake#3707)) ([2d01f8c](snakemake@2d01f8c)) * parser.py incorrectly parsing multiline f-strings ([snakemake#3701](snakemake#3701)) ([06ece76](snakemake@06ece76)) * parsing ordinary yaml strings ([snakemake#3506](snakemake#3506)) ([ddf334e](snakemake@ddf334e)) * remove temp files when using checkpoints ([snakemake#3716](snakemake#3716)) ([5ff3e20](snakemake@5ff3e20)) * Restore python rules changes triggering reruns. (GH: [snakemake#3213](snakemake#3213)) ([snakemake#3485](snakemake#3485)) ([2f663be](snakemake@2f663be)) * unit testing ([snakemake#3680](snakemake#3680)) ([06ba7e6](snakemake@06ba7e6)) * use queue_input_wait_time when updating queue input jobs ([snakemake#3712](snakemake#3712)) ([a945a19](snakemake@a945a19)) ### Documentation * output files within output directories is an error ([snakemake#2848](snakemake#2848)) ([snakemake#2913](snakemake#2913)) ([37272e5](snakemake@37272e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This PR is inspired by #3706, resolves issues #3456 and #3474, and addresses the concern raised in #3148 (comment)_
In brief, commit e502312 made changes so that the modification time (mtime) of script and notebook files no longer triggers a rerun of the workflow, later, commit e8a0b83 reversed this, making mtime changes always trigger reruns -- even when RerunTrigger.CODE is not specified in
--rerun-triggers.This created an undesirable situation: legitimate and common scenarios (e.g., copying scripts or pulling them from a Git repository) could unnecessarily trigger reruns — as reported in #3474.
To make the behavior more intuitive, the
outputs_older_than_script_or_notebookcondition should take precedence over metadata checks. However, this condition should still be governed by the presence ofRerunTrigger.CODE.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
Bug Fixes
Tests
Chores