feat: measuring compute efficiency per job #221
Conversation
WalkthroughThis update introduces job efficiency reporting to the SLURM executor plugin for Snakemake. It replaces the log cleanup mechanism with a shutdown hook, adds new configuration options for efficiency reports, implements the report generation logic, updates documentation, adjusts dependencies, and expands the test suite to validate the new feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor
participant EfficiencyReport
participant SLURM (sacct)
participant FileSystem
User->>Executor: Run workflow with efficiency_report enabled
Executor->>Executor: On shutdown, call clean_old_logs
Executor->>EfficiencyReport: create_efficiency_report(threshold, uuid, path, logger)
EfficiencyReport->>SLURM (sacct): Query job data (by workflow UUID)
SLURM (sacct)-->>EfficiencyReport: Return job accounting data
EfficiencyReport->>FileSystem: Write efficiency_report_<uuid>.csv
EfficiencyReport->>Executor: Log report location and warnings if needed
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
…-executor-plugin-slurm into docs/review-new-docs
…TODOs for clarification
…on for regular jobs, as this is covered in the where to do configuration section and the main snakemake docs
…seful for a user -- just explain special cases MPI and GPU below
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…utor-plugin-slurm into feat/eff-report
![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
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/efficiency_report.py (1)
123-127: Handle division by zero in CPU efficiency calculation.The CPU efficiency calculation can produce infinite values when
Elapsed_secorNCPUSare zero, which are not handled byfillna(0). This can lead toinfvalues in the final report.- df["CPU Efficiency (%)"] = ( - df["TotalCPU_sec"] / (df["Elapsed_sec"] * df["NCPUS"]) - ) * 100 - df["CPU Efficiency (%)"] = df["CPU Efficiency (%)"].fillna(0).round(2) + # Avoid division by zero by using clip to ensure minimum values of 1 + df["CPU Efficiency (%)"] = ( + df["TotalCPU_sec"] / (df["Elapsed_sec"].clip(lower=1) * df["NCPUS"].clip(lower=1)) + ) * 100 + # Handle any remaining infinite values + df["CPU Efficiency (%)"] = df["CPU Efficiency (%)"].replace([float('inf'), float('-inf')], 0) + df["CPU Efficiency (%)"] = df["CPU Efficiency (%)"].fillna(0).round(2)
🧹 Nitpick comments (4)
snakemake_executor_plugin_slurm/efficiency_report.py (4)
7-8: Remove temporary debug import.The
osimport appears to be temporary for debugging purposes and should be removed before production deployment.-import os # only temporarily needed for printf debugging -
16-21: Simplify conditional structure per static analysis recommendation.The
elifstatements afterreturnare unnecessary and can be simplified toifstatements.if len(parts) == 3: # H:M:S return int(parts[0]) * 3600 + int(parts[1]) * 60 + float(parts[2]) - elif len(parts) == 2: # M:S + if len(parts) == 2: # M:S return int(parts[0]) * 60 + float(parts[1]) - elif len(parts) == 1: # S + if len(parts) == 1: # S return float(parts[0])
172-175: Simplify file path construction with ternary operator.Static analysis suggests using a ternary operator for cleaner code.
- if e_report_path: - logfile = Path(e_report_path) / logfile - else: - logfile = p.cwd() / logfile + logfile = Path(e_report_path) / logfile if e_report_path else p.cwd() / logfile
183-183: Remove debug logging statement.The debug statement using the temporary
osimport should be removed along with the import.- logger.debug(f"Current directory contents in '{p.cwd()}': {os.listdir(p.cwd())}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/efficiency_report.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
snakemake_executor_plugin_slurm/efficiency_report.py (2)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: snakemake_executor_plugin_slurm/efficiency_report.py:34-49
Timestamp: 2025-05-26T12:22:07.005Z
Learning: In the parse_reqmem function in snakemake_executor_plugin_slurm/efficiency_report.py, the conditional check for per_unit == "/node" is necessary for proper conversion of memory values in multi-node jobs, not redundant as it might initially appear.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
🪛 Ruff (0.11.9)
snakemake_executor_plugin_slurm/efficiency_report.py
172-175: Use ternary operator logfile = Path(e_report_path) / logfile if e_report_path else p.cwd() / logfile instead of if-else-block
Replace if-else-block with logfile = Path(e_report_path) / logfile if e_report_path else p.cwd() / logfile
(SIM108)
🪛 Pylint (3.3.7)
snakemake_executor_plugin_slurm/efficiency_report.py
[refactor] 16-21: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 57-57: Either all return statements in a function should return an expression, or none of them should.
(R1710)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (1)
snakemake_executor_plugin_slurm/efficiency_report.py (1)
57-182: LGTM! Well-structured efficiency reporting implementation.The overall implementation is robust and handles the complex task of parsing SLURM accounting data effectively. The function properly handles edge cases like empty data, missing comments, and various memory format specifications. The integration of lessons learned from previous reviews (NaN handling, empty string checks, decimal parsing) makes this a solid implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
snakemake_executor_plugin_slurm/efficiency_report.py (6)
7-8: Remove temporary debugging imports and comment.The comment indicates
osandnumpyare temporarily needed for debugging, but both are actually used in the production code (os.listdiron line 185,np.infon line 128).-import os # only temporarily needed for printf debugging -import numpy as np +import os +import numpy as np
17-22: Simplify control flow by removing unnecessary elif statements.The
elifstatements afterreturnare unnecessary and can be simplified for better readability.- if len(parts) == 3: # H:M:S + if len(parts) == 3: # H:M:S return int(parts[0]) * 3600 + int(parts[1]) * 60 + float(parts[2]) - elif len(parts) == 2: # M:S + if len(parts) == 2: # M:S return int(parts[0]) * 60 + float(parts[1]) - elif len(parts) == 1: # S + if len(parts) == 1: # S return float(parts[0]) return 0
58-80: Make return behavior consistent throughout the function.The function returns
Nonein error cases but implicitly returnsNoneat the end of successful execution. Consider returning a consistent value or explicit success indicator.def create_efficiency_report(e_threshold, run_uuid, e_report_path, logger): """ Fetch sacct job data for a Snakemake workflow and compute efficiency metrics. + + Returns: + str: Path to the generated report file, or None if failed """And at the end of the function:
# write out the efficiency report at normal verbosity in any case logger.info(f"Efficiency report for workflow {run_uuid} saved to {logfile}.") # state directory contents for debugging purposes logger.debug(f"Current directory contents in '{p.cwd()}': {os.listdir(p.cwd())}") + return str(logfile)
168-177: Simplify path construction using ternary operator.The if-else block for constructing the logfile path can be simplified using a ternary operator for better readability.
- # we construct a path object to allow for a customi + # we construct a path object to allow for a custom # logdir, if specified p = Path() # Save the report to a CSV file logfile = f"efficiency_report_{run_uuid}.csv" - if e_report_path: - logfile = Path(e_report_path) / logfile - else: - logfile = p.cwd() / logfile + logfile = Path(e_report_path) / logfile if e_report_path else p.cwd() / logfile
180-180: Consider adding index=False to CSV output for cleaner reports.The CSV file includes pandas DataFrame index by default, which may not be useful for end users reading the efficiency reports.
- df.to_csv(logfile) + df.to_csv(logfile, index=False)
185-185: Replace debug directory listing with more informative logging.The current debug output lists all files in the current directory, which may be verbose and not directly relevant. Consider logging only efficiency report files or making it more targeted.
- # state directory contents for debugging purposes - logger.debug(f"Current directory contents in '{p.cwd()}': {os.listdir(p.cwd())}") + # log efficiency report files for debugging purposes + efficiency_files = list(p.cwd().glob("efficiency_report_*.csv")) + logger.debug(f"Efficiency report files in '{p.cwd()}': {efficiency_files}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snakemake_executor_plugin_slurm/efficiency_report.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
snakemake_executor_plugin_slurm/efficiency_report.py (2)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: snakemake_executor_plugin_slurm/efficiency_report.py:34-49
Timestamp: 2025-05-26T12:22:07.005Z
Learning: In the parse_reqmem function in snakemake_executor_plugin_slurm/efficiency_report.py, the conditional check for per_unit == "/node" is necessary for proper conversion of memory values in multi-node jobs, not redundant as it might initially appear.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: snakemake_executor_plugin_slurm/efficiency_report.py:0-0
Timestamp: 2025-06-16T08:54:07.957Z
Learning: In the context of SLURM executor plugin development, user cmeesters (experienced with SLURM) noted they are not aware of SLURM typically reporting memory values with decimals (like 1.5G), but accepted defensive programming to handle such cases anyway.
🪛 Ruff (0.11.9)
snakemake_executor_plugin_slurm/efficiency_report.py
174-177: Use ternary operator logfile = Path(e_report_path) / logfile if e_report_path else p.cwd() / logfile instead of if-else-block
Replace if-else-block with logfile = Path(e_report_path) / logfile if e_report_path else p.cwd() / logfile
(SIM108)
🪛 Pylint (3.3.7)
snakemake_executor_plugin_slurm/efficiency_report.py
[refactor] 17-22: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 58-58: Either all return statements in a function should return an expression, or none of them should.
(R1710)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/tests.py
70-70: Undefined name expected_path
(F821)
🪛 Flake8 (7.2.0)
tests/tests.py
[error] 70-70: undefined name 'expected_path'
(F821)
🪛 Pylint (3.3.7)
tests/tests.py
[error] 70-70: Undefined variable 'expected_path'
(E0602)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (3)
tests/tests.py (3)
1-4: Imports look good.The import statements are well-organized and include all necessary modules for the efficiency report testing functionality.
24-28: Good adjustment to timing settings.Increasing the initial delay before status checks from 1 to 2 seconds is a reasonable change that should help with test reliability.
30-45: Well-structured test class for efficiency reporting.The test class is properly configured with appropriate settings for testing the efficiency report feature. The use of
Path.cwd()for the report path is a good approach.
🤖 I have created a release *beep* *boop* --- ## [1.5.0](v1.4.0...v1.5.0) (2025-07-04) ### Features * measuring compute efficiency per job ([#221](#221)) ([3cef6b7](3cef6b7)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated the changelog to include version 1.5.0 with details about the new compute efficiency measurement feature. * **Chores** * Bumped the package version to 1.5.0. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
The aim of this PR is:
seffNote:
seffreports so-called "Memory Efficiency". What is meant is: "Memory Usage", because an application which reserve a compute node to compute and hardly uses RAM to do so, will have an apparently low "Memory Efficiency". It needs to reserve the memory of that node, might be highly efficient, but will not have used memory.The resulting code of this PR will hence NOT report warnings about memory usage.
This PR is related to issue #147
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores