fix: Minor fixes/additions to logging module.#3802
fix: Minor fixes/additions to logging module.#3802cademirch merged 11 commits intosnakemake:mainfrom
Conversation
📝 WalkthroughWalkthroughReplaces the logging event-extraction API: Changes
Sequence Diagram(s)sequenceDiagram
participant Record as LogRecord
participant GE as get_event(record)
participant LM as LoggerManager
participant HF as Handler(s)
Note over Record,GE: New single-step event extraction
Record->>GE: pass record
GE-->>Record: Optional(LogEvent) / None
alt Event present
Record->>LM: format/filter using LogEvent
LM->>HF: emit formatted message
else No event
Record->>LM: use record.msg / default formatting
LM->>HF: emit formatted message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas to review closely:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
⏰ 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)
🔇 Additional comments (2)
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 |
jlumpe
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/snakemake/logging.py(16 hunks)src/snakemake/settings/types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of 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/settings/types.py
🧬 Code graph analysis (1)
src/snakemake/logging.py (2)
src/snakemake/settings/enums.py (1)
Quietness(24-29)src/snakemake/settings/types.py (1)
OutputSettings(397-434)
🪛 Ruff (0.14.0)
src/snakemake/logging.py
162-162: Unused method argument: msg
(ARG002)
⏰ 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 (7, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (7, macos-latest, py313)
- GitHub Check: tests (10, ubuntu-latest, py313)
- GitHub Check: tests (5, macos-latest, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (7, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (7, ubuntu-latest, py313)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (3, macos-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (2, macos-latest, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py313)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (1, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: apidocs
🔇 Additional comments (12)
src/snakemake/settings/types.py (1)
398-434: LGTM! Excellent documentation addition.The comprehensive docstring clearly documents all attributes of the
OutputSettingsclass, improving code maintainability and developer experience.src/snakemake/logging.py (11)
19-27: LGTM! Type annotation improvements.The import changes correctly support the refined type annotations, with
OutputSettingsproperly forward-referenced as a string for the TYPE_CHECKING block.
106-108: LGTM! Good API simplification.Replacing
get_event_levelwithget_eventremoves an unused return value, making the API cleaner and more focused.
132-156: LGTM! Consistent with API refactor.The changes correctly use the simplified
get_eventAPI. The change tord["msg"] or ""assumes themsgkey is always present in the record dict (which is standard forlogging.LogRecord), and theor ""handlesNonevalues appropriately.
158-219: Type annotations improve clarity.The addition of
dict[str, Any]parameter annotations to formatter methods is a good improvement for code clarity and type safety.
234-331: LGTM! Consistent type annotations.The parameter type annotations for the helper methods are consistent with the rest of the formatter class.
334-346: LGTM! Clear documentation.The docstring clearly documents the
DefaultFilterclass attributes and their purposes.
367-406: LGTM! Consistent with API refactor.The change to use
get_event(record)is correct. The level value was previously discarded, confirming it was unused.
478-508: LGTM! Correct API usage.The change correctly uses the simplified
get_eventAPI.
530-570: LGTM! Improved documentation and type precision.The comprehensive docstring clearly documents the
LoggerManagerclass and its attributes. The type annotation changes fromOutputSettingsLoggerInterfaceto"OutputSettings"correctly reflect the actual concrete type being used, improving type safety.
638-638: LGTM! Important bug fix.Setting
self.initialized = Trueat the end ofsetup()correctly marks the logger as initialized. This was missing before and is mentioned in the PR objectives as a fix.
510-527: Behavioral equivalence verified — change is correct.The refactoring replaces
get_event_level(record)with direct access torecord.levelno, which is the standard Python logging API. Theget_event()function now only returns the event, and the log level is accessed directly from the record object viarecord.levelno— functionally identical to the previous implementation while being simpler and more maintainable.
| def format_host(self, msg: dict[str, Any]): | ||
| """Format for host log.""" | ||
| return f"host: {platform.node()}" |
There was a problem hiding this comment.
🧩 Analysis chain
Note: Unused parameter flagged by static analysis.
Static analysis reports that the msg parameter in format_host is unused. The method returns a static string and doesn't reference the parameter. Additionally, this method doesn't appear to be registered in the formatters dictionary (lines 142-153), suggesting it may be dead code.
Consider either:
- Removing the unused
msgparameter if it's not needed by the method signature contract - Removing the entire method if it's unused
🏁 Script executed:
#!/bin/bash
# Search for calls to format_host to verify if it's used
rg -nP '\bformat_host\b' --type=py -g '!test*'Length of output: 76
I need to verify the formatters dictionary and check for dynamic method invocations.Let me search the codebase properly for the formatters dictionary and any dynamic invocations.
[scratchpad_update]
- Need to examine the formatters dictionary definition to confirm if format_host is registered
- Need to check for dynamic method calls that might invoke format_host
🏁 Script executed:
#!/bin/bash
# First, view the formatters dictionary in the logging file (lines 142-153)
echo "=== Formatters Dictionary (lines 140-160) ==="
sed -n '140,160p' src/snakemake/logging.py
echo -e "\n=== Search for dynamic method invocations (getattr, format patterns) ==="
# Look for patterns that might dynamically call formatters
rg -nP -A2 -B2 'getattr|formatters\[|format_' src/snakemake/logging.py | head -50Length of output: 2725
Let me check for any dynamic method invocations and verify the method is truly unused:
🏁 Script executed:
#!/bin/bash
# Check the complete context around format_host and method calling patterns
echo "=== Full context around format_host (lines 158-170) ==="
sed -n '158,170p' src/snakemake/logging.py
echo -e "\n=== Search for dynamic method invocations (f-strings, format method names, etc) ==="
rg -nP 'f.*format_|getattr.*format|setattr.*format' src/snakemake/logging.py
echo -e "\n=== Check if method names are built dynamically ==="
rg -nP 'method|formatter.*=.*"' src/snakemake/logging.py | head -20Length of output: 3206
Remove the format_host method as it is dead code.
The verification confirms the review findings: format_host is not registered in the formatters dictionary (lines 142-153) and has no callers in the codebase. The method is defined but unused, and the msg parameter is not referenced in its body. Since the method serves no purpose, it should be removed entirely rather than retained with an unused parameter.
🧰 Tools
🪛 Ruff (0.14.0)
162-162: Unused method argument: msg
(ARG002)
🤖 Prompt for AI Agents
In src/snakemake/logging.py around lines 162-164, remove the dead method
format_host(msg: dict[str, Any]) entirely: delete its definition and any unused
parameter, and ensure there are no remaining references; no other changes to
formatters dict are required since it is not registered or called anywhere.
cademirch
left a comment
cademirch
left a comment
There was a problem hiding this comment.
looks good to me, thanks!
|
Tests failing currently are unrelated to this PR. Think its related to cloudlflare outage today, as its happening in other repo of mine. |
🤖 I have created a release *beep* *boop* --- ## [9.14.0](v9.13.7...v9.14.0) (2025-11-27) ### Features * Support Hy in script directive ([#3824](#3824)) ([2329c9e](2329c9e)) ### Bug Fixes * Add support for pathlib in `notebook` field ([#3811](#3811)) ([7b2180a](7b2180a)) * Addressed race condition in workdir_handler.py ([#3844](#3844)) ([8dbfcfb](8dbfcfb)) * cleanup update-marked output files of failed jobs if there was no backup to restore them ([#3843](#3843)) ([41f1ce8](41f1ce8)) * correct Windows callable path handling ([#3832](#3832)) ([5caad70](5caad70)) * expand env vars on resources ([#3823](#3823)) ([fcfa1bc](fcfa1bc)) * fix backup for output marked by `update` ([#3839](#3839)) ([09c64b7](09c64b7)) * Minor fixes/additions to logging module. ([#3802](#3802)) ([3b3986d](3b3986d)) * mount local storage prefix into containers ([#3840](#3840)) ([f1e8b62](f1e8b62)) * properly format input/output files in case of missing rule to produce them ([#3849](#3849)) ([69d5d24](69d5d24)) * Unpack AnnotatedString in _apply_wildcards ([#3798](#3798)) ([7886508](7886508)) ### Performance Improvements * retrieve storage inputs immediately before scheduling jobs instead of before running the entire workflow ([#3850](#3850)) ([4ac6cda](4ac6cda)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
- Set the `initialized` attribute in `LoggerManager.setup()`. - Remove incorrect `if` statement in `DefaultFilter.filter()` that never matched. - Remove unused `DefaultFormatter` methods `format_info()` and `format_d3dag()`. - Replace the `get_event_level()` function with `get_event()` as the other return value wasn't being used. - Correct `LoggerManager.settings` annotation from `OutputSettingsLoggerInterface` to `OutputSettings` (uses attributes not defined in the former). - Add parameter annotations to some `DefaultFormatter` methods. - Document attributes for for `OutputSettings`, `LoggerManager`, and `DefaultFilter` classes. ### QC <!-- Make sure that you can tick the boxes below. --> * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Improved internal logging/event extraction and formatting flow, with stricter type annotations for more predictable behavior and maintainability. * **Chores** * Expanded documentation for output settings and simplified a default setting representation for clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Cade Mirchandani <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [9.14.0](snakemake/snakemake@v9.13.7...v9.14.0) (2025-11-27) ### Features * Support Hy in script directive ([snakemake#3824](snakemake#3824)) ([2329c9e](snakemake@2329c9e)) ### Bug Fixes * Add support for pathlib in `notebook` field ([snakemake#3811](snakemake#3811)) ([7b2180a](snakemake@7b2180a)) * Addressed race condition in workdir_handler.py ([snakemake#3844](snakemake#3844)) ([8dbfcfb](snakemake@8dbfcfb)) * cleanup update-marked output files of failed jobs if there was no backup to restore them ([snakemake#3843](snakemake#3843)) ([41f1ce8](snakemake@41f1ce8)) * correct Windows callable path handling ([snakemake#3832](snakemake#3832)) ([5caad70](snakemake@5caad70)) * expand env vars on resources ([snakemake#3823](snakemake#3823)) ([fcfa1bc](snakemake@fcfa1bc)) * fix backup for output marked by `update` ([snakemake#3839](snakemake#3839)) ([09c64b7](snakemake@09c64b7)) * Minor fixes/additions to logging module. ([snakemake#3802](snakemake#3802)) ([3b3986d](snakemake@3b3986d)) * mount local storage prefix into containers ([snakemake#3840](snakemake#3840)) ([f1e8b62](snakemake@f1e8b62)) * properly format input/output files in case of missing rule to produce them ([snakemake#3849](snakemake#3849)) ([69d5d24](snakemake@69d5d24)) * Unpack AnnotatedString in _apply_wildcards ([snakemake#3798](snakemake#3798)) ([7886508](snakemake@7886508)) ### Performance Improvements * retrieve storage inputs immediately before scheduling jobs instead of before running the entire workflow ([snakemake#3850](snakemake#3850)) ([4ac6cda](snakemake@4ac6cda)) --- 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
initializedattribute inLoggerManager.setup().ifstatement inDefaultFilter.filter()that never matched.DefaultFormattermethodsformat_info()andformat_d3dag().get_event_level()function withget_event()as the other return value wasn't being used.LoggerManager.settingsannotation fromOutputSettingsLoggerInterfacetoOutputSettings(uses attributes not defined in the former).DefaultFormattermethods.OutputSettings,LoggerManager, andDefaultFilterclasses.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
Refactor
Chores