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>
![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 (5)
tests/tests.py (2)
44-87: Critical: Test still missing SLURM environment mocking.Based on the extensive discussion in previous reviews, this test will fail in CI environments without SLURM because the
sacctcommand won't be available. The efficiency report generation depends on SLURM'ssacctcommand, which needs to be mocked for the test to work reliably.Additionally, there are several other issues:
- Line 63: Another instance of
Path.pwd()that should bePath.cwd()- No error handling for missing directories
- The test logic seems to search in a different location than configured
As discussed extensively in previous reviews, you need to implement SLURM mocking:
def test_simple_workflow(self, tmp_path): - # for an unkown reason, the efficiency report is not created - # reliably in `tmp_path`, so we use a fixed path - # to ensure the test is reproducible - - # a worklfow aborted: - # error message: - # OSError: Cannot save file into a non-existent directory: - # '/tmp/efficiency_report_test' - # runpath = Path("/tmp/efficiency_report_test") - # runpath.mkdir(parents=True, exist_ok=True) - self.run_workflow("simple", tmp_path) - - # The efficiency report is created in the - # current working directory - pattern = re.compile(r"efficiency_report_[\w-]+\.csv") - report_found = False - - report_path = None - expected_path = Path.pwd() / "efficiency_report_test" - - # Check if the efficiency report file exists - based on the regex pattern - for fname in os.listdir(expected_path): - if pattern.match(fname): - report_found = True - report_path = os.path.join(expected_path, fname) - # Verify it's not empty - assert ( - os.stat(report_path).st_size > 0 - ), f"Efficiency report {report_path} is empty" - break - assert report_found, "Efficiency report file not found" + """Test efficiency report generation with proper SLURM mocking.""" + import subprocess + from unittest.mock import patch, MagicMock + + # Mock sacct output with the exact format expected: + # JobID|JobName|Comment|Elapsed|TotalCPU|NNodes|NCPUS|MaxRSS|ReqMem + mock_sacct_output = "1234|test_job|rule_test|00:01:30|00:00:45|1|2|1024K|2G\n1235|another_job|rule_another|00:02:00|00:01:00|1|4|2048K|4G" + + # Create a mock result object that mimics subprocess.CompletedProcess + mock_result = MagicMock() + mock_result.stdout = mock_sacct_output + mock_result.returncode = 0 + + # Mock subprocess.run to return our fake sacct data + def mock_subprocess_run(cmd, *args, **kwargs): + # Check if this is a sacct command + if isinstance(cmd, list) and len(cmd) > 0 and 'sacct' in cmd[0]: + return mock_result + else: + # For other commands, call the real subprocess.run + return subprocess.run(cmd, *args, **kwargs) + + # Patch subprocess.run in the correct module + with patch('snakemake_executor_plugin_slurm.subprocess.run', side_effect=mock_subprocess_run): + self.run_workflow("simple", tmp_path) + + # The efficiency report is created in the configured path + pattern = re.compile(r"efficiency_report_[\w-]+\.csv") + report_found = False + + # Check the configured efficiency_report_path + expected_path = Path.cwd() / "efficiency_report_test" + if expected_path.exists(): + for fname in os.listdir(expected_path): + if pattern.match(fname): + report_found = True + report_path = expected_path / fname + # Verify it's not empty + assert report_path.stat().st_size > 0, f"Efficiency report {report_path} is empty" + break + + assert report_found, "Efficiency report file not found"This solution:
- ✅ Mocks the SLURM
sacctcommand as discussed in previous reviews- ✅ Works in any environment (with or without SLURM)
- ✅ Uses proper Path objects instead of string concatenation
- ✅ Adds directory existence checking
- ✅ Eliminates the hardcoded path issues
40-40: Fix the invalid method call -Path.pwd()does not exist.
Path.pwd()is not a valid method. UsePath.cwd()instead to get the current working directory.- efficiency_report_path=Path.pwd() / "efficiency_report_test", + efficiency_report_path=Path.cwd() / "efficiency_report_test",snakemake_executor_plugin_slurm/efficiency_report.py (3)
95-95: Improve empty comment detection.The current check only catches
NaNvalues but misses empty strings that SLURM might return.- if df["Comment"].isnull().all(): + if df["Comment"].replace("", pd.NA).isna().all():
118-121: Handle division by zero in CPU efficiency calculation.When
Elapsed_secorNCPUSare zero, the calculation produces infinity values that survivefillna(0)and appear in the output.# Compute CPU efficiency + # Avoid division by zero by clipping denominators to minimum of 1 df["CPU Efficiency (%)"] = ( - df["TotalCPU_sec"] / (df["Elapsed_sec"] * df["NCPUS"]) + df["TotalCPU_sec"] / (df["Elapsed_sec"].clip(lower=1) * df["NCPUS"].clip(lower=1)) ) * 100 - df["CPU Efficiency (%)"] = df["CPU Efficiency (%)"].fillna(0).round(2) + # Replace any remaining inf/-inf values with 0 + df["CPU Efficiency (%)"] = df["CPU Efficiency (%)"].replace([float('inf'), float('-inf')], 0).fillna(0).round(2)
67-74: Handle empty sacct output gracefully.If
sacctreturns no data (e.g., wrong UUID or accounting data purged), the DataFrame creation will fail. Add a check for empty output.try: result = subprocess.run( shlex.split(cmd), capture_output=True, text=True, check=True ) - lines = result.stdout.strip().split("\n") + raw_output = result.stdout.strip() + if not raw_output: + logger.warning(f"No accounting data found for workflow {run_uuid}.") + return None + lines = raw_output.split("\n")
🧹 Nitpick comments (4)
snakemake_executor_plugin_slurm/efficiency_report.py (4)
7-7: Remove temporary debugging code before release.The
osimport and its usage on line 176 are marked as temporary for debugging. Consider removing this before the final release to keep the code clean.-import os # only temporarily needed for printf debugging- # state directory contents for debugging purposes - logger.debug(f"Current directory contents in '{p.cwd()}': {os.listdir(p.cwd())}")
16-21: Simplify conditional structure.Since each branch returns a value, the
elifstatements can be simplified toifstatements for better readability.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])
167-171: Simplify path assignment logic.The if-else block can be simplified using a ternary operator for better readability.
# 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
57-177: Ensure consistent return behavior.The function sometimes returns
Noneon error but doesn't explicitly return anything on success. For consistency, add an explicit return statement.# 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 logfile
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/efficiency_report.py(1 hunks)tests/tests.py(2 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
168-171: 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)
🪛 Flake8 (7.2.0)
snakemake_executor_plugin_slurm/efficiency_report.py
[error] 7-7: at least two spaces before inline comment
(E261)
🪛 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] 58-58: Either all return statements in a function should return an expression, or none of them should.
(R1710)
tests/tests.py
[error] 40-40: Class 'Path' has no 'pwd' member; maybe 'cwd'?
(E1101)
[error] 63-63: Class 'Path' has no 'pwd' member; maybe 'cwd'?
(E1101)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (3)
tests/tests.py (2)
1-4: LGTM: Clean import additions.The new imports (
os,re,Path) are appropriately added to support the efficiency report testing functionality.
24-28: Minor timing adjustment looks appropriate.The increase from 1 to 2 seconds for
init_seconds_before_status_checksshould help with test stability.snakemake_executor_plugin_slurm/efficiency_report.py (1)
25-54: LGTM! Memory parsing functions are well-implemented.The
parse_maxrssandparse_reqmemfunctions correctly handle:
- Fractional values (e.g., "1.5G")
- Various unit suffixes (K, M, G)
- SLURM-specific per-node and per-CPU memory specifications
- NaN value handling to prevent error propagation
The implementation aligns with SLURM documentation and handles edge cases defensively.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…utor-plugin-slurm into feat/eff-report
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 -->
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