fix: remove erroneous assert in the scheduler (is_main_process) by johanneskoester · Pull Request #3881 · snakemake/snakemake

johanneskoester · 2025-12-15T12:54:25Z

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
- Enhanced job scheduling for remote execution environments with improved handling of shared filesystem configurations and more flexible storage input retrieval logic across different execution scenarios.

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

coderabbitai · 2025-12-15T12:54:38Z

📝 Walkthrough

Walkthrough

The change modifies the control flow in job scheduler logic for retrieving storage inputs for remote jobs. It introduces a condition variable to distinguish between shared filesystem usage and remote execution scenarios, removes an assertion about main process execution, and reworks the conditional branching for storage input retrieval based on filesystem type.

Changes

Cohort / File(s)	Summary
Job scheduler storage input retrieval `src/snakemake/scheduling/job_scheduler.py`	Added `is_shared_fs` variable to detect shared filesystem usage; removed main process assertion; reworked storage input retrieval conditions to handle shared filesystem and remote execution branches separately

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Key area of focus: Verify the logic change correctly handles both shared filesystem and remote execution paths for storage input retrieval
Understand the implications of removing the main process assertion and how the new conditions ensure correctness
Confirm the is_shared_fs detection properly identifies STORAGE_LOCAL_COPIES usage

Possibly related PRs

PR #3850: Introduced the assertion for main process execution in storage input retrieval logic that is now being reworked and removed in this PR

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: removing an erroneous assert related to is_main_process in the scheduler, which aligns with the summary showing removal of the assertion.
Description check	✅ Passed	The PR description follows the required template with all QC checklist items completed, though it lacks detailed explanation of the changes and their rationale.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ 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 fix/wrong_assert

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/snakemake/scheduling/job_scheduler.py (1)
368-380: Logic change correctly removes the erroneous assertion.

The conditional logic properly handles both execution modes:

Main process with shared FS: retrieves storage inputs once on shared storage

Remote execution without shared FS: retrieves storage inputs in the remote context

The fix appropriately removes what was likely an overly restrictive assertion about running only in the main process.

The comment at lines 372-374 could be updated to better reflect the dual-path logic:
-                            # Retrieve storage inputs for remote jobs, as storage local copies are handled
-                            # via a shared filesystem.
-                            # If local copies are not shared, they will be downloaded in the remote job.
+                            # Retrieve storage inputs for remote jobs based on execution mode:
+                            # - In main process with shared FS: download once on shared storage for all remote jobs
+                            # - In remote execution without shared FS: download in this remote instance
+                            # Otherwise, remote jobs will download their own inputs as needed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9598655 and d5bee2b.

📒 Files selected for processing (1)

src/snakemake/scheduling/job_scheduler.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:

src/snakemake/scheduling/job_scheduler.py

🧬 Code graph analysis (1)

src/snakemake/scheduling/job_scheduler.py (1)

src/snakemake/workflow.py (3)

dryrun (419-423)

is_main_process (273-274)

remote_exec (458-459)

⏰ 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). (47)

GitHub Check: tests (9, windows-2022, py313)
GitHub Check: tests (10, windows-2022, py313)
GitHub Check: tests (10, ubuntu-latest, py313)
GitHub Check: tests (7, ubuntu-latest, py313)
GitHub Check: tests (4, ubuntu-latest, py312)
GitHub Check: tests (9, ubuntu-latest, py311)
GitHub Check: tests (10, ubuntu-latest, py312)
GitHub Check: tests (9, ubuntu-latest, py312)
GitHub Check: tests (8, ubuntu-latest, py312)
GitHub Check: tests (9, ubuntu-latest, py313)
GitHub Check: tests (4, ubuntu-latest, py313)
GitHub Check: tests (6, windows-2022, py313)
GitHub Check: tests (8, windows-2022, py313)
GitHub Check: tests (7, ubuntu-latest, py311)
GitHub Check: tests (6, ubuntu-latest, py313)
GitHub Check: tests (8, ubuntu-latest, py311)
GitHub Check: tests (8, macos-latest, py313)
GitHub Check: tests (7, windows-2022, py313)
GitHub Check: tests (1, ubuntu-latest, py313)
GitHub Check: tests (8, ubuntu-latest, py313)
GitHub Check: tests (5, windows-2022, py313)
GitHub Check: tests (6, ubuntu-latest, py311)
GitHub Check: tests (7, macos-latest, py313)
GitHub Check: tests (10, ubuntu-latest, py311)
GitHub Check: tests (7, ubuntu-latest, py312)
GitHub Check: tests (3, windows-2022, py313)
GitHub Check: tests (6, ubuntu-latest, py312)
GitHub Check: tests (5, ubuntu-latest, py313)
GitHub Check: tests (5, ubuntu-latest, py312)
GitHub Check: tests (4, windows-2022, py313)
GitHub Check: tests (4, ubuntu-latest, py311)
GitHub Check: tests (1, ubuntu-latest, py311)
GitHub Check: tests (6, macos-latest, py313)
GitHub Check: tests (2, ubuntu-latest, py311)
GitHub Check: tests (3, ubuntu-latest, py313)
GitHub Check: tests (5, ubuntu-latest, py311)
GitHub Check: tests (1, windows-2022, py313)
GitHub Check: tests (2, ubuntu-latest, py313)
GitHub Check: tests (3, ubuntu-latest, py312)
GitHub Check: tests (3, ubuntu-latest, py311)
GitHub Check: tests (2, ubuntu-latest, py312)
GitHub Check: tests (2, windows-2022, py313)
GitHub Check: tests (2, macos-latest, py313)
GitHub Check: tests (1, ubuntu-latest, py312)
GitHub Check: tests (1, macos-latest, py313)
GitHub Check: docs
GitHub Check: apidocs

🔇 Additional comments (1)

src/snakemake/scheduling/job_scheduler.py (1)

362-365: LGTM!

The is_shared_fs flag correctly identifies whether storage local copies are handled via a shared filesystem, enabling the conditional logic below to distinguish between shared and non-shared filesystem execution modes.

🤖 I have created a release *beep* *boop* --- ## [9.14.5](v9.14.4...v9.14.5) (2025-12-15) ### Bug Fixes * remove erroneous assert in the scheduler (is_main_process) ([#3881](#3881)) ([d5acdd1](d5acdd1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…emake#3881)  ### QC  * [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).  ## Summary by CodeRabbit * **Bug Fixes** * Enhanced job scheduling for remote execution environments with improved handling of shared filesystem configurations and more flexible storage input retrieval logic across different execution scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>

🤖 I have created a release *beep* *boop* --- ## [9.14.5](snakemake/snakemake@v9.14.4...v9.14.5) (2025-12-15) ### Bug Fixes * remove erroneous assert in the scheduler (is_main_process) ([snakemake#3881](snakemake#3881)) ([d5acdd1](snakemake@d5acdd1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

fix: remove erroneous assert in the scheduler (is_main_process)

d5bee2b

coderabbitai Bot reviewed Dec 15, 2025

View reviewed changes

johanneskoester merged commit d5acdd1 into main Dec 15, 2025
111 of 113 checks passed

johanneskoester deleted the fix/wrong_assert branch December 15, 2025 13:50

snakemake-bot mentioned this pull request Dec 15, 2025

chore(main): release 9.14.5 #3882

Merged

This was referenced Dec 22, 2025

assert self.workflow.is_main_process AssertionError #3874

Closed

Job submission with scheduler broken by #3850 #3853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove erroneous assert in the scheduler (is_main_process)#3881

fix: remove erroneous assert in the scheduler (is_main_process)#3881
johanneskoester merged 1 commit intomainfrom
fix/wrong_assert

johanneskoester commented Dec 15, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Dec 15, 2025 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

johanneskoester commented Dec 15, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

QC

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

johanneskoester commented Dec 15, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Dec 15, 2025 •

edited

Loading