fix: logging refinements#3571
Conversation
…se queue for plugin handlers
📝 WalkthroughWalkthroughLogger initialization and configuration are centralized: SnakemakeApi now sets up logging at construction and when a workflow starts; LoggerManager and handlers were simplified (removed mode-based branching); OutputSettings gained logging-related fields; CLI creation of OutputSettings was extracted; comprehensive logging tests and test Snakefiles were added; an obsolete test was removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SnakemakeApi
participant LoggerManager
participant WorkflowApi
User->>CLI: supply args
CLI->>SnakemakeApi: instantiate with OutputSettings (via create_output_settings)
SnakemakeApi->>SnakemakeApi: __post_init__ -> setup_logger()
SnakemakeApi->>LoggerManager: setup(handlers, settings)
User->>SnakemakeApi: start workflow(workdir)
SnakemakeApi->>LoggerManager: setup_logfile(workdir)
SnakemakeApi->>WorkflowApi: instantiate WorkflowApi (logging already configured)
WorkflowApi->>LoggerManager: emit events/logs (handled by configured handlers)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/snakemake/logging.py (4)
408-413: Use a single-line boolean return for the new filter ruleThe added guard works, but you can collapse the last two lines into one boolean expression (ruff SIM103).
Keeps the method crisp and avoids an additional branch.- if event == LogEvent.WORKFLOW_STARTED: - return False - - return True + return event != LogEvent.WORKFLOW_STARTED🧰 Tools
🪛 Ruff (0.8.2)
410-413: Return the condition
not event == LogEvent.WORKFLOW_STARTEDdirectlyReplace with
return not event == LogEvent.WORKFLOW_STARTED(SIM103)
565-572: Inconsistent log level for the default stream handler in REMOTE mode
ExecMode.SUBPROCESSlimits the default handler toERROR, butExecMode.REMOTEinstalls the same handler without level restriction.
If REMOTE workers are noisy this will flood the parent process.
Consider aligning the behaviour:- elif self.mode == ExecMode.REMOTE: - self.logger.addHandler(self._default_streamhandler()) + elif self.mode == ExecMode.REMOTE: + handler = self._default_streamhandler() + handler.setLevel(logging.ERROR) # mirror SUBPROCESS default + self.logger.addHandler(handler)
588-601: Minor: store the queue on the listener instead of the manager
self._queueis created only for wiring theQueueListener.
Unless the queue is accessed elsewhere, keeping it as a private attribute of the listener avoids an extra attribute on the manager:- self._queue = Queue(-1) - self.queue_listener = logging.handlers.QueueListener( - self._queue, + _queue = Queue(-1) + self.queue_listener = logging.handlers.QueueListener( + _queue, *other_handlers, respect_handler_level=True, ) self.queue_listener.start() - self.logger.addHandler(logging.handlers.QueueHandler(self._queue)) + self.logger.addHandler(logging.handlers.QueueHandler(_queue))
665-684: Path handling insetup_logfilecan be simplified & made pathlib-safe
os.path.joinon aPathobject may raise on some platforms.utils.makedirs()already exists for this purpose.- Mixing string &
Pathhurts readability.-from snakemake.utils import makedirs -... - if workdir: - logdir = os.path.join(workdir, ".snakemake", "log") - else: - logdir = os.path.join(".snakemake", "log") - try: - os.makedirs(logdir, exist_ok=True) - logfile = os.path.abspath( - os.path.join( - logdir, - datetime.datetime.now().isoformat().replace(":", "") - + ".snakemake.log", - ) - ) + from pathlib import Path + logdir_path = ( + Path(workdir) if workdir is not None else Path(".") + ) / ".snakemake" / "log" + try: + logdir_path.mkdir(parents=True, exist_ok=True) + logfile = ( + logdir_path + / f"{datetime.datetime.now().isoformat().replace(':','')}.snakemake.log" + ) handler = self._default_filehandler(logfile) self.logger.addHandler(handler) self.logfile_handlers[handler] = str(logfile)src/snakemake/settings/types.py (1)
398-406:OutputSettings.modeduplicatesWorkflowSettings.exec_modeBoth dataclasses now carry an execution-mode field (
WorkflowSettings.exec_modeandOutputSettings.mode).
Having two sources of truth risks divergence, e.g. if one is mutated without updating the other.Consider:
- Removing one of the fields, or
- Making one a
@propertyview of the other, or- Adding a validation check in
__post_init__to assert they match.This will prevent subtle bugs where the wrong instance is consulted.
tests/tests.py (2)
48-52: More helpful failure diagnostics for multi-file scenariosYou already improved the error message with the directory path (👍).
Consider also logging the contents ofglob.glob(...)when the list is empty – this makes triaging CI failures quicker (e.g. mis-configured work-dir vs. wrong filename pattern).- assert log_files, "No log files found" + assert log_files, f"No log files found in {log_dir}. Contents: {os.listdir(log_dir)}"
67-90: New test looks solid but can be tightened & sped up
- The
stmtslist is static; iterate once instead of sorting files every time.glob.glob+sort+[0]can be replaced withmax(..., key=os.path.getmtime)which avoids a Python-level sort of the whole list.- log_files = glob.glob(os.path.join(log_dir, "*.snakemake.log")) - assert log_files, "No log files found" - - log_files.sort(key=os.path.getmtime, reverse=True) - latest_log = log_files[0] + log_files = glob.glob(os.path.join(log_dir, "*.snakemake.log")) + assert log_files, f"No log files found in {log_dir}" + + latest_log = max(log_files, key=os.path.getmtime)These micro-changes make the test fractionally faster and the intent a bit clearer.
Everything else—including cleanup—looks correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/snakemake/api.py(4 hunks)src/snakemake/cli.py(1 hunks)src/snakemake/logging.py(4 hunks)src/snakemake/settings/types.py(1 hunks)src/snakemake/workflow.py(1 hunks)tests/test_workflow_logger/Snakefile(1 hunks)tests/tests.py(2 hunks)
🧰 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...
**/*.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.
src/snakemake/settings/types.pytests/tests.pysrc/snakemake/cli.pysrc/snakemake/logging.pysrc/snakemake/api.pysrc/snakemake/workflow.py
🧬 Code Graph Analysis (2)
src/snakemake/logging.py (2)
src/snakemake/workflow.py (2)
workdir(1646-1650)dryrun(393-397)src/snakemake/utils.py (1)
makedirs(293-300)
src/snakemake/api.py (1)
src/snakemake/logging.py (2)
setup_logfile(665-687)setup(552-602)
🪛 Ruff (0.8.2)
src/snakemake/logging.py
410-413: Return the condition not event == LogEvent.WORKFLOW_STARTED directly
Replace with return not event == LogEvent.WORKFLOW_STARTED
(SIM103)
⏰ 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, macos-latest, py312)
- 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)
🔇 Additional comments (6)
src/snakemake/workflow.py (1)
1163-1167: Confirm thatlogger_manager.setup()is always invoked before rulegraph logging
logger_manager.initializedwas removed, solog_rulegraph()now relies solely onneeds_rulegraph.
If a workflow reaches this point beforeLoggerManager.setup()runs,needs_rulegraphis stillFalseand no rule-graph will be emitted, even if a plugin actually wants it.Please double-check the execution order (especially for CLI entry-points) or guard the call with a fallback such as:
if getattr(logger_manager, "needs_rulegraph", False): ...src/snakemake/cli.py (1)
1994-1994: Good addition of execution mode to output settingsAdding the execution mode from CLI arguments to the OutputSettings constructor aligns with centralizing logger setup in the SnakemakeAPI. This change enables proper logger initialization before workflow execution starts.
tests/test_workflow_logger/Snakefile (3)
1-3: Logger usage in workflow file looks goodTesting different logging severity levels (info, warning, error) is a good approach to verify the refined logging system.
5-8: Rule 'all' implementation is correctThe rule properly specifies the final targets using expand to generate multiple output files.
10-15: Rule 'a' implementation is correctThe rule correctly uses wildcards to generate the output files with their indices.
src/snakemake/api.py (1)
147-151:setup_logfile()should run afterWorkdirHandler.change_to()
setup_logfile(workdir=workdir)executes before theWorkdirHandleractuallychdirs intoworkdir.
If any relative paths are used insidesetup_logfile(or outside callers forget to passworkdir), the log ends up in the calling dir, not the workflow dir.Two low-impact fixes:
- snakefile = resolve_snakefile(snakefile) - logger_manager.setup_logfile(workdir=workdir) + snakefile = resolve_snakefile(snakefile) + + # order matters: change into workdir first, then open the logfile there + # (WorkdirHandler.change_to() is triggered inside WorkflowApi.__post_init__) +…or, move the
setup_logfile()call intoWorkflowApi.__post_init__()right after the directory change.
| keep_logger: bool = False | ||
| stdout: bool = False | ||
| benchmark_extended: bool = False | ||
| mode: ExecMode = ExecMode.DEFAULT |
There was a problem hiding this comment.
So, what you do here is adding exec mode here but not removing it from WorkflowSettings?
If we would keep the old way, couldn't the logger behavior just be modified once the exec mode is known, instead of setting it up again there? This way, one would avoid the multiple logger setups as well.
There was a problem hiding this comment.
Another alternative: what if we don't set the execMode here but instead offer settings for the individual logger behavior that so far is triggered by the exec mode. Then, whoever calls the API would have full control, and we don't have that duplication of settings. When the CLI calls the API, it could then set those settings according to exec mode (no need to expose them to command line flags). And their default could be the current ExecMode.DEFAULT behavior.
|
Regarding dryrun and stdout, the rationale was that when you do a dryrun, you want logging to happen on stdout so that you can easily pipe into |
|
Thanks for the review @johanneskoester, will try to address this weekend. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/snakemake/logging.py (2)
414-415: Simplify the return statement as suggested by static analysis.The filtering logic is correct, but it can be simplified for better readability.
Apply this diff to simplify the return statement:
- if event == LogEvent.WORKFLOW_STARTED: - return False + if event == LogEvent.WORKFLOW_STARTED: + return FalseActually, you can make it even more concise:
- if event == LogEvent.WORKFLOW_STARTED: - return False - - return True + return event != LogEvent.WORKFLOW_STARTED
650-654: Apply static analysis suggestion for cleaner dictionary iteration.The logic is correct, but the dictionary iteration can be improved as suggested by static analysis.
Apply this diff to improve the dictionary iteration:
- if self.settings.enable_file_logging: - for handler in self.logfile_handlers.keys(): + if self.settings.enable_file_logging: + for handler in self.logfile_handlers:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/snakemake/api.py(4 hunks)src/snakemake/cli.py(2 hunks)src/snakemake/logging.py(5 hunks)src/snakemake/settings/types.py(1 hunks)src/snakemake/workflow.py(1 hunks)tests/tests.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/snakemake/settings/types.py
- src/snakemake/cli.py
- src/snakemake/workflow.py
- tests/tests.py
- src/snakemake/api.py
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for...
**/*.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/snakemake/logging.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
src/snakemake/logging.py (3)
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's `Logger.handler` method, the mapping of the custom log level `'info'` to `_logging.WARNING` is intentional to preserve the existing log output coloring.
Learnt from: johanneskoester
PR: snakemake/snakemake#3107
File: snakemake/logging.py:510-515
Timestamp: 2025-01-14T14:04:30.554Z
Learning: In Snakemake's logging handlers, KeyboardInterrupt and SystemExit exceptions should be ignored (pass) rather than re-raised, as these are control flow signals that should not disrupt the logging process. The exception variable in the final except block should be kept for potential debugging purposes.
Learnt from: johanneskoester
PR: snakemake/snakemake#3114
File: snakemake/cli.py:708-708
Timestamp: 2024-10-13T14:10:37.796Z
Learning: In the `snakemake/cli.py` file, `ExecutorPluginRegistry()` is a singleton class, so multiple instantiations are acceptable and do not cause unnecessary overhead.
🪛 Ruff (0.11.9)
src/snakemake/logging.py
414-417: Return the condition event != LogEvent.WORKFLOW_STARTED directly
Replace with return event != LogEvent.WORKFLOW_STARTED
(SIM103)
651-651: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: tests (10, macos-latest, py312)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
🔇 Additional comments (5)
src/snakemake/logging.py (5)
455-455: LGTM! Simplified color determination logic.The removal of mode dependency from color determination aligns well with the PR's objective of decoupling execution modes from logging internals.
462-479: LGTM! Clean simplification of color detection logic.The removal of mode-based logic makes the color detection more straightforward while maintaining the essential functionality for determining TTY color support.
557-594: LGTM! Excellent refactoring of handler setup logic.The replacement of mode-based logic with explicit boolean settings greatly improves code clarity and maintainability. The conditional queue listener setup based on
settings.enable_queue_listeneraligns perfectly with the PR's goal of reducing overhead when plugins are not used.
644-644: LGTM! Consistent use of settings flags.The change from mode-based checks to
settings.enable_file_loggingimproves consistency with the overall refactoring approach.
655-672: LGTM! Enhanced flexibility with dynamic log directory handling.The addition of the optional
workdirparameter and dynamic log directory path determination improves the method's flexibility while maintaining backward compatibility. The immediate handler addition to the logger aligns well with the centralized setup approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/snakemake/logging.py (2)
414-416: Simplify the conditional logic.The if/else structure can be simplified by returning the condition directly, as suggested by the static analysis tool.
- if event == LogEvent.WORKFLOW_STARTED: - return False - - return True + return event != LogEvent.WORKFLOW_STARTED
654-654: Optimize dictionary key iteration.Use direct key membership check instead of iterating over
.keys()for better performance.- for handler in self.logfile_handlers.keys(): + for handler in self.logfile_handlers:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/logging.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for...
**/*.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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/snakemake/logging.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in `tests/test_wrapper/Snakefile`, are for testing purposes and do not require updates to the project documentation.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: leoschwarz
PR: snakemake/snakemake#3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: mbhall88
PR: snakemake/snakemake#3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
src/snakemake/logging.py (3)
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's `Logger.handler` method, the mapping of the custom log level `'info'` to `_logging.WARNING` is intentional to preserve the existing log output coloring.
Learnt from: johanneskoester
PR: snakemake/snakemake#3107
File: snakemake/logging.py:510-515
Timestamp: 2025-01-14T14:04:30.554Z
Learning: In Snakemake's logging handlers, KeyboardInterrupt and SystemExit exceptions should be ignored (pass) rather than re-raised, as these are control flow signals that should not disrupt the logging process. The exception variable in the final except block should be kept for potential debugging purposes.
Learnt from: johanneskoester
PR: snakemake/snakemake#3114
File: snakemake/cli.py:708-708
Timestamp: 2024-10-13T14:10:37.796Z
Learning: In the `snakemake/cli.py` file, `ExecutorPluginRegistry()` is a singleton class, so multiple instantiations are acceptable and do not cause unnecessary overhead.
🪛 Ruff (0.11.9)
src/snakemake/logging.py
414-417: Return the condition event != LogEvent.WORKFLOW_STARTED directly
Replace with return event != LogEvent.WORKFLOW_STARTED
(SIM103)
654-654: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (35)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, macos-latest, py312)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, macos-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (7)
src/snakemake/logging.py (7)
455-455: LGTM! Simplified color detection logic.The removal of the mode parameter simplifies the color detection logic and aligns with the overall refactoring to remove mode dependencies.
462-479: LGTM! Cleaner TTY color detection.The simplified
can_color_ttymethod removes mode-specific logic while maintaining the same color detection behavior. The logic correctly handles "dumb" terminals and TTY detection on non-Windows systems.
542-542: LGTM! Simplified LoggerManager initialization.The removal of the
initializedandmodeattributes aligns with the centralized logger setup approach described in the PR objectives.
557-597: LGTM! Improved settings-based configuration.The refactoring from mode-based to settings-based configuration is well-implemented:
- Clear separation of stream handlers and other handlers
- Proper conditional logic for different logging scenarios
- Queue listener is only created when needed (
settings.enable_queue_listenerandother_handlersexist)- Direct handler addition to logger instead of collecting first
This change improves flexibility and reduces overhead when queue listeners aren't required.
647-647: LGTM! Settings-based logfile hint logic.The change from mode-based to settings-based check (
settings.enable_file_logging) is consistent with the overall refactoring approach.
653-653: LGTM! Settings-based cleanup logic.The change to use
settings.enable_file_logginginstead of mode checks is consistent with the refactoring approach.
658-675: LGTM! Enhanced logfile setup with workdir support.The addition of the optional
workdirparameter enhances flexibility by allowing dynamic log directory determination. The implementation:
- Correctly handles both cases (with and without workdir)
- Uses
os.path.joinfor proper path construction- Creates directories with
exist_ok=Trueto avoid errors- Immediately adds the handler to the logger
- Properly handles OSError exceptions
This change supports the centralized logger setup mentioned in the PR objectives.
|
Is this ready to merge? |
Almost! I'm going to add some more tests and finally write some docs on the logging. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/snakemake/logging.py (2)
414-416: New filter condition for WORKFLOW_STARTED events.The logic correctly filters out
WORKFLOW_STARTEDevents. Consider the static analysis suggestion to simplify:- if event == LogEvent.WORKFLOW_STARTED: - return False + return event != LogEvent.WORKFLOW_STARTED
654-657: Settings-based cleanup logic with minor optimization opportunity.The conversion to
settings.enable_file_loggingis consistent with the refactoring approach. Consider the static analysis suggestion for line 655:- for handler in self.logfile_handlers.keys(): + for handler in self.logfile_handlers:tests/test_logging.py (1)
1-21: Clean up unused imports.Several imports are flagged as unused by static analysis. Consider removing the clearly unused ones to keep the imports clean:
-import pytest -from .common import run, dpath, apptainer, connected +from .common import run, dpath -from .conftest import ( - skip_on_windows, - only_on_windows, - ON_WINDOWS, - needs_strace, - ON_MACOS, -) +from .conftest import ON_WINDOWS
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/snakemake/cli.py(2 hunks)src/snakemake/logging.py(4 hunks)src/snakemake/settings/types.py(1 hunks)tests/logging/test_logging_config/Snakefile(1 hunks)tests/logging/test_logging_config/results/0.txt(1 hunks)tests/logging/test_logging_config/results/1.txt(1 hunks)tests/logging/test_logging_config/results/2.txt(1 hunks)tests/logging/test_logging_config/results/3.txt(1 hunks)tests/logging/test_logging_config/results/4.txt(1 hunks)tests/logging/test_rule_failure/Snakefile(1 hunks)tests/logging/test_workflow_logger/Snakefile(1 hunks)tests/test_logging.py(1 hunks)tests/tests.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/tests.py
✅ Files skipped from review due to trivial changes (5)
- tests/logging/test_logging_config/results/4.txt
- tests/logging/test_logging_config/results/2.txt
- tests/logging/test_logging_config/results/3.txt
- tests/logging/test_logging_config/results/0.txt
- tests/logging/test_logging_config/results/1.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/snakemake/settings/types.py
- src/snakemake/cli.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:
tests/test_logging.pysrc/snakemake/logging.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's `Logger.handler` method, the mapping of the custom log level `'info'` to `_logging.WARNING` is intentional to preserve the existing log output coloring.
Learnt from: johanneskoester
PR: snakemake/snakemake#2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-08-27T14:31:21.449Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: johanneskoester
PR: snakemake/snakemake#2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:0-0
Timestamp: 2024-08-13T16:07:33.369Z
Learning: In the Snakemake project, trailing commas are not needed in logger messages if Black formatting is used, as it doesn't require them in such contexts.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:0-0
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake project, trailing commas are not needed in logger messages if Black formatting is used, as it doesn't require them in such contexts.
tests/logging/test_rule_failure/Snakefile (8)
Learnt from: johanneskoester
PR: #3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in tests/test_wrapper/Snakefile, are for testing purposes and do not require updates to the project documentation.
Learnt from: johanneskoester
PR: #2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: leoschwarz
PR: #3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: mbhall88
PR: #3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Learnt from: johanneskoester
PR: #3204
File: tests/test_cores_cluster/qsub:1-6
Timestamp: 2024-11-12T20:22:54.184Z
Learning: In the Snakemake codebase, the tests/test_cores_cluster/qsub script is a dummy script for testing, and input validation and error handling are not required in such scripts.
Learnt from: johanneskoester
PR: #2985
File: tests/tests.py:2051-2051
Timestamp: 2024-08-13T09:25:24.046Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: #2985
File: tests/tests.py:2051-2051
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: #2927
File: tests/test_subpath/Snakefile:11-11
Timestamp: 2024-12-14T13:10:48.450Z
Learning: In Snakemake, when a rule defines a single output file (even if the output is specified as a dictionary), {output} in the shell command can be used directly to refer to that file.
tests/logging/test_workflow_logger/Snakefile (10)
Learnt from: johanneskoester
PR: #3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in tests/test_wrapper/Snakefile, are for testing purposes and do not require updates to the project documentation.
Learnt from: johanneskoester
PR: #2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: cademirch
PR: #3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: leoschwarz
PR: #3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: cademirch
PR: #3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's Logger.handler method, the mapping of the custom log level 'info' to _logging.WARNING is intentional to preserve the existing log output coloring.
Learnt from: mbhall88
PR: #3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Learnt from: johanneskoester
PR: #3159
File: docs/tutorial/interaction_visualization_reporting/workdir/workflow/Snakefile:1-1
Timestamp: 2024-10-20T19:42:16.694Z
Learning: The report directive at the workflow level is still valid in Snakemake and is not deprecated.
Learnt from: johanneskoester
PR: #2985
File: tests/tests.py:2051-2051
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: #2985
File: tests/tests.py:2051-2051
Timestamp: 2024-08-13T09:25:24.046Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
Learnt from: johanneskoester
PR: #2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-08-27T14:31:21.449Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
tests/test_logging.py (6)
Learnt from: johanneskoester
PR: #3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in tests/test_wrapper/Snakefile, are for testing purposes and do not require updates to the project documentation.
Learnt from: leoschwarz
PR: #3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: cademirch
PR: #3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: johanneskoester
PR: #2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: johanneskoester
PR: #3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Learnt from: mbhall88
PR: #3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
tests/logging/test_logging_config/Snakefile (10)
Learnt from: johanneskoester
PR: #3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in tests/test_wrapper/Snakefile, are for testing purposes and do not require updates to the project documentation.
Learnt from: johanneskoester
PR: #2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: cademirch
PR: #3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: leoschwarz
PR: #3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: cademirch
PR: #3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's Logger.handler method, the mapping of the custom log level 'info' to _logging.WARNING is intentional to preserve the existing log output coloring.
Learnt from: johanneskoester
PR: #3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Learnt from: johanneskoester
PR: #2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-08-27T14:31:21.449Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: johanneskoester
PR: #2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: mbhall88
PR: #3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
Learnt from: johanneskoester
PR: #2985
File: tests/tests.py:2051-2051
Timestamp: 2024-08-13T09:25:24.046Z
Learning: In the Snakemake repository, avoid suggesting return type annotations for test functions.
src/snakemake/logging.py (3)
Learnt from: cademirch
PR: #3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's Logger.handler method, the mapping of the custom log level 'info' to _logging.WARNING is intentional to preserve the existing log output coloring.
Learnt from: johanneskoester
PR: #3107
File: snakemake/logging.py:510-515
Timestamp: 2025-01-14T14:04:30.554Z
Learning: In Snakemake's logging handlers, KeyboardInterrupt and SystemExit exceptions should be ignored (pass) rather than re-raised, as these are control flow signals that should not disrupt the logging process. The exception variable in the final except block should be kept for potential debugging purposes.
Learnt from: johanneskoester
PR: #3114
File: snakemake/cli.py:708-708
Timestamp: 2024-10-13T14:10:37.796Z
Learning: In the snakemake/cli.py file, ExecutorPluginRegistry() is a singleton class, so multiple instantiations are acceptable and do not cause unnecessary overhead.
🪛 Ruff (0.12.2)
tests/test_logging.py
5-5: pytest imported but unused
Remove unused import: pytest
(F401)
13-13: .common.apptainer imported but unused
Remove unused import
(F401)
13-13: .common.connected imported but unused
Remove unused import
(F401)
15-15: .conftest.skip_on_windows imported but unused
Remove unused import
(F401)
16-16: .conftest.only_on_windows imported but unused
Remove unused import
(F401)
18-18: .conftest.needs_strace imported but unused
Remove unused import
(F401)
19-19: .conftest.ON_MACOS imported but unused
Remove unused import
(F401)
src/snakemake/logging.py
414-417: Return the condition event != LogEvent.WORKFLOW_STARTED directly
Replace with return event != LogEvent.WORKFLOW_STARTED
(SIM103)
655-655: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ 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). (36)
- GitHub Check: apidocs
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (10, macos-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (6, macos-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (5, macos-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
🔇 Additional comments (18)
tests/logging/test_workflow_logger/Snakefile (3)
1-6: Effective test for logger availability in Snakefile context.This test correctly validates that the logger is accessible within a Snakefile after the centralized logging setup changes. The code assumes
loggeris available in the global scope, which aligns with the PR's objective to enable workflow files to utilize the logger.
9-11: Standard workflow entry point.The
allrule is correctly defined and follows Snakemake conventions for specifying workflow targets.
14-18: Correct rule implementation for test scenario.Rule
aproperly uses wildcards and shell commands to generate the expected test outputs.tests/logging/test_rule_failure/Snakefile (2)
1-3: Standard workflow entry point.The
allrule correctly defines the workflow targets using proper Snakemake syntax.
6-10: Intentional error for failure testing.The typo
ehoinstead ofechois deliberately included to trigger rule failure and test error logging behavior. This design effectively serves the test's purpose.tests/logging/test_logging_config/Snakefile (3)
1-28: Well-structured logging configuration for testing.The logging configuration dictionary is comprehensive and correctly formatted:
- Uses proper dictConfig version 1 format
- Custom formatter includes test identifier
[TESTLOGGINGCONFIG]for verification- Console handler redirects to stdout as needed for test validation
- Snakemake logger is properly configured with DEBUG level and disabled propagation
29-31: Standard workflow entry point.The
allrule follows standard Snakemake conventions for defining workflow targets.
34-38: Correct rule implementation.Rule
aproperly implements the pattern for generating test output files using wildcards and shell commands.src/snakemake/logging.py (5)
455-456: Simplified constructor logic.Good simplification removing the mode parameter dependency. The nocolor determination now relies solely on TTY capability detection, making the logic cleaner.
462-480: Improved TTY color detection logic.The simplified method now focuses on actual terminal capabilities rather than execution mode. This provides more accurate color support determination based on real TTY status and terminal type.
547-600: Excellent refactoring of logger setup logic.This refactoring successfully:
- Removes mode dependency in favor of settings-based configuration
- Handles error-only logging with early return for better performance
- Validates plugin handlers to prevent conflicts
- Sets up asynchronous logging only when plugin handlers exist
- Ensures proper fallback to default handlers
The centralized approach aligns well with the PR's objectives to simplify logging management.
647-651: Consistent settings-based logic.Good conversion from mode-based to settings-based logic using
settings.enable_file_logging. This maintains the same functionality while aligning with the refactoring approach.
659-680: Enhanced logfile setup with dynamic directory support.The addition of the optional
workdirparameter provides better flexibility for log file placement. The immediate handler addition and simplified logic make the method more straightforward and maintainable.tests/test_logging.py (5)
23-48: Comprehensive logfile functionality test.Excellent test coverage for:
- Log directory creation validation
- Log file generation verification
- Log content assertion with specific completion message
- Proper cleanup with Windows compatibility
The test structure is clear and assertions are meaningful.
50-64: Effective custom logging configuration test.Good test design that validates custom logging configuration in Snakefiles. The subprocess approach and assertion of the custom tag
[TESTLOGGINGCONFIG]effectively verifies the logging setup functionality.
66-101: Critical test for workflow logger integration.This test validates a key PR objective - enabling logger usage within workflow files. The dual validation approach (checking both Snakemake logs and custom log file) provides comprehensive coverage of the logger accessibility feature.
103-127: Detailed log event validation for dry run mode.Excellent granular testing of log events with precise count expectations. This provides valuable regression protection for the logging event system during dry run execution.
129-178: Comprehensive log event validation for full execution.Outstanding test coverage that validates:
- Expected log event counts across multiple event types
- Absence of unexpected log events (important for regression detection)
- Stderr output content verification
- Integration between different logging mechanisms
This provides robust protection against logging regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_logging.py (1)
180-181: Complete the rule failure test implementation.This test function is missing assertions and validation logic for rule failure logging behavior, as noted in previous reviews.
🧹 Nitpick comments (1)
tests/test_logging.py (1)
5-5: Clean up unused imports.Several imports are not used in this file and can be removed to improve code clarity.
-import pytest import logging from collections import Counter from snakemake_interface_logger_plugins.common import LogEvent sys.path.insert(0, os.path.dirname(__file__)) -from .common import run, dpath, apptainer, connected +from .common import run, dpath from .conftest import ( - skip_on_windows, - only_on_windows, ON_WINDOWS, - needs_strace, - ON_MACOS, )Also applies to: 13-13, 15-16, 18-19
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/snakemake/settings/types.py(1 hunks)tests/test_logging.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/snakemake/settings/types.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:
tests/test_logging.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's `Logger.handler` method, the mapping of the custom log level `'info'` to `_logging.WARNING` is intentional to preserve the existing log output coloring.
Learnt from: johanneskoester
PR: snakemake/snakemake#2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-08-27T14:31:21.449Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: johanneskoester
PR: snakemake/snakemake#2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:0-0
Timestamp: 2024-08-13T16:07:33.369Z
Learning: In the Snakemake project, trailing commas are not needed in logger messages if Black formatting is used, as it doesn't require them in such contexts.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:0-0
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake project, trailing commas are not needed in logger messages if Black formatting is used, as it doesn't require them in such contexts.
tests/test_logging.py (6)
Learnt from: johanneskoester
PR: #3117
File: tests/test_wrapper/Snakefile:11-11
Timestamp: 2024-10-06T14:09:54.370Z
Learning: Changes made within test cases, such as in tests/test_wrapper/Snakefile, are for testing purposes and do not require updates to the project documentation.
Learnt from: leoschwarz
PR: #3176
File: tests/test_output_index.py:99-157
Timestamp: 2025-01-17T12:00:09.368Z
Learning: New test dependencies for Snakemake should be introduced in separate PRs rather than being added as part of feature or refactoring PRs.
Learnt from: cademirch
PR: #3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: johanneskoester
PR: #2925
File: tests/test_nonstr_params/Snakefile:14-15
Timestamp: 2024-12-21T15:10:31.992Z
Learning: The file "test.jsonl" in tests/test_nonstr_params is automatically created by Snakemake, rather than manually generated in the Snakefile.
Learnt from: johanneskoester
PR: #3135
File: snakemake/profiles.py:54-55
Timestamp: 2024-10-13T14:29:38.902Z
Learning: In the Snakemake test suite, tests for configuration keys like 'group-components' may already be covered within existing tests for 'groups'. Ensure to verify this before suggesting additional tests.
Learnt from: mbhall88
PR: #3188
File: tests/test_script/scripts/test.sh:4-4
Timestamp: 2024-11-07T00:32:44.137Z
Learning: In test scripts within the Snakemake project, concise code is preferred over verbose error handling when simplicity suffices.
🪛 Ruff (0.12.2)
tests/test_logging.py
5-5: pytest imported but unused
Remove unused import: pytest
(F401)
13-13: .common.apptainer imported but unused
Remove unused import
(F401)
13-13: .common.connected imported but unused
Remove unused import
(F401)
15-15: .conftest.skip_on_windows imported but unused
Remove unused import
(F401)
16-16: .conftest.only_on_windows imported but unused
Remove unused import
(F401)
18-18: .conftest.needs_strace imported but unused
Remove unused import
(F401)
19-19: .conftest.ON_MACOS imported but unused
Remove unused import
(F401)
⏰ 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). (35)
- GitHub Check: tests (8, macos-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (7, macos-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, macos-latest, py312)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (4, macos-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
🔇 Additional comments (5)
tests/test_logging.py (5)
23-48: Well-structured test for log file verification.The test correctly validates log directory creation, file existence, and content verification. The Windows-specific cleanup handling is appropriate.
50-64: Appropriate test for logging configuration validation.The test correctly uses subprocess to validate that custom logging configuration produces expected output, even in failure scenarios. The reference to the GitHub issue provides helpful context.
66-101: Comprehensive test for workflow logger functionality.This test effectively validates that loggers work correctly within workflow files by checking both Snakemake's standard logs and custom log files. The dual validation approach ensures proper logger integration.
103-127: Well-designed test for dry-run log events.The test effectively validates log event emission during dry-run execution. The use of Counter for event tracking and detailed assertion messages makes it robust and debuggable.
129-178: Comprehensive test for normal execution log events.The test provides excellent coverage by validating both structured log events and stderr output. The defensive check for unexpected events and detailed assertion messages enhance the test's robustness.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/snakemake/logging.py (1)
380-381: Bug: dryrun filter compares wrong field; use event, not levelname
levelis the logging level name (e.g., "INFO"), not the Snakemake event. Comparing to "run_info" will likely never match. Use the LogEvent instead.- if self.dryrun and level == "run_info": + if self.dryrun and event == LogEvent.RUN_INFO: return True
🧹 Nitpick comments (1)
src/snakemake/logging.py (1)
414-417: Tidy: return the condition directly (Ruff SIM103)No functional change; shorter and clearer.
- if event == LogEvent.WORKFLOW_STARTED: - return False - - return True + return event != LogEvent.WORKFLOW_STARTED
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/project_info/codebase.rst(1 hunks)src/snakemake/cli.py(4 hunks)src/snakemake/logging.py(4 hunks)src/snakemake/settings/types.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/project_info/codebase.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- src/snakemake/settings/types.py
- src/snakemake/cli.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/logging.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-08-13T16:22:58.430Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:1250-1251
Timestamp: 2024-10-08T17:41:54.542Z
Learning: Avoid commenting on trailing commas in code reviews for the Snakemake repository as per the user's preference.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:20-20
Timestamp: 2025-01-13T18:50:40.791Z
Learning: Dependencies for Snakemake are managed through test-environment.yml file, which includes snakemake-interface-logger-plugins (>= 0.1.0) and other required packages.
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's `Logger.handler` method, the mapping of the custom log level `'info'` to `_logging.WARNING` is intentional to preserve the existing log output coloring.
Learnt from: johanneskoester
PR: snakemake/snakemake#2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: johanneskoester
PR: snakemake/snakemake#2963
File: snakemake/scheduler.py:199-199
Timestamp: 2024-08-27T14:31:21.449Z
Learning: In the Snakemake project, do not suggest adding trailing commas in multi-line logging statements, as Black autoformatter handles this.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:0-0
Timestamp: 2024-08-13T16:07:33.369Z
Learning: In the Snakemake project, trailing commas are not needed in logger messages if Black formatting is used, as it doesn't require them in such contexts.
Learnt from: johanneskoester
PR: snakemake/snakemake#3014
File: snakemake/workflow.py:0-0
Timestamp: 2024-10-08T17:41:54.542Z
Learning: In the Snakemake project, trailing commas are not needed in logger messages if Black formatting is used, as it doesn't require them in such contexts.
📚 Learning: 2024-09-30T17:17:37.360Z
Learnt from: cademirch
PR: snakemake/snakemake#3107
File: snakemake/logging.py:806-830
Timestamp: 2024-09-30T17:17:37.360Z
Learning: In the Snakemake project's `Logger.handler` method, the mapping of the custom log level `'info'` to `_logging.WARNING` is intentional to preserve the existing log output coloring.
Applied to files:
src/snakemake/logging.py
📚 Learning: 2025-01-14T14:04:30.554Z
Learnt from: johanneskoester
PR: snakemake/snakemake#3107
File: snakemake/logging.py:510-515
Timestamp: 2025-01-14T14:04:30.554Z
Learning: In Snakemake's logging handlers, KeyboardInterrupt and SystemExit exceptions should be ignored (pass) rather than re-raised, as these are control flow signals that should not disrupt the logging process. The exception variable in the final except block should be kept for potential debugging purposes.
Applied to files:
src/snakemake/logging.py
📚 Learning: 2024-10-13T14:10:37.796Z
Learnt from: johanneskoester
PR: snakemake/snakemake#3114
File: snakemake/cli.py:708-708
Timestamp: 2024-10-13T14:10:37.796Z
Learning: In the `snakemake/cli.py` file, `ExecutorPluginRegistry()` is a singleton class, so multiple instantiations are acceptable and do not cause unnecessary overhead.
Applied to files:
src/snakemake/logging.py
🧬 Code Graph Analysis (1)
src/snakemake/logging.py (2)
src/snakemake/workflow.py (2)
dryrun(400-404)workdir(1662-1666)src/snakemake/utils.py (1)
makedirs(291-298)
🪛 Ruff (0.12.2)
src/snakemake/logging.py
414-417: Return the condition event != LogEvent.WORKFLOW_STARTED directly
Replace with return event != LogEvent.WORKFLOW_STARTED
(SIM103)
672-672: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ 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). (56)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-latest, py313)
- GitHub Check: tests (9, windows-latest, py312)
- GitHub Check: tests (7, windows-latest, py312)
- GitHub Check: tests (8, windows-latest, py312)
- GitHub Check: tests (8, windows-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, windows-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, windows-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-latest, py313)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, windows-latest, py313)
- GitHub Check: tests (2, windows-latest, py313)
- GitHub Check: tests (2, macos-latest, py312)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- 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, windows-latest, py312)
- GitHub Check: tests (1, windows-latest, py313)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: apidocs
🔇 Additional comments (7)
src/snakemake/logging.py (7)
513-516: Good: ignore KeyboardInterrupt/SystemExit in emitMatches Snakemake guidance to treat them as control-flow signals.
462-479: Windows color output now disabled; verify intended UX changeThis path disables ANSI colors on Windows entirely. If that’s intentional, fine. If not, consider optionally enabling colors on modern Windows terminals or when colorama is present.
Would you like to maintain color on Windows terminals that support ANSI sequences? If yes, I can propose a minimal, dependency-free detection improvement.
573-617: Plugin queue init LGTM; one stream handler enforcement is goodQueue is only used when plugins exist and single-stream constraint is enforced. Looks solid after addressing listener lifecycle in setup.
665-668: Logfile hint gating LGTMUsing
enable_file_loggingandnot dryrunfor hints is correct for the new model.
676-694: Ordering and threading considerations for setup_logfile
- If
setup_logfile()is called beforesetup(), the file handler will be removed bysetup()(handlers are cleared). Ensure call order issetup()thensetup_logfile().- File handler is attached directly to the logger while plugin handlers are behind a queue; that’s fine if you want immediate file writes outside the queue. If you want all handlers to be fed via the queue for consistent ordering, consider routing the file handler through the queue as well.
Please confirm intended call order and whether file handler should be queued alongside plugin handlers. I can provide a diff for either approach.
390-401: No issue with LogEvent.RESOURCES_INFO availability
TheRESOURCES_INFOmember is defined in thesnakemake_interface_logger_plugins.common.LogEventenum (included via thesnakemake-interface-logger-plugins (>= 0.1.0)dependency). References in both the workflow code andtests/test_logging.pyconfirm it’s present at import time, so no runtime failure will occur.
558-571: Swap logger setup and logfile initialization insrc/snakemake/api.pySince
logger_manager.setup()clears all handlers, it must be called beforesetup_logfile()to avoid discarding the file handler. Insrc/snakemake/api.pythe calls are currently inverted:• Line 148:
logger_manager.setup_logfile(workdir=workdir)…
• Line 258:logger_manager.setup( handlers=log_handlers, settings=self.output_settings, )Please swap them as follows:
- logger_manager.setup_logfile(workdir=workdir) + logger_manager.setup( + handlers=log_handlers, + settings=self.output_settings, + ) + logger_manager.setup_logfile(workdir=workdir) - logger_manager.setup( - handlers=log_handlers, - settings=self.output_settings, - )This ensures the file handler isn’t cleared immediately after being added.
⛔ Skipped due to learnings
Learnt from: cademirch PR: snakemake/snakemake#3107 File: snakemake/logging.py:806-830 Timestamp: 2024-09-30T17:17:37.360Z Learning: In the Snakemake project's `Logger.handler` method, the mapping of the custom log level `'info'` to `_logging.WARNING` is intentional to preserve the existing log output coloring.Learnt from: johanneskoester PR: snakemake/snakemake#3600 File: src/snakemake/jobs.py:960-964 Timestamp: 2025-05-23T09:40:24.474Z Learning: In the `cleanup` method of the `Job` class in `src/snakemake/jobs.py`, files in the `to_remove` list should be formatted with `fmt_iofile` without specifying `as_output=True` or `as_input=True` parameters, as these files should be displayed as generic files rather than specifically as output files.
|
Replaced the boolean log_errors_only flag with settings:
This gives us better control over logging behavior based on execution mode: Subprocess mode: Sets log level to ERROR (currently), skips file logging Eventually I'd like to clean up the output when a subprocess job fails as its quite verbose. But that is not very high priority. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/snakemake/api.py (1)
257-269: Always initialize the logger; keep_logger should only affect teardownsetup_logger currently returns early when output_settings.keep_logger is True, leaving logging unconfigured and causing missing logs. Initialize logger_manager unconditionally (or at least only skip when logger_manager is already configured).
Location: src/snakemake/api.py — def setup_logger (≈ lines 257–269)
- Remove the early guard so logger_manager.setup(...) is called with handlers built from output_settings.log_handler_settings.
- Ensure LoggerManager.setup is safe to call repeatedly: stop any existing QueueListener and close previous handlers before creating new ones, or make setup idempotent; alternatively perform a config-equality check and skip setup only when already configured.
♻️ Duplicate comments (1)
src/snakemake/api.py (1)
111-113: Preserve init chain: call super().post_init()Keeps base invariants and future checks intact.
def __post_init__(self): - self.setup_logger() + super().__post_init__() + self.setup_logger()
🧹 Nitpick comments (2)
src/snakemake/workflow.py (1)
1204-1206: LGTM with a tiny nit: early-return before defining helperFunctional change is sound. For a micro win, return early when the rulegraph isn’t needed to avoid defining
simple_rulegraph()on every call.def log_rulegraph(self): - def simple_rulegraph(): + if not logger_manager.needs_rulegraph: + return + def simple_rulegraph(): @@ - if logger_manager.needs_rulegraph: - rulegraph = simple_rulegraph() - logger.info(None, extra=dict(event=LogEvent.RULEGRAPH, rulegraph=rulegraph)) + rulegraph = simple_rulegraph() + logger.info(None, extra=dict(event=LogEvent.RULEGRAPH, rulegraph=rulegraph))src/snakemake/api.py (1)
155-166: Avoid creating the workdir implicitly during logfile setup
setup_logfile(workdir=workdir)willos.makedirs()the full path (including a non-existent workdir) beforeWorkflowApiswitches directories. Move logfile setup to afterWorkflowApihas changed into the workdir; call without an explicit path so it’s created relative to the effective CWD.- logger_manager.setup_logfile(workdir=workdir) self._workflow_api = WorkflowApi( snakemake_api=self, snakefile=snakefile, workdir=workdir, @@ storage_provider_settings=storage_provider_settings, ) + # now that WorkdirHandler has switched CWD, create the per-workdir logfile + logger_manager.setup_logfile()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/snakemake/api.py(4 hunks)src/snakemake/cli.py(3 hunks)src/snakemake/settings/types.py(1 hunks)src/snakemake/workflow.py(1 hunks)tests/tests.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/tests.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/snakemake/cli.py
- src/snakemake/settings/types.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/workflow.pysrc/snakemake/api.py
🧬 Code graph analysis (1)
src/snakemake/api.py (2)
src/snakemake/logging.py (1)
setup_logfile(676-696)src/snakemake/workflow.py (1)
workdir(1692-1696)
⏰ 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, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-2022, py312)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (9, windows-2022, py312)
- GitHub Check: tests (9, macos-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (4, windows-2022, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, windows-2022, py312)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, windows-2022, py312)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (7, macos-latest, py312)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (6, windows-2022, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, windows-2022, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-2022, py312)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (2, windows-2022, py312)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-2022, py312)
🤖 I have created a release *beep* *boop* --- ## [9.11.3](v9.11.2...v9.11.3) (2025-09-16) ### Bug Fixes * logging refinements ([#3571](#3571)) ([47ae16e](47ae16e)) * use proper default and fallback LP solver in scheduler settings (thanks [@raffienficiaud](https://github.com/raffienficiaud)) ([#3736](#3736)) ([e00e5aa](e00e5aa)) ### Documentation * mention nodefaults for installation ([a6db049](a6db049)) --- 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 refines some of the changes from the logging refactor to tackle some issues as well as simplify the code. Changes: ## `ExecMode` added to `OutputSettings`: - ExecMode is critical to setting up the logger and controls how logs are written, so from this perspective I think it makes sense that ExecMode fits here - Moving ExecMode here allows the logger to be setup in SnakemakeAPI, which has multiple positive effects: the logger only needs to be setup once in all of Snakemake's lifecycle, the logger is setup before the workflow, allowing for workflow files to use the logger (snakemake#3558), and the logger does not need to manage state anymore. - This could be seen as a breaking change, though I've set the default for this field to be ExecMode.DEFAULT so existing callers would be unaffected. ## Setup logger no longer dependent on `executor_plugin.common_settings.dryrun_exec` - Another reason the logger had to be setup late in the Snakemake lifecycle was that the stdout setting was dependent on this executor plugin setting - During refactoring I didn't question this, but now after looking at most of the executor plugins, I haven't seen any that use this, so I'm not sure the rationale behind tying stdout to this. @johanneskoester do you happen to know? ## QueueHandler only used for plugins - Only spin up queue overhead for logger plugins if they are being used. ## Added more tests - Added test for specifiyng logger in Snakefile ### QC <!-- Make sure that you can tick the boxes below. --> * [ ] 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 * **New Features** * Added configurable output settings to control log level overrides, plugin handler usage, and file logging. * Added comprehensive logging tests and supporting test workflows to validate logging behavior and failure scenarios. * **Bug Fixes** * Centralized and stabilized logger initialization for more consistent logging across runs. * **Refactor** * Simplified logging setup by removing mode-based branching and consolidating output-settings creation and handler management. * **Documentation** * Documented the logging module and centralized logger management. * **Chores** * Removed an outdated logfile test and updated test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Johannes Köster <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [9.11.3](snakemake/snakemake@v9.11.2...v9.11.3) (2025-09-16) ### Bug Fixes * logging refinements ([snakemake#3571](snakemake#3571)) ([47ae16e](snakemake@47ae16e)) * use proper default and fallback LP solver in scheduler settings (thanks [@raffienficiaud](https://github.com/raffienficiaud)) ([snakemake#3736](snakemake#3736)) ([e00e5aa](snakemake@e00e5aa)) ### Documentation * mention nodefaults for installation ([a6db049](snakemake@a6db049)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
This PR refines some of the changes from the logging refactor to tackle some issues as well as simplify the code.
Changes:
ExecModeadded toOutputSettings:Setup logger no longer dependent on
executor_plugin.common_settings.dryrun_execQueueHandler only used for plugins
Added more tests
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
Bug Fixes
Refactor
Documentation
Chores