Skip to content

fix: only trigger script with CODE label#3707

Merged
johanneskoester merged 4 commits intomainfrom
ignore_script_change
Sep 4, 2025
Merged

fix: only trigger script with CODE label#3707
johanneskoester merged 4 commits intomainfrom
ignore_script_change

Conversation

@Hocnonsense
Copy link
Copy Markdown
Contributor

@Hocnonsense Hocnonsense commented Aug 25, 2025

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_notebook condition should take precedence over metadata checks. However, this condition should still be governed by the presence of RerunTrigger.CODE.

QC

  • 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).

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.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Aug 25, 2025

📝 Walkthrough

Walkthrough

Compute CODE-based reason.code_changed in DAG.update_needrun only when RerunTrigger.CODE is enabled; short-circuit async iteration over job.outputs_older_than_script_or_notebook() to set reason.code_changed on first older output; aggregate persistence-based CODE checks with |=. Tests: adjust urllib import/exception handling and add MTIME-based rerun test.

Changes

Cohort / File(s) Summary
DAG rerun logic
src/snakemake/dag.py
Guard CODE-based code-change detection behind RerunTrigger.CODE; short-circuit async iteration over job.outputs_older_than_script_or_notebook() to set reason.code_changed on first older output; accumulate persistence-based CODE checks with `reason.code_changed
Tests: imports & connectivity
tests/common.py
Replace import urllib with import urllib.request; broaden connectivity error handling to also catch TimeoutError.
Tests: MTIME rerun behavior
tests/tests.py
Rework test_script_python to use a persistent temp dir, record an output mtime, touch/update script mtime, run Snakemake with --rerun-triggers mtime (no cleanup) asserting output mtime unchanged, then rerun without triggers and assert mtime 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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • johanneskoester

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 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 702f5e0 and 455f5e5.

📒 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 (2)
  • src/snakemake/dag.py
  • tests/tests.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). (55)
  • GitHub Check: tests (9, ubuntu-latest, py313)
  • GitHub Check: tests (9, windows-latest, py313)
  • GitHub Check: tests (6, windows-latest, py313)
  • GitHub Check: tests (10, windows-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py312)
  • GitHub Check: tests (10, windows-latest, py312)
  • GitHub Check: tests (7, ubuntu-latest, py313)
  • GitHub Check: tests (7, ubuntu-latest, py311)
  • GitHub Check: tests (6, ubuntu-latest, py311)
  • GitHub Check: tests (8, ubuntu-latest, py313)
  • GitHub Check: tests (9, windows-latest, py312)
  • GitHub Check: tests (6, ubuntu-latest, py313)
  • GitHub Check: tests (6, ubuntu-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py313)
  • GitHub Check: tests (4, macos-latest, py312)
  • GitHub Check: tests (10, ubuntu-latest, py312)
  • GitHub Check: tests (9, ubuntu-latest, py311)
  • GitHub Check: tests (8, windows-latest, py313)
  • GitHub Check: tests (10, ubuntu-latest, py311)
  • GitHub Check: tests (7, windows-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py312)
  • GitHub Check: tests (7, windows-latest, py312)
  • GitHub Check: tests (8, windows-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py311)
  • GitHub Check: tests (9, ubuntu-latest, py312)
  • GitHub Check: tests (5, ubuntu-latest, py313)
  • GitHub Check: tests (8, ubuntu-latest, py311)
  • GitHub Check: tests (5, macos-latest, py312)
  • GitHub Check: tests (6, windows-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py311)
  • GitHub Check: tests (3, windows-latest, py313)
  • GitHub Check: tests (5, windows-latest, py312)
  • GitHub Check: tests (4, ubuntu-latest, py312)
  • GitHub Check: tests (5, windows-latest, py313)
  • GitHub Check: tests (4, windows-latest, py312)
  • GitHub Check: tests (4, windows-latest, py313)
  • GitHub Check: tests (3, macos-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py311)
  • GitHub Check: tests (4, ubuntu-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py313)
  • GitHub Check: tests (1, ubuntu-latest, py312)
  • GitHub Check: tests (1, macos-latest, py313)
  • GitHub Check: tests (3, windows-latest, py312)
  • GitHub Check: tests (3, ubuntu-latest, py312)
  • GitHub Check: tests (1, ubuntu-latest, py311)
  • GitHub Check: tests (2, windows-latest, py312)
  • GitHub Check: tests (2, ubuntu-latest, py311)
  • GitHub Check: tests (3, ubuntu-latest, py313)
  • GitHub Check: tests (2, windows-latest, py313)
  • GitHub Check: tests (2, macos-latest, py313)
  • GitHub Check: tests (2, ubuntu-latest, py313)
  • GitHub Check: tests (1, windows-latest, py312)
  • GitHub Check: tests (1, windows-latest, py313)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ignore_script_change

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
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 (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.

📥 Commits

Reviewing files that changed from the base of the PR and between 26fcd38 and c2dfa12.

📒 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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

Files:

  • tests/common.py
  • src/snakemake/dag.py
  • tests/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.request matches the references to urllib.request.urlopen and urllib.request.URLError below. Good catch.

@Hocnonsense Hocnonsense force-pushed the ignore_script_change branch 2 times, most recently from bd2c611 to 702f5e0 Compare August 25, 2025 18:49
Copy link
Copy Markdown
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: 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 14a1f3c and bd2c611.

📒 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 the self argument of methods.
Do not suggest type annotation of the cls argument of classmethods.
Do not suggest return type annotation if a function or method does not contain a return statement.

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: All outputs_older_than_script_or_notebook checks are properly scoped to CODE or for reporting

  • The only other occurrence of
    [f async for f in job.outputs_older_than_script_or_notebook()]
    at src/snakemake/dag.py:3249 lives inside the 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.

@Hocnonsense Hocnonsense force-pushed the ignore_script_change branch from 702f5e0 to c03fbff Compare August 25, 2025 18:52
@Hocnonsense
Copy link
Copy Markdown
Contributor Author

So sad for many fails when pixi install

@jasonho1308
Copy link
Copy Markdown

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.

@johanneskoester johanneskoester merged commit 2d01f8c into main Sep 4, 2025
80 checks passed
@johanneskoester johanneskoester deleted the ignore_script_change branch September 4, 2025 18:53
@johanneskoester
Copy link
Copy Markdown
Contributor

Thanks a lot!

johanneskoester pushed a commit that referenced this pull request Sep 5, 2025
🤖 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).
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
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 -->
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
🤖 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).
@Hocnonsense Hocnonsense self-assigned this Mar 18, 2026
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.

3 participants