refactor: LoggerManager setup and scope#3851
Conversation
📝 WalkthroughWalkthroughAdds a LoggerManager instance to SnakemakeApi, initializes it with OutputSettings, centralizes logfile setup/teardown through LoggerManager, passes it into Workflow instances, refactors logging to plugin-driven handlers and lifecycle methods (setup_logfile, get_logfile, logfile_hint, stop), and deprecates OutputSettings.keep_logger. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant API as SnakemakeApi
participant LM as LoggerManager
participant PR as PluginRegistry
participant WF as Workflow
App->>API: instantiate(logger, output_settings)
API->>API: __post_init__()
API->>LM: LoggerManager.__init__(logger, output_settings)
LM->>LM: store settings, init state
App->>API: workflow(workdir,...)
API->>LM: setup_logfile(workdir)
LM->>LM: _setup_plugins()
LM->>PR: query plugin handlers
PR-->>LM: handlers
LM->>LM: _init_plugin_handler() x N
alt no plugin writes to stream
LM->>LM: add default stream handler
end
API->>API: _get_workflow(...)
API->>WF: Workflow(..., logger_manager=API.logger_manager)
WF->>LM: get_logfile() / logfile_hint()
App->>API: cleanup()
API->>LM: stop()
LM->>LM: stop queue listener & remove handlers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@cademirch sorry got another one for you |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake/logging.py (1)
586-632: Consider extracting the error message to improve maintainability.The
_setup_pluginsmethod correctly initializes plugin handlers and tracks stream/file handler state. However, line 612 has an inline exception message.Consider defining the error message as a constant or using a custom exception class:
+ if has_stream_handler: + raise ValueError( + "Only one stream log handler may be specified. " + "Multiple plugins are attempting to write to stdout/stderr." + ) - if has_stream_handler: - raise ValueError("More than 1 stream log handler specified!")This is a minor style suggestion per the static analysis hint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/snakemake/api.py(5 hunks)src/snakemake/cli.py(0 hunks)src/snakemake/logging.py(2 hunks)src/snakemake/settings/types.py(0 hunks)src/snakemake/workflow.py(5 hunks)
💤 Files with no reviewable 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/api.pysrc/snakemake/workflow.pysrc/snakemake/logging.py
🧠 Learnings (2)
📚 Learning: 2025-01-13T18:50:40.791Z
Learnt from: cademirch
Repo: snakemake/snakemake 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.
Applied to files:
src/snakemake/api.pysrc/snakemake/workflow.pysrc/snakemake/logging.py
📚 Learning: 2025-07-29T14:53:04.598Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3676
File: src/snakemake/cli.py:0-0
Timestamp: 2025-07-29T14:53:04.598Z
Learning: In the Snakemake repository, deprecated scheduler interfaces (like scheduler_ilp_solver, --scheduler-solver-path, --scheduler-greediness) should be kept for backward compatibility during the migration to the plugin-based scheduler system, rather than being removed immediately.
Applied to files:
src/snakemake/workflow.py
🧬 Code graph analysis (2)
src/snakemake/api.py (1)
src/snakemake/logging.py (4)
LoggerManager(532-724)setup_logfile(700-720)cleanup_logfile(694-698)stop(722-724)
src/snakemake/workflow.py (1)
src/snakemake/logging.py (3)
LoggerManager(532-724)get_logfile(683-684)logfile_hint(686-692)
🪛 Ruff (0.14.6)
src/snakemake/logging.py
612-612: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (45)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (15)
src/snakemake/workflow.py (5)
67-67: LGTM!Import updated correctly to include
LoggerManageralongsideloggerandformat_resources.
155-155: LGTM!The
logger_managerfield is correctly added as a required parameter in the dataclass. Its position after the other required settings fields maintains proper argument ordering.
1215-1217: LGTM!Correctly references
self.logger_manager.needs_rulegraphto determine if rulegraph logging is needed.
1432-1432: LGTM!The
_onstarthook correctly usesself.logger_manager.get_logfile()to pass log file paths.
1480-1486: LGTM!All success/error paths correctly use
self.logger_managerfor:
logfile_hint()to display log file location on successget_logfile()to pass log files to_onsuccessand_onerrorhookslogfile_hint()again on error pathsrc/snakemake/api.py (5)
59-59: LGTM!Import correctly updated to include
LoggerManager.
107-112: LGTM!The
logger_managerfield is correctly declared withinit=Falseand initialized in__post_init__. This ensures theLoggerManageris created early with the output settings, setting up plugin handlers before any workflow operations begin.
155-155: LGTM!Calling
setup_logfile(workdir=workdir)here ensures the log file is created in the correct working directory before the workflow is parsed. This is the appropriate location since the workdir is known at this point.
170-177: LGTM!The cleanup flow is correct:
cleanup_logfile()removes file handlers (internally guards withenable_file_loggingcheck)stop()stops the queue listener (internally guards with_thread is not Nonecheck)- Workflow teardown follows
The unconditional calls are safe since both methods have internal guards.
399-399: LGTM!The
logger_manageris correctly passed fromSnakemakeApitoWorkflow, completing the dependency injection pattern and eliminating the global state.src/snakemake/logging.py (5)
24-25: LGTM!Imports correctly added for plugin registry and settings base class needed for the new plugin-based handler initialization.
559-584: LGTM!The
__init__method properly:
- Stores settings for later use
- Clears existing handlers to prevent duplicates
- Sets up plugin handlers via
_setup_plugins()- Adds default stream handler only if no plugin provides one
- Sets the appropriate log level based on settings
- Marks initialization as complete
This consolidates what was previously in a separate
setup()method.
634-647: LGTM!The
_init_plugin_handlermethod correctly:
- Retrieves the plugin from the registry
- Validates settings before creating the handler
- Creates the handler with both output settings and plugin-specific settings
- Applies default filter/formatter only when the plugin hasn't provided its own
722-724: I'll help you verify this review comment about the queue listener thread cleanup. Let me start by examining the Python stdlib'sQueueListener.stop()behavior and the actual code context.
<function_calls>
<function_calls>
python3 << 'EOF'
import logging.handlers
import inspectGet the stop method
stop_method = logging.handlers.QueueListener.stop
Print the docstring
print("=== QueueListener.stop() docstring ===")
print(stop_method.doc)Get the source code
print("\n=== QueueListener.stop() source code ===")
try:
source = inspect.getsource(stop_method)
print(source)
except Exception as e:
print(f"Could not get source: {e}")Check if is_alive exists
print("\n=== Checking for is_alive() method ===")
print(f"Has is_alive: {hasattr(logging.handlers.QueueListener, 'is_alive')}")List available methods
print("\n=== QueueListener methods ===")
methods = [m for m in dir(logging.handlers.QueueListener) if not m.startswith('_')]
print(methods)
EOF
</function_calls>
<function_calls>
cat -n src/snakemake/logging.py | sed -n '710,735p'
</function_calls>
rg -n "queue_listener" src/snakemake/logging.py -A 2 -B 2
</function_calls>
700-720: Based on my investigation, I'm unable to fully verify the review comment due to repository access issues. However, I can provide an assessment:The web search confirms that Python's QueueHandler/QueueListener pattern is indeed used for thread-safe logging in plugins, but it doesn't provide Snakemake-specific implementation details. The concern raised in the original review comment—about potential log ordering differences between queue-based plugin handlers and directly-attached file handlers—is theoretically valid, but I cannot verify:
- Whether the queue listener is actually implemented for plugin handlers in this codebase
- Whether there are documented reasons for the direct file handler attachment design
- Whether synchronization mechanisms exist or are needed
Verify that the file handler is attached to the logger correctly.
The
setup_logfilemethod creates a file handler and adds it directly to the logger (line 716:self.logger.addHandler(handler)), while plugin handlers may use a queue-based architecture for thread-safety. Since the file handler is created per-workflow and doesn't go through the queue listener like plugin handlers do, there could potentially be log ordering inconsistencies between the two handler types. Confirm whether this direct attachment is intentional and whether log ordering between plugin handlers (via queue) and the file handler (direct) has been tested or is not a concern for the intended use case.
|
Will review asap - this looks great at first glance. Thank you @jlumpe. A few thoughts:
I agree that this would be ideal, but difficult to do. Lets talk about this more though.
I'm still not sure what this attribute is/was for. Perhaps @johanneskoester can chime in. |
jlumpe
force-pushed
the
logging-plugin-setup
branch
from
38a1465 to
4a0ee80
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/snakemake/logging.py (2)
586-632: Plugin handler setup is well-structured.The method correctly:
- Handles
skip_plugin_handlersearly return- Validates only one stream handler is configured
- Tracks file handlers for later reference
- Uses a
QueueListenerfor thread-safe plugin handler processingMinor note:
self._queueis dynamically assigned without a class-level declaration. While valid Python, declaring it alongside other instance attributes (likequeue_listener) would improve clarity.Consider declaring
_queueas a class attribute for clarity:queue_listener: Optional[logging.handlers.QueueListener] needs_rulegraph: bool logfile_handlers: dict[logging.Handler, str] settings: "OutputSettings" + _queue: Optional[Queue]
723-725: Reliance on private_threadattribute should be replaced with internal state tracking.The check
self.queue_listener._thread is not Noneaccesses a private implementation detail ofQueueListener. While this pattern is common, it is fragile because:
_threadis not part of the public API and is not guaranteed to remain stable across Python versions.QueueListener.stop()has no documented idempotence guarantee, so the guard is necessary—but relying on private attributes for this check couples your code to stdlib internals.A more robust approach is to track the running state internally:
+ _queue_listener_running: bool = False + def stop(self) -> None: - if self.queue_listener is not None and self.queue_listener._thread is not None: + if self.queue_listener is not None and self._queue_listener_running: self.queue_listener.stop() + self._queue_listener_running = FalseSet
self._queue_listener_running = Trueafter callingself.queue_listener.start().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/snakemake/api.py(5 hunks)src/snakemake/cli.py(0 hunks)src/snakemake/logging.py(2 hunks)src/snakemake/settings/types.py(0 hunks)src/snakemake/workflow.py(5 hunks)
💤 Files with no reviewable 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.pysrc/snakemake/workflow.pysrc/snakemake/api.py
🧠 Learnings (2)
📚 Learning: 2025-01-13T18:50:40.791Z
Learnt from: cademirch
Repo: snakemake/snakemake 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.
Applied to files:
src/snakemake/logging.pysrc/snakemake/workflow.pysrc/snakemake/api.py
📚 Learning: 2025-07-29T14:53:04.598Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3676
File: src/snakemake/cli.py:0-0
Timestamp: 2025-07-29T14:53:04.598Z
Learning: In the Snakemake repository, deprecated scheduler interfaces (like scheduler_ilp_solver, --scheduler-solver-path, --scheduler-greediness) should be kept for backward compatibility during the migration to the plugin-based scheduler system, rather than being removed immediately.
Applied to files:
src/snakemake/workflow.py
🧬 Code graph analysis (2)
src/snakemake/workflow.py (2)
src/snakemake/logging.py (3)
LoggerManager(532-725)get_logfile(683-684)logfile_hint(686-692)tests/logging/plugins/snakemake_logger_plugin_test/__init__.py (1)
needs_rulegraph(120-121)
src/snakemake/api.py (1)
src/snakemake/logging.py (4)
LoggerManager(532-725)setup_logfile(701-721)cleanup_logfile(694-699)stop(723-725)
🪛 Ruff (0.14.7)
src/snakemake/logging.py
612-612: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (45)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (8, macos-latest, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, macos-latest, py313)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
🔇 Additional comments (11)
src/snakemake/workflow.py (4)
67-67: LGTM!The import correctly brings in
LoggerManageralongsideloggerandformat_resources, aligning with the refactoring to use an instance-based logger manager.
155-155: LGTM!The
logger_managerfield is correctly added as a required dataclass field with proper type annotation. Its placement before optional fields ensures proper dataclass initialization order.
1215-1217: LGTM!The
needs_rulegraphcheck correctly uses the instance attributeself.logger_managerinstead of the previous module-level reference.
1480-1486: LGTM!All
logfile_hint()andget_logfile()calls are correctly updated to useself.logger_manager, maintaining consistency with the instance-based logging refactor.src/snakemake/api.py (4)
107-112: LGTM!The
logger_managerfield is correctly declared withfield(init=False)since it's initialized in__post_init__. The initialization properly creates aLoggerManagerinstance with the globalloggerandoutput_settings.
155-155: LGTM!The logfile setup is correctly invoked with the optional
workdirparameter before creating theWorkflowApi, ensuring file logging is ready before workflow operations begin.
170-177: LGTM!The cleanup is correctly structured:
cleanup_logfile()removes and closes file handlers,stop()halts the queue listener for plugin handlers, and the workflow teardown follows. The unconditional cleanup aligns with the per-instance LoggerManager lifecycle.
395-406: LGTM!The
logger_manageris correctly propagated fromSnakemakeApitoWorkflowvia the constructor, ensuring the workflow has access to the centralized logging management.src/snakemake/logging.py (3)
24-25: LGTM!The imports for
LoggerPluginRegistryandLogHandlerSettingsBaseare correctly added to support the plugin handler initialization logic.
561-584: Initialization correctly consolidates setup logic.The
__init__method properly:
- Clears existing handlers to prevent duplicates
- Sets up plugin handlers and checks if any write to stream
- Adds a default stream handler only if needed
- Configures the logger level based on settings
Note: Since a global
loggerinstance is shared, clearing handlers in__init__means creating a newLoggerManagerwill reset handlers from any previous instance. The PR description acknowledges this limitation and defers a full refactor to a separate issue.
634-647: LGTM!The method cleanly instantiates plugin handlers and applies default filter/formatter only when the plugin handler doesn't provide its own, following a sensible convention over configuration approach.
|
Rebased onto current master, including #3848. Bioconda appears to be down, which is causing the failing tests. |
|
I also removed the def cleanup_logfile(self) -> None:
if self.settings.enable_file_logging:
for handler in self.logfile_handlers.keys():
self.logger.removeHandler(handler)
handler.close()
self.logfile_handlers.clear()This was only called in |
Re-add keep_logger field and restore its original behavior to avoid breaking the public API. The original purpose of this attribute is unclear, but still may be used my API users. Added a deprecation warning. Scheduled for removal in v10.0.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake/api.py (1)
172-182: Deprecation handling is clear and appropriate.The warning message effectively communicates the deprecation and invites users to provide feedback if they depend on this feature. The conditional cleanup via
logger_manager.stop()ensures proper resource management for the typical case.Optional: Consider adjusting stacklevel for better warning attribution
The current
stacklevel=2points to__exit__. Usingstacklevel=3would point to the user'swithstatement, making it easier for users to locate where the deprecated setting is being used:warnings.warn( "OutputSettings.keep_logger is deprecated and will be removed in v10.0. " "If you rely on this behavior, please open an issue describing your use case.", FutureWarning, - stacklevel=2, + stacklevel=3, )However, since the warning message is clear, this is a minor improvement.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/snakemake/api.pysrc/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/api.pysrc/snakemake/settings/types.py
🧠 Learnings (2)
📚 Learning: 2025-01-13T18:50:40.791Z
Learnt from: cademirch
Repo: snakemake/snakemake 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.
Applied to files:
src/snakemake/api.py
📚 Learning: 2025-07-29T14:53:04.598Z
Learnt from: johanneskoester
Repo: snakemake/snakemake PR: 3676
File: src/snakemake/cli.py:0-0
Timestamp: 2025-07-29T14:53:04.598Z
Learning: In the Snakemake repository, deprecated scheduler interfaces (like scheduler_ilp_solver, --scheduler-solver-path, --scheduler-greediness) should be kept for backward compatibility during the migration to the plugin-based scheduler system, rather than being removed immediately.
Applied to files:
src/snakemake/settings/types.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (46)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (4, macos-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: apidocs
🔇 Additional comments (6)
src/snakemake/settings/types.py (2)
419-420: Clear deprecation notice, well documented.The deprecation message clearly indicates the field is non-functional and will be removed in v10.0, which aligns with the project's backward compatibility practices.
436-436: Good practice to position deprecated field at end of dataclass.Maintaining the field for backward compatibility while clearly marking it as deprecated follows the project's approach for managing deprecated interfaces.
src/snakemake/api.py (4)
59-59: Appropriate import for the refactored logging architecture.Importing
LoggerManagerandloggersupports the per-instance logging management introduced in this PR.
155-155: Appropriate placement of logfile setup.Setting up the logfile after resolving the snakefile and before creating the WorkflowApi ensures logging is properly configured for the workflow execution.
409-409: Correct propagation of LoggerManager to Workflow.Passing the
logger_managerto the Workflow constructor ensures consistent logging context throughout the workflow execution.
107-112: Approve the LoggerManager initialization in SnakemakeApi.The per-instance LoggerManager architecture is sound. The LoggerManager class orchestrates logging behavior, ensuring appropriate output whether Snakemake runs in the main process, subprocesses, or remote execution environments. The constructor correctly accepts a logger instance and OutputSettings, and both setup_logfile() and stop() methods are properly implemented to manage the workflow logging lifecycle.
- Resolved conflicts with LoggerManager refactor from snakemake#3851 - Integrated plugin_handlers storage with new _setup_plugins() architecture - Updated get_log_handlers() to use logger_manager.get_log_handlers()
🤖 I have created a release *beep* *boop* --- ## [9.14.6](v9.14.5...v9.14.6) (2026-01-08) ### Bug Fixes * create local clone of git repos for source files from hosting providers ([#3643](#3643)) ([d2f8aba](d2f8aba)) * create potentially missing .snakemake folder in case of very long command lines for spawned jobs ([#3894](#3894)) ([4b431dd](4b431dd)) * make ilp solver enumeration lazy ([#3900](#3900)) ([30e1509](30e1509)) * Prevent broken report_href links by using deterministic report IDs with fixed prefix length ([#3889](#3889)) ([6d8f4d8](6d8f4d8)) * refactor LoggerManager setup and scope ([#3851](#3851)) ([f46d904](f46d904)) * yield proper error message in case a local git source file is not retrievable ([#3892](#3892)) ([ed79cae](ed79cae)) ### Documentation * explain how to pass nested config via CLI ([#3885](#3885)) ([9d8c539](9d8c539)) --- 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
Currently there is a single global
LoggerManagerinstance, with itssetup()method called fromSnakemakeApi's constructor. This is a potential source of issues if the global instance gets used multiple times in the same Python session, as happens when running the test suite. It's also not very necessary, as the global is only used in a couple of places (api.pyandworkflow.py), and there's not really any setup or configuration that actually needs to be shared (a couple of exceptions are mentioned below).This PR removes the global instance and instead creates a new one attached to each
SnakemakeApiinstance. This instance is also now passed explicitly toWorkflow's constructor. Because the setup now happens when theLoggerManageris created, I removed thesetup()function and merged its contents into__init__().I also refactored and simplified some of the code previously in
setup(), and also moved the initial creation of plugin log handlers fromSnakemakeApi.setup_logger()toLoggerManager.__init__().Edit: Also removed the
cleanup_logfile()method and instead remove all handlers from the logger instop().Potential issues
No tests
Currently there are no tests for most of this behavior, that is fixed in #3848. I have personally tested that those tests still succeed with these changes, but probably best to merge that PR first.
Global logger
There is still a single global
Loggerinstance, which gets (mostly) reset and then set up again for eachLoggerManagerinstance. Ideally eachLoggerManagerwould create its own logger, however this is much more difficult to refactor because the global logger is used extensively throughout the codebase. I'll leave that to a separate PR/issue.OutputSettings.keep_loggerSetup and teardown of the global
LoggerManagerwas previously skipped ifOutputSettings.keep_loggerwas True. As far as I can tell this was never the case - the dataclass field's default value is False, I checked all instances I could find of the constructor being called and the only case that sets the field is insnakemake/cli.py:create_output_settings(), where it is always set to False (link). I removed the attribute fromOutputSettings. If we want to optionally allow for similar behavior, I could add an argument toSnakemakeApi's constructor that accepts an existing instance.Default stream handler level
In current
LoggerManager.setup():I removed this if statement because most of the code was duplicated below (some of it is conditional on no plugin handlers outputting to stream, which is always the case if there are no handlers or plugins are skipped). The only thing that is different is that the default stream handler's level is set based on
log_level_override, which doesn't happen otherwise. I'm pretty confident this has no effect because theLoggerthe handler is attached to is set to the same level.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.