fix: improve labelling of input and output files when logging rules#3516
fix: improve labelling of input and output files when logging rules#3516johanneskoester merged 5 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughThe changes involve the removal of the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (1)`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
⏰ Context from checks skipped due to timeout of 90000ms (30)
🔇 Additional comments (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
src/snakemake/jobs.py(1 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/jobs.py
🪛 Ruff (0.8.2)
src/snakemake/jobs.py
76-76: Undefined name phrase
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (33)
- 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, 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 (3)
src/snakemake/jobs.py (3)
81-84: Good addition of helper function for consistent formattingThe new
annotatehelper function nicely consolidates the formatting logic that was previously duplicated across multiple conditions, improving maintainability and consistency.
87-98: Clean refactoring using the new annotate functionReplacing multiple similar code blocks with calls to the
annotatefunction is a good refactoring that makes the code more maintainable and consistent.
103-103: Consistent formatting for sourcecache entriesUsing the
annotatefunction for sourcecache entries maintains consistency with the formatting of other file types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake/jobs.py (1)
69-76: Consider using a ternary operator for cleaner code.The code defines the storage-related
phraseusing an if-else block, which works correctly but could be simplified using a ternary operator.if f.is_storage: if is_input: - if f.storage_object.retrieve: - phrase = "retrieve from" - else: - phrase = "keep remote on" + phrase = "retrieve from" if f.storage_object.retrieve else "keep remote on" else: phrase = "send to" f_str = f.storage_object.print_query storage_phrase = f"{phrase} storage"🧰 Tools
🪛 Ruff (0.8.2)
69-72: Use ternary operator
phrase = "retrieve from" if f.storage_object.retrieve else "keep remote on"instead ofif-else-blockReplace
if-else-block withphrase = "retrieve from" if f.storage_object.retrieve else "keep remote on"(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/jobs.py(1 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/jobs.py
🪛 Ruff (0.8.2)
src/snakemake/jobs.py
69-72: Use ternary operator phrase = "retrieve from" if f.storage_object.retrieve else "keep remote on" instead of if-else-block
Replace if-else-block with phrase = "retrieve from" if f.storage_object.retrieve else "keep remote on"
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (35)
- 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, macos-latest, py312)
- 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, 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)
- GitHub Check: apidocs
🔇 Additional comments (3)
src/snakemake/jobs.py (3)
81-84: Great addition of theannotatehelper function.The introduction of the
annotatefunction effectively consolidates the string formatting logic that was previously scattered throughout the code. This approach improves code readability and maintainability by centralizing the annotation logic in one place.
87-98: Good refactoring of the flag handling code.The consistent use of the
annotatefunction for all flag conditions is a significant improvement. This creates a more uniform approach to string formatting and makes the code easier to maintain, as any changes to the annotation format only need to be made in one place.
103-103: Consistent handling of sourcecache entries.The sourcecache entry case now also uses the
annotatefunction, maintaining consistency with the other conditions in the function. This completes the refactoring approach across all relevant code paths.
|
Please format your code with black: |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/snakemake/io/fmt.py (2)
5-47: Well-structured new utility function for file formatting.The
fmt_iofilefunction properly handles different file types and provides consistent labeling across various storage scenarios. This implementation will make log messages more informative and consistent.However, adding a docstring would improve usability for other developers:
def fmt_iofile(f, as_input: bool = False, as_output: bool = False): + """Format I/O file objects with appropriate annotations based on their flags and storage status. + + Parameters: + f: The file object to format + as_input: Whether to treat the file as input + as_output: Whether to treat the file as output + + Returns: + Formatted string representation of the file with appropriate annotations + """ as_io = as_input or as_output
28-30: Inconsistent indentation at line 29.The line is indented with 8 spaces rather than the 4 spaces used in the rest of the function.
if is_flagged(f, "sourcecache_entry"): - orig_path_or_uri = get_flag_value(f, "sourcecache_entry") - return annotate(orig_path_or_uri, "cached") + orig_path_or_uri = get_flag_value(f, "sourcecache_entry") + return annotate(orig_path_or_uri, "cached")src/snakemake/dag.py (1)
930-937: Missed opportunity for fmt_iofile in unneeded_files logic.While most file logging has been updated, there's an opportunity to enhance formatting in one more place:
def unneeded_files(): # temp input for job_, files in self._dependencies[job].items(): tempfiles = set(f for f in job_.output if is_temp(f)) yield from filterfalse( partial(self.is_needed_tempfile, job_), tempfiles & files )This method yields files but doesn't directly format the output. When these files are used in logging at lines 947 and 949, they benefit from the fmt_iofile changes, so no actual changes are needed here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/snakemake/dag.py(8 hunks)src/snakemake/exceptions.py(1 hunks)src/snakemake/io/__init__.py(1 hunks)src/snakemake/io/fmt.py(1 hunks)src/snakemake/jobs.py(2 hunks)src/snakemake/report/__init__.py(1 hunks)src/snakemake/template_rendering/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/snakemake/jobs.py
🧰 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/template_rendering/__init__.pysrc/snakemake/report/__init__.pysrc/snakemake/io/__init__.pysrc/snakemake/exceptions.pysrc/snakemake/io/fmt.pysrc/snakemake/dag.py
🧬 Code Definitions (3)
src/snakemake/template_rendering/__init__.py (2)
src/snakemake/io/fmt.py (1)
fmt_iofile(5-47)src/snakemake/io/__init__.py (1)
storage_object(455-456)
src/snakemake/io/fmt.py (2)
src/snakemake/common/tbdstring.py (1)
TBDString(3-96)src/snakemake/io/__init__.py (5)
get_flag_value(1158-1163)is_flagged(85-86)is_flagged(971-974)is_storage(422-423)storage_object(455-456)
src/snakemake/dag.py (1)
src/snakemake/io/fmt.py (1)
fmt_iofile(5-47)
🪛 Ruff (0.8.2)
src/snakemake/template_rendering/__init__.py
68-68: Undefined name fmt_iofile
(F821)
src/snakemake/io/__init__.py
1033-1033: Undefined name fmt_iofile
(F821)
1035-1035: Undefined name fmt_iofile
(F821)
src/snakemake/exceptions.py
387-404: .format call has unused named argument(s): f
Remove extra named arguments: f
(F522)
387-404: .format call is missing argument(s) for placeholder(s): fmt_iofile(f)
(F524)
⏰ Context from checks skipped due to timeout of 90000ms (36)
- 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, macos-latest, py312)
- 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)
- GitHub Check: apidocs
🔇 Additional comments (8)
src/snakemake/dag.py (8)
26-26: LGTM! New import added for the file formatting function.The addition of
fmt_iofileimport enables consistent formatting of file objects in log messages.
413-413: Improved log message with better file labeling.Using
fmt_iofile(f)instead of justfprovides more informative output when retrieving files from storage.
877-877: Enhanced log message clarity for write-protected files.The use of
fmt_iofile(f)improves output legibility when handling protected files.
885-885: Better file representation in touching output logs.Using
fmt_iofile(f)makes the log message about touching output files more informative.
947-949: Improved temp file removal logging.The changes enhance the clarity of messages for temporary output handling, whether they are being removed or would be removed in a dry run.
1041-1041: Better formatting for storage file removal logs.Using
fmt_iofile(f)provides clearer information when removing local copies of storage files.
1863-1866: Improved error message for pipe output conflicts.The error message now includes better formatting for pipe files in consumer rules, making it easier to identify problematic files.
2275-2276: Enhanced job priority message formatting.Log messages about job priority adjustments now have improved file representation using
fmt_iofile(f).
🤖 I have created a release *beep* *boop* --- ## [9.1.8](v9.1.7...v9.1.8) (2025-04-07) ### Bug Fixes * improve labelling of input and output files when logging rules ([#3516](#3516)) ([56203a4](56203a4)) * unify error logging in scheduler, fixing a missing attribute error for group jobs ([#3515](#3515)) ([245ec10](245ec10)) ### Documentation * activate gurubase AI ([#3517](#3517)) ([eaffc6e](eaffc6e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…nakemake#3516) ### Description <!--Add a description of your PR here--> ### 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** - Enhanced the display of file storage details by streamlining the formatting of output messages. Users will now see more consistent and clear labeling of file storage information across different contexts. - Updated error and log messages to utilize improved file formatting, enhancing clarity and readability in various components of the application. - Introduced a new function for formatting input and output files, simplifying previous implementations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [9.1.8](snakemake/snakemake@v9.1.7...v9.1.8) (2025-04-07) ### Bug Fixes * improve labelling of input and output files when logging rules ([snakemake#3516](snakemake#3516)) ([56203a4](snakemake@56203a4)) * unify error logging in scheduler, fixing a missing attribute error for group jobs ([snakemake#3515](snakemake#3515)) ([245ec10](snakemake@245ec10)) ### Documentation * activate gurubase AI ([snakemake#3517](snakemake#3517)) ([eaffc6e](snakemake@eaffc6e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
1 participant
Description
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