feat: custom log file behaviour #159
Conversation
WalkthroughThe pull request introduces enhancements to the Snakemake SLURM executor plugin, focusing on improved log management. Key changes include the addition of a method for cleaning old log files, expanded settings for log configuration, and new command-line flags for user control over log retention and directory management. The documentation has been updated to reflect these changes, providing clearer guidance on utilizing the new features effectively. Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
snakemake_executor_plugin_slurm/utils.py (2)
19-23: Enhance function documentation and add type hints.The function documentation should include:
- Parameter descriptions with types
- Return value (if any)
- Examples of usage
- Explanation of the age cutoff unit (days)
-def clean_old_logs(logdir, age_cutoff): +def clean_old_logs(logdir: str, age_cutoff: float) -> None: """ - Function to delete files older than 'age_cutoff' - in the SLURM 'logdir' + Delete files in the SLURM log directory that are older than the specified age cutoff. + + Args: + logdir: Path to the SLURM log directory + age_cutoff: Maximum age of files in days before deletion (0 or negative to keep all files) + + Example: + >>> clean_old_logs("/path/to/logs", 10.0) # Delete files older than 10 days """
24-24: Replace magic number with a named constant.The conversion factor from days to seconds should be defined as a constant at module level for better maintainability.
+# Constants +SECONDS_PER_DAY = 24 * 60 * 60 # 86400 seconds in a day + def clean_old_logs(logdir: str, age_cutoff: float) -> None: """ ... """ - cutoff_secs = age_cutoff * 86400 + cutoff_secs = age_cutoff * SECONDS_PER_DAYsnakemake_executor_plugin_slurm/__init__.py (6)
51-54: Correct typographical error in help textThere's a misspelling in the help text for
keep_successful_logs. The word "sucessful" should be "successful". Additionally, consider replacing "Per default" with "By default" for improved clarity.Apply this diff to fix the typo and improve phrasing:
- Per default SLURM log files will be deleted upon sucessful completion + By default, SLURM log files will be deleted upon successful completion
150-152: Remove commented-out codeThe method
delete_old_logsis commented out and appears to be obsolete due to the introduction ofclean_old_logs. Removing unused code helps maintain clarity.Apply this diff to remove the commented code:
- # def delete_old_logs(self): - # self.workflow.executor_settings.delete_logfiles_older_than
432-436: Handle exceptions when removing log filesWhen deleting log files using
os.remove, it's advisable to handle potential exceptions such asFileNotFoundErrorto prevent the application from crashing if the file does not exist.Modify the code to include exception handling:
try: os.remove(j.aux["slurm_logfile"]) except FileNotFoundError: self.logger.warning(f"Log file {j.aux['slurm_logfile']} not found when attempting to remove.") except Exception as e: self.logger.error(f"Error removing log file {j.aux['slurm_logfile']}: {e}")
51-54: Standardize help text formattingThe help texts for the fields in
ExecutorSettingshave inconsistent indentation and spacing. Standardizing the formatting improves readability and maintains consistency across the codebase.Consider adjusting the help texts to align properly and use consistent indentation.
Also applies to: 64-65, 71-73
Line range hint
139-147: Ensuredoneflag inwarn_on_jobcontextmethod is correctly managedIn the
warn_on_jobcontextmethod, thedoneparameter is checked but might not be managed correctly. Sincedoneis not stored or updated outside the method scope, the recursive call towarn_on_jobcontext(done=True)has no practical effect.Consider refactoring the method to properly manage the
donestate or remove the parameter if it's unnecessary.🧰 Tools
🪛 Ruff
139-140: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
Line range hint
281-283: Handle potential exceptions when parsingslurm_jobidWhen parsing
slurm_jobidfrom thesbatchoutput, ensure that the code handles cases where the output might not be as expected, which could lead to anIndexError.Add a check before accessing
out.strip().split(";")[0]:slurm_jobid_parts = out.strip().split(";") if slurm_jobid_parts: slurm_jobid = slurm_jobid_parts[0] else: raise WorkflowError("Failed to retrieve SLURM job ID from sbatch output.")🧰 Tools
🪛 Ruff
15-15:
pathlibimported but unused(F401)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(6 hunks)snakemake_executor_plugin_slurm/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff
snakemake_executor_plugin_slurm/__init__.py
15-15: pathlib imported but unused
(F401)
🔇 Additional comments (4)
snakemake_executor_plugin_slurm/__init__.py (4)
61-66: Clarify behavior when delete_logfiles_older_than is set to 0
The help text mentions that setting delete_logfiles_older_than to '0' disables deletion of old log files. Since the field is defined as an Optional[int], it's more appropriate to use 0 (without quotes) to represent an integer value. Ensure that this behavior is correctly implemented and consider clarifying the help text.
181-184: Update assertion to reflect new slurm_logdir construction
With the changes to slurm_logfile, ensure that the assertion checking for %j in the directory path is still valid and correctly reflects the new path construction.
186-186: Ensure slurm_logdir is correctly created
After reconstructing slurm_logfile using os.path.join, verify that slurm_logdir is correctly obtained using os.path.dirname(slurm_logfile) and that the directory is created as intended.
61-66: Confirm type handling for delete_logfiles_older_than
Ensure that the delete_logfiles_older_than field, which is an Optional[int], correctly handles the value 0 to disable log deletion. Verify that the logic in clean_old_logs interprets 0 appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
169-171: Use consistent f-string formattingThe comment string construction can be simplified using f-strings consistently.
Apply this diff to improve readability:
- comment_str = f"{group_or_rule}" - else: - comment_str = f"{group_or_rule}_wildcards_{wildcard_str}" + comment_str = group_or_rule + else: + comment_str = f"{group_or_rule}_wildcards_{wildcard_str}"
504-505: LGTM! Consider using a single f-stringThe error logging improvement is good, but the string concatenation can be simplified.
Apply this diff for better readability:
- "The job status query failed with" - f" command: {command}\n" + f"The job status query failed with command: {command}\n"
…de within __postinit__
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
51-54: Fix typos in help textThere are typos in the help text:
- "sucessful" should be "successful"
- The sentence structure could be improved
Per default SLURM log files will be deleted upon - sucessful completion + successful completion of a job. Whenever a SLURM job fails, its log file will be preserved.
455-459: Add error handling for log file deletionThe log file deletion should handle potential errors gracefully, as the file might have been deleted by another process or the user.
if not self.workflow.executor_settings.keep_successful_logs: self.logger.debug( f"removing log for successful job with SLURM ID '{j.external_jobid}'" ) - os.remove(j.aux["slurm_logfile"]) + try: + if os.path.exists(j.aux["slurm_logfile"]): + os.remove(j.aux["slurm_logfile"]) + except OSError as e: + self.logger.warning(f"Could not remove log file {j.aux['slurm_logfile']}: {e}")
173-175: Remove commented-out codeThe commented-out method
delete_old_logsshould be removed as it's no longer needed and can cause confusion.- # def delete_old_logs(self): - # self.workflow.executor_settings.delete_logfiles_older_than -
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(6 hunks)snakemake_executor_plugin_slurm/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff
snakemake_executor_plugin_slurm/__init__.py
30-30: .utils.clean_old_logs imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
131-131: Redefinition of unused clean_old_logs from line 30
(F811)
snakemake_executor_plugin_slurm/utils.py
4-4: time imported but unused
Remove unused import: time
(F401)
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
35-46:
Use os.path.expanduser('~') for more robust home directory handling
Using os.environ['USER'] directly can fail if the USER environment variable is not set. Using os.path.expanduser('~') is more robust and cross-platform compatible.
logdir: Optional[str] = field(
- default=f"/home/{os.environ['USER']}/.snakemake/slurm_logs",
+ default=os.path.join(os.path.expanduser('~'), ".snakemake", "slurm_logs"),
metadata={Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
177-179: Remove commented-out codeThe commented-out
delete_old_logsmethod is no longer needed as the functionality has been implemented elsewhere.Remove these commented-out lines to improve code cleanliness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(7 hunks)snakemake_executor_plugin_slurm/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_slurm/utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
47-60: Fix typo in documentation stringThere's a typo in the help text: "sucessful" should be "successful".
- Per default SLURM log files will be deleted upon sucessful + Per default SLURM log files will be deleted upon successful
61-72: Validate delete_logfiles_older_than valueWhile the code checks for values ≤ 0, there should be validation for extremely large values that could cause performance issues. Consider adding an upper limit.
delete_logfiles_older_than: Optional[int] = field( default=10, + validators=[field(lambda x: 0 <= x <= 365, "Value must be between 0 and 365 days")], metadata={
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(7 hunks)
🔇 Additional comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
199-204: 🛠️ Refactor suggestion
Use os.path.join for log file path construction
The current implementation uses string concatenation which is error-prone and not cross-platform friendly.
- slurm_logfile = (
- self.workflow.executor_settings.logdir
- + os.path.sep
- + f"{group_or_rule}/{wildcard_str}/%j.log"
- )
+ slurm_logfile = os.path.join(
+ self.workflow.executor_settings.logdir,
+ group_or_rule,
+ wildcard_str,
+ "%j.log"
+ )Likely invalid or redundant comment.
135-163: 🛠️ Refactor suggestion
Move clean_old_logs outside post_init
The current implementation has several issues:
- Function defined inside post_init reduces maintainability and testability
- Potential race condition in directory removal
- Missing type hints
Move the function to class level and improve its implementation:
class Executor(RemoteExecutor):
+ def clean_old_logs(self, logdir: str, age_cutoff: int) -> None:
+ """Delete files older than specified age from the SLURM log directory.
+
+ Args:
+ logdir: Path to the log directory
+ age_cutoff: Number of days after which files should be deleted
+ """
+ if age_cutoff <= 0:
+ return
+ cutoff_secs = age_cutoff * 86400
+ current_time = time.time()
+ self.logger.info(f"Cleaning up log files older than {age_cutoff} day(s)")
+
+ for root, _, files in os.walk(logdir, topdown=False):
+ for fname in files:
+ file_path = os.path.join(root, fname)
+ try:
+ file_age = current_time - os.stat(file_path).st_mtime
+ if file_age > cutoff_secs:
+ os.remove(file_path)
+ except (OSError, FileNotFoundError) as e:
+ self.logger.warning(f"Could not delete file {file_path}: {e}")
+ # Remove empty directories
+ try:
+ if os.path.exists(root) and not os.listdir(root):
+ os.rmdir(root)
+ except (OSError, FileNotFoundError) as e:
+ self.logger.warning(f"Could not remove empty directory {root}: {e}")
def __post_init__(self):
# ... other initialization code ...
atexit.register(
self.clean_old_logs,
self.workflow.executor_settings.logdir,
self.workflow.executor_settings.delete_logfiles_older_than,
)Likely invalid or redundant comment.
35-46:
Make logdir path construction more robust
The current implementation directly accesses os.environ['USER'] which can raise KeyError if USER is not set. Use os.path.expanduser('~') for a more robust and cross-platform solution.
- logdir: Optional[str] = field(
- default=f"/home/{os.environ['USER']}/.snakemake/slurm_logs",
+ logdir: Optional[str] = field(
+ default=os.path.join(os.path.expanduser('~'), ".snakemake", "slurm_logs"),Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…cutor-plugin-slurm into feat/logfeatures
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
51-51: Fix typo in documentationThe word "sucessful" is misspelled in the help text.
Apply this diff to fix the typo:
- Per default SLURM log files will be deleted upon sucessful + Per default SLURM log files will be deleted upon successful
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(7 hunks)
🔇 Additional comments (4)
snakemake_executor_plugin_slurm/__init__.py (4)
459-470: LGTM: Log cleanup implementation is robust
The implementation includes proper error handling, path existence checks, and appropriate logging.
199-204: 🛠️ Refactor suggestion
Use os.path.join for path construction
The current implementation uses string concatenation which is error-prone and not cross-platform friendly.
Apply this diff to improve path construction:
- slurm_logfile = (
- self.workflow.executor_settings.logdir
- + os.path.sep
- + f"{group_or_rule}/{wildcard_str}/%j.log"
- )
+ slurm_logfile = os.path.join(
+ self.workflow.executor_settings.logdir,
+ group_or_rule,
+ wildcard_str,
+ "%j.log"
+ )Likely invalid or redundant comment.
35-46:
Use os.path.expanduser('~') for home directory path
The current implementation has potential issues:
- Using
os.environ['USER']can raise KeyError if USER is not set - The help text mentions '~' but the code uses an absolute path
Apply this diff to fix the path construction:
logdir: Optional[str] = field(
- default=f"/home/{os.environ['USER']}/.snakemake/slurm_logs",
+ default=os.path.join(os.path.expanduser('~'), ".snakemake", "slurm_logs"),Likely invalid or redundant comment.
135-163: 🛠️ Refactor suggestion
Improve clean_old_logs implementation
Several issues with the current implementation:
- Function defined inside post_init reduces maintainability and testability
- Missing error handling for directory removal
- Missing type hints
- Potential race condition in directory removal logic
Apply this diff to improve the implementation:
+ def clean_old_logs(self, logdir: str, age_cutoff: int) -> None:
+ """Delete files older than specified age from the SLURM log directory.
+
+ Args:
+ logdir: Path to the log directory
+ age_cutoff: Number of days after which files should be deleted
+ """
+ if age_cutoff <= 0:
+ return
+ cutoff_secs = age_cutoff * 86400
+ current_time = time.time()
+ self.logger.info(f"Cleaning up log files older than {age_cutoff} day(s)")
+
+ for root, _, files in os.walk(logdir, topdown=False):
+ for fname in files:
+ file_path = os.path.join(root, fname)
+ try:
+ file_age = current_time - os.stat(file_path).st_mtime
+ if file_age > cutoff_secs:
+ os.remove(file_path)
+ except (OSError, FileNotFoundError) as e:
+ self.logger.warning(f"Could not delete file {file_path}: {e}")
+ # Remove empty directories
+ try:
+ if os.path.exists(root) and not os.listdir(root):
+ os.rmdir(root)
+ except (OSError, FileNotFoundError) as e:
+ self.logger.warning(f"Could not remove empty directory {root}: {e}")
def __post_init__(self):
# ... other initialization code ...
atexit.register(
self.clean_old_logs,
self.workflow.executor_settings.logdir,
self.workflow.executor_settings.delete_logfiles_older_than,
)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
35-72: LGTM! Clear and well-documented settings.The new settings for log management are well-structured with appropriate defaults and helpful documentation. The path construction for logdir is safe and cross-platform compatible.
Consider fixing the typo in the help text:
- Per default SLURM log files will be deleted upon sucessful + Per default SLURM log files will be deleted upon successful
177-179: Remove commented-out code.The commented-out
delete_old_logsmethod appears to be obsolete now that the new log cleanup functionality has been implemented.Apply this diff to remove the commented-out code:
- # def delete_old_logs(self): - # self.workflow.executor_settings.delete_logfiles_older_than -
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(7 hunks)
🔇 Additional comments (2)
snakemake_executor_plugin_slurm/__init__.py (2)
460-471: LGTM! Well-implemented log cleanup for successful jobs.
The implementation includes:
- Proper conditional check for keep_successful_logs setting
- Comprehensive error handling for file operations
- Appropriate debug and warning logging
135-163: 🛠️ Refactor suggestion
Refactor log cleanup implementation for better maintainability.
While the log cleanup functionality works correctly, there are some improvements that could make it more maintainable:
- Move the function definition outside post_init to make it a proper class method
- Add error handling for directory removal operations
- Add type hints for better code clarity
Here's the suggested implementation:
- def __post_init__(self):
- # ... other init code ...
- def clean_old_logs(logdir, age_cutoff):
- """
- Function to delete files older than 'age_cutoff'
- in the SLURM 'logdir'
- """
+ def clean_old_logs(self, logdir: str, age_cutoff: int) -> None:
+ """Delete files older than specified age from the SLURM log directory.
+
+ Args:
+ logdir: Path to the log directory
+ age_cutoff: Number of days after which files should be deleted
+ """
+ if age_cutoff <= 0:
+ return
+ cutoff_secs = age_cutoff * 86400
+ current_time = time.time()
+ self.logger.info(f"Cleaning up log files older than {age_cutoff} day(s)")
+
+ for root, _, files in os.walk(logdir, topdown=False):
+ for fname in files:
+ file_path = os.path.join(root, fname)
+ try:
+ file_age = current_time - os.stat(file_path).st_mtime
+ if file_age > cutoff_secs:
+ os.remove(file_path)
+ except (OSError, FileNotFoundError) as e:
+ self.logger.warning(f"Could not delete file {file_path}: {e}")
+ # remove empty rule top dir, if empty
+ try:
+ if len(os.listdir(root)) == 0:
+ os.rmdir(root)
+ except (OSError, FileNotFoundError) as e:
+ self.logger.warning(f"Could not remove empty directory {root}: {e}")
def __post_init__(self):
+ # ... other init code ...
atexit.register(
self.clean_old_logs,
self.workflow.executor_settings.logdir,
self.workflow.executor_settings.delete_logfiles_older_than,
)Likely invalid or redundant comment.
The action is fixed again. It was using an old slurm plugin that is deprecated in the new slurm release that is on the current ubuntu. |
Co-authored-by: Johannes Köster <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/__init__.py(8 hunks)
🔇 Additional comments (3)
snakemake_executor_plugin_slurm/__init__.py (3)
6-6: LGTM: Required imports added.The new imports
atexitandpathlib.Pathare necessary for the log cleanup functionality.Also applies to: 10-10
194-201: LGTM: Improved log file path construction.The use of
os.path.joinfor constructing the log file path is more robust and cross-platform friendly.
455-467: LGTM: Proper error handling for log file deletion.The implementation includes appropriate error handling and logging for log file deletion operations.
|
@johanneskoester Happy New Year! IMO, it's just the issue with the default path, which might be subject to change. Please take a look and weight the argument. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
docs/further.md (4)
261-261: Clarify log retention configuration optionsThe documentation should specify how to disable the log deletion feature and explain the format for the retention period value.
-This executor will remove SLURM logs of sucessful jobs immediately when they are finished. You can change this behaviour with the flag `--slurm-keep-successful-logs`. A log file for a failed job will be preserved per default for 10 days. You may change this value using the `--slurm-delete-logfiles-older-than` flag. +This executor will remove SLURM logs of successful jobs immediately when they are finished. You can change this behavior with the flag `--slurm-keep-successful-logs`. A log file for a failed job will be preserved by default for 10 days. You may change this value using the `--slurm-delete-logfiles-older-than` flag (e.g., `--slurm-delete-logfiles-older-than 30` for 30 days, or `--slurm-delete-logfiles-older-than 0` to disable auto-deletion).
265-267: Enhance the YAML configuration exampleThe example could be more comprehensive by showing all log-related settings.
```YAML -slurm-logdir: "/home/<username>/.snakemake/slurm_logs" +# Log management settings +slurm-logdir: "/home/<username>/.snakemake/slurm_logs" +slurm-keep-successful-logs: false # Set to true to keep logs of successful jobs +slurm-delete-logfiles-older-than: 10 # Number of days to keep failed job logs--- `263-263`: **Fix grammar and spelling** There are several issues in this line: 1. Missing "to" before "avoid" 2. American English consistency ```diff -The default location of Snakemake log files are relative to the directory where the workflow is started or relative to the directory indicated with `--directory`. SLURM logs, produced by Snakemake, can be redirected using `--slurm-logdir`. If you want avoid that log files accumulate in different directories, you can store them in your home directory. Best put the parameter in your profile then, e.g.: +The default location of Snakemake log files is relative to the directory where the workflow is started or relative to the directory indicated with `--directory`. SLURM logs, produced by Snakemake, can be redirected using `--slurm-logdir`. If you want to avoid log files accumulating in different directories, you can store them in your home directory. It's best to put this parameter in your profile, e.g.:🧰 Tools
🪛 LanguageTool
[grammar] ~263-~263: Did you mean “to avoid”?
Context: ...ted using--slurm-logdir. If you want avoid that log files accumulate in different ...(MISSING_TO_BEFORE_A_VERB)
257-258: Improve section heading consistencyThe heading style should match other sections in the document. Also, consider moving this section after the "Retries" section since log management is typically a more advanced topic.
-### Log Files - Getting Information on Failures +### Log Filessnakemake_executor_plugin_slurm/utils.py (2)
21-27: Fix documentation issuesThe function documentation has a typo and is missing return value documentation.
Apply these improvements:
""" Function to delete all empty directories in a given path. This is needed to clean up the working directory after - a job has sucessfully finished. This function is needed because + a job has successfully finished. This function is needed because the shutil.rmtree() function does not delete empty directories. + + Args: + path (Path): The directory path to clean up + + Returns: + list: List of directories that were processed + + Raises: + OSError: If a directory cannot be removed """
29-37: Consider improving the recursive directory traversalThe current implementation of
get_dirsmight not handle deep directory structures efficiently due to Python's recursion limit.Consider using an iterative approach:
- def get_dirs(path: Path, result=None): - if result is None: - result = [] - for p in path.iterdir(): - if p.is_dir(): - result.append(p) - if any(p.iterdir()): - get_dirs(p) - return result + def get_dirs(path: Path): + result = [] + stack = [path] + while stack: + current = stack.pop() + for p in current.iterdir(): + if p.is_dir(): + result.append(p) + if any(p.iterdir()): + stack.append(p) + return result🧰 Tools
🪛 Ruff (0.8.2)
30-30: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/further.md(2 hunks)snakemake_executor_plugin_slurm/__init__.py(7 hunks)snakemake_executor_plugin_slurm/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snakemake_executor_plugin_slurm/init.py
🧰 Additional context used
🪛 Ruff (0.8.2)
snakemake_executor_plugin_slurm/utils.py
30-30: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🪛 LanguageTool
docs/further.md
[grammar] ~263-~263: Did you mean “to avoid”?
Context: ...ted using --slurm-logdir. If you want avoid that log files accumulate in different ...
(MISSING_TO_BEFORE_A_VERB)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/utils.py (1)
4-4: LGTM: Clean import additionThe
Pathimport frompathlibis appropriately placed and properly utilized in the new function.
not convinced, that this is better, but replacing [] for None as the default arg in `get_dirs` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
snakemake_executor_plugin_slurm/utils.py (2)
21-27: Fix typo and enhance docstring.
- Fix the typo: "sucessfully" → "successfully"
- Add information about the recursive nature of the function
""" Function to delete all empty directories in a given path. This is needed to clean up the working directory after - a job has sucessfully finished. This function is needed because + a job has successfully finished. This function is needed because the shutil.rmtree() function does not delete empty directories. + + The function uses a bottom-up approach to recursively process + and delete nested empty directories. """
28-42: LGTM! Consider adding warning logs.The implementation correctly handles recursive deletion of empty directories using a bottom-up approach. The error handling is robust, re-raising exceptions with proper context.
Consider adding warning logs before re-raising the exception to help with debugging:
import os from pathlib import Path +import logging + +logger = logging.getLogger(__name__) def delete_empty_dirs(path: Path) -> None: # ... existing code ... try: if not any(path.iterdir()): path.rmdir() except (OSError, FileNotFoundError) as e: + logger.warning(f"Failed to remove empty directory {path}: {e}") raise OSError(f"Failed to remove empty directory {path}: {e}") from e
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/utils.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: formatting
- GitHub Check: testing
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/utils.py (1)
4-4: LGTM!The import statement and spacing between functions follow Python's PEP 8 style guidelines.
Also applies to: 18-19
|
@johanneskoester all changes are in - I added docs, too. To me, everything is now good enough. Some refactoring is due, soon. |
🤖 I have created a release *beep* *boop* --- ## [0.12.0](v0.11.2...v0.12.0) (2025-01-08) ### Features * custom log file behaviour ([#159](#159)) ([cc3d21b](cc3d21b)) * experimenting with automated release post to Mastodon ([#166](#166)) ([c06325d](c06325d)) ### Documentation * fix headings in further.md ([#168](#168)) ([531ebc6](531ebc6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR should allow to
The idea behind this PR is to
It addresses issues #94 and #123.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements