fix: setup_logfile in setup logger to ensure logfile handler is added#3486
Merged
johanneskoester merged 9 commits intosnakemake:mainfrom Mar 26, 2025
Merged
fix: setup_logfile in setup logger to ensure logfile handler is added#3486johanneskoester merged 9 commits intosnakemake:mainfrom
johanneskoester merged 9 commits intosnakemake:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes modify the logging behavior by removing the log file setup call from the workflow execution method in the API module and integrating the log file setup into the logger manager. The logger manager now conditionally creates a log file during setup and improves the cleanup process by detaching handlers before closing the logger. Additionally, a new test for log file verification has been added, and error handling for temporary directory cleanup in tests has been enhanced to address potential issues on Windows platforms. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoggerManager
participant DAGApi
User->>DAGApi: execute_workflow()
DAGApi-->>LoggerManager: setup_logfile()
LoggerManager-->>DAGApi: Log file setup complete
DAGApi-->>User: Workflow execution complete
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 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...
⏰ Context from checks skipped due to timeout of 90000ms (34)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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 (
|
This reverts commit 4b93318.
This is the default behavior of common.run, but this test opts out of run's cleanup, so we need to fix the rmtree call here
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_logfile/Snakefile (1)
1-11: LGTM! Clean test workflow for log file verification.This Snakefile creates a simple workflow with two rules that will generate 5 output files, which is appropriate for testing the log file functionality that's being fixed in this PR.
One minor suggestion: you might want to consider adding a directory creation step for the "results" directory to ensure the workflow doesn't fail if the directory doesn't exist. This could be done either:
- Within the Snakefile using
shell: "mkdir -p results && echo {wildcards.i} > {output}"- Or in the Python test that uses this Snakefile
However, if the test framework automatically handles this, the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_logfile/Snakefile(1 hunks)tests/tests.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tests.py
⏰ Context from checks skipped due to timeout of 90000ms (34)
- 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, 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
fgvieira
approved these changes
Mar 26, 2025
johanneskoester
approved these changes
Mar 26, 2025
johanneskoester
pushed a commit
that referenced
this pull request
Mar 28, 2025
🤖 I have created a release *beep* *boop* --- ## [9.1.3](v9.1.2...v9.1.3) (2025-03-26) ### Bug Fixes * pass correct local storage prefix in case an executor spawns an inner job ([#3488](#3488)) ([1454ca1](1454ca1)) * setup_logfile in setup logger to ensure logfile handler is added ([#3486](#3486)) ([2ed13fc](2ed13fc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 tasks
kjohnsen
pushed a commit
to kjohnsen/snakemake
that referenced
this pull request
Dec 15, 2025
…snakemake#3486) Fixes oversight that resulted in empty log files. Will add a test soon. ### 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 - **New Features** - Introduced a new test for verifying the existence and content of log files generated during execution. - Added a new workflow definition with rules for generating output files. - **Refactor** - Enhanced the logging system to ensure log files are created during setup and improved the cleanup process by detaching handlers. - **Bug Fixes** - Improved error handling during temporary directory cleanup, ensuring resources are properly released on Windows systems. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Johannes Köster <[email protected]>
kjohnsen
pushed a commit
to kjohnsen/snakemake
that referenced
this pull request
Dec 15, 2025
🤖 I have created a release *beep* *boop* --- ## [9.1.3](snakemake/snakemake@v9.1.2...v9.1.3) (2025-03-26) ### Bug Fixes * pass correct local storage prefix in case an executor spawns an inner job ([snakemake#3488](snakemake#3488)) ([1454ca1](snakemake@1454ca1)) * setup_logfile in setup logger to ensure logfile handler is added ([snakemake#3486](snakemake#3486)) ([2ed13fc](snakemake@2ed13fc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes oversight that resulted in empty log files. Will add a test soon.
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
New Features
Refactor
Bug Fixes