Skip to content

fix: fix issues with cyclic dependencies when using the update and before_update flag#3857

Merged
johanneskoester merged 8 commits intomainfrom
test_cyclic_dependency
Dec 8, 2025
Merged

fix: fix issues with cyclic dependencies when using the update and before_update flag#3857
johanneskoester merged 8 commits intomainfrom
test_cyclic_dependency

Conversation

@FelixMoelder
Copy link
Copy Markdown
Contributor

@FelixMoelder FelixMoelder commented Dec 2, 2025 •
edited by coderabbitai Bot
Loading

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

    • Cycle detection now ignores cases involving before_update flags to avoid false-cycle reports.
    • Flag precedence corrected so update jobs take precedence over before_update when both apply.

  • Tests

    • Added tests and fixtures covering cyclic dependency scenarios, including a forced-rerun case, to validate the updated behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 2, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

Cycle detection and update-flag handling in Snakemake's DAG were changed: cycles involving files flagged with before_update are ignored in strict ordering checks, and before_update assignments are recorded only after update jobs are determined and only when no update job exists for that file.

Changes

Cohort / File(s) Summary
Core cycle detection logic
src/snakemake/dag.py		 is_strictly_higher_ordered() now ignores cycle-detection failures when the involved file is flagged with before_update. handle_update_flags() computes update_jobs first and records before_update candidates only if no corresponding update job exists, ensuring update precedence.
Test workflows
tests/test_cyclic_dependency_single/Snakefile, tests/test_cyclic_dependency_split/Snakefile		 Added two Snakefiles exercising before_update/update interactions: a single-rule scenario (forced rerun) and a split-rule scenario demonstrating chained before_update → update behavior.
Test fixtures
tests/test_cyclic_dependency_single/expected-results/test.txt, tests/test_cyclic_dependency_split/expected-results/test.txt		 Added expected-result files containing the literal string "test" for the new tests.
Test harness
tests/tests.py		 Added test_cyclic_dependency_single() and test_cyclic_dependency_split() to run the new scenarios (single test uses forceall=True).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Review src/snakemake/dag.py:is_strictly_higher_ordered() for edge cases where ignoring cycles might allow invalid execution orders.
  • Verify src/snakemake/dag.py:handle_update_flags() correctly computes update_jobs before assigning before_update and that precedence between update and before_update is deterministic.
  • Run the two new tests in tests/ to ensure they exercise the intended code paths and behavior.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.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
Description check ✅ Passed The PR description follows the template, with both QC checklist items completed and marked as checked, confirming test coverage and documentation status.
Title check ✅ Passed The title accurately describes the main change: fixing cyclic dependency issues related to update and before_update flags in the codebase.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch test_cyclic_dependency

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Dec 2, 2025

Please format your code with pixi run format

@johanneskoester johanneskoester marked this pull request as ready for review December 7, 2025 15:48
@johanneskoester johanneskoester self-requested a review as a code owner December 7, 2025 15:48
@johanneskoester
Copy link
Copy Markdown
Contributor

johanneskoester commented Dec 7, 2025 •
edited
Loading

Great work with the two testcases. They made it super easy to find the issue and will prevent it in the future.

@johanneskoester johanneskoester changed the title test: test cycling dependencies fix: fix issues with cyclic dependencies when using the update and before_update flag Dec 8, 2025
@johanneskoester johanneskoester merged commit 21cc94e into main Dec 8, 2025
59 checks passed
@johanneskoester johanneskoester deleted the test_cyclic_dependency branch December 8, 2025 10:55
@snakemake-bot snakemake-bot mentioned this pull request Dec 8, 2025
Merged
johanneskoester pushed a commit that referenced this pull request Dec 8, 2025
@snakemake-bot
chore(main): release 9.14.3 (#3868)
02c1198 
🤖 I have created a release *beep* *boop*
---


##
[9.14.3](v9.14.2...v9.14.3)
(2025-12-08)


### Bug Fixes

* ensure that queue input jobs wait if the queue is still empty upon job
creation ([#3866](#3866))
([555ab6a](555ab6a))
* fix issues with cyclic dependencies when using the update and
before_update flag
([#3857](#3857))
([21cc94e](21cc94e))

---
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
@FelixMoelder @johanneskoester @kjohnsen
fix: fix issues with cyclic dependencies when using the update and be…
407316c 
…fore_update flag (snakemake#3857)

<!--Add a description of your PR here-->

### 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.
* [x] 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**
* Cycle detection now ignores cases involving before_update flags to
avoid false-cycle reports.
* Flag precedence corrected so update jobs take precedence over
before_update when both apply.

* **Tests**
* Added tests and fixtures covering cyclic dependency scenarios,
including a forced-rerun case, to validate the updated behavior.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Johannes Koester <[email protected]>
kjohnsen pushed a commit to kjohnsen/snakemake that referenced this pull request Dec 15, 2025
@snakemake-bot @kjohnsen
chore(main): release 9.14.3 (snakemake#3868)
3353053 
🤖 I have created a release *beep* *boop*
---


##
[9.14.3](snakemake/snakemake@v9.14.2...v9.14.3)
(2025-12-08)


### Bug Fixes

* ensure that queue input jobs wait if the queue is still empty upon job
creation ([snakemake#3866](snakemake#3866))
([555ab6a](snakemake@555ab6a))
* fix issues with cyclic dependencies when using the update and
before_update flag
([snakemake#3857](snakemake#3857))
([21cc94e](snakemake@21cc94e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johanneskoester johanneskoester johanneskoester approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@FelixMoelder @johanneskoester