feat: measuring compute efficiency per job #221
Conversation
WalkthroughThis update introduces a job efficiency reporting feature for SLURM workflows. It adds an efficiency report option, supporting logic, and documentation. New utility functions parse SLURM resource strings. The workflow is tested with a new test class, and pandas is added as a dependency. The GitHub workflow action for Mastodon releases is also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor
participant SLURM
participant EfficiencyReport
User->>Executor: Run workflow with efficiency report enabled
Executor->>SLURM: Submit jobs
SLURM-->>Executor: Return job info and status
Executor->>Executor: On shutdown, clean logs
Executor->>EfficiencyReport: Generate efficiency report (if enabled)
EfficiencyReport->>SLURM: Query job accounting data (sacct)
SLURM-->>EfficiencyReport: Return job resource usage
EfficiencyReport->>Executor: Save CSV report
Executor-->>User: Report saved, workflow complete
Possibly related PRs
Suggested reviewers
Poem
✨ 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/tests.py (1)
27-59:⚠️ Potential issueCritical: Test will fail without SLURM environment - implement mocking.
Based on the past review comments, this test will continue to fail in environments without SLURM because the efficiency report creation depends on the
sacctcommand. The current implementation lacks the mocking solution that was previously discussed.The test needs proper mocking to simulate the
sacctcommand output. Here's the working solution based on previous discussions:def test_simple_workflow(self, tmp_path): + 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)Without this mocking, the test will continue to fail because no efficiency report file gets created when
sacctis unavailable.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 27-27: Missing class docstring
(C0115)
[convention] 30-30: Missing function or method docstring
(C0116)
[convention] 33-33: Missing function or method docstring
(C0116)
[convention] 38-38: Missing function or method docstring
(C0116)
🧹 Nitpick comments (4)
tests/tests.py (2)
27-28: Add class docstring for better documentation.Consider adding a docstring to explain the purpose of this test class.
class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for verifying SLURM efficiency report generation functionality.""" __test__ = True🧰 Tools
🪛 Pylint (3.3.7)
[convention] 27-27: Missing class docstring
(C0115)
38-38: Add method docstring for clarity.Consider adding a docstring to explain what this test verifies.
def test_simple_workflow(self, tmp_path): + """Test that efficiency report file is generated after workflow completion.""" self.run_workflow("simple", tmp_path)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 38-38: Missing function or method docstring
(C0116)
snakemake_executor_plugin_slurm/__init__.py (2)
879-889: Simplify path construction logic.The path construction can be simplified by removing the unnecessary
Path()instantiation and improving readability.- # we construct a path object to allow for a customi - # logdir, if specified - p = Path() - # Save the report to a CSV file logfile = f"efficiency_report_{self.run_uuid}.csv" if self.workflow.executor_settings.logdir: logfile = Path(self.workflow.executor_settings.logdir) / logfile else: - logfile = p.cwd() / logfile + logfile = Path.cwd() / logfile🧰 Tools
🪛 Pylint (3.3.7)
[convention] 882-882: Trailing whitespace
(C0303)
890-897: Remove debugging logs and add consistent return.The debugging logs about directory contents seem unnecessary for production code, and the method should have consistent return behavior.
df.to_csv(logfile) - content = os.listdir() - self.logger.info(f"Current dir: {p.cwd()}") - self.logger.info(f"content of current dir: {content}") # write out the efficiency report at normal verbosity in any case self.logger.info( f"Efficiency report for workflow {self.run_uuid} saved to {logfile}." ) + return str(logfile)🧰 Tools
🪛 Ruff (0.11.9)
891-891: Undefined name
cwd(F821)
🪛 Pylint (3.3.7)
[error] 891-891: Undefined variable 'cwd'
(E0602)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(6 hunks)tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/utils.py (1)
delete_empty_dirs(26-48)snakemake_executor_plugin_slurm/efficiency_report.py (3)
time_to_seconds(5-17)parse_maxrss(20-30)parse_reqmem(33-50)
🪛 Pylint (3.3.7)
snakemake_executor_plugin_slurm/__init__.py
[error] 19-19: Unable to import 'pandas'
(E0401)
[convention] 882-882: Trailing whitespace
(C0303)
[error] 891-891: Undefined variable 'cwd'
(E0602)
[refactor] 774-774: Either all return statements in a function should return an expression, or none of them should.
(R1710)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 27-27: Missing class docstring
(C0115)
[convention] 30-30: Missing function or method docstring
(C0116)
[convention] 33-33: Missing function or method docstring
(C0116)
[convention] 38-38: Missing function or method docstring
(C0116)
🪛 Ruff (0.11.9)
snakemake_executor_plugin_slurm/__init__.py
891-891: Undefined name cwd
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (8)
tests/tests.py (2)
1-2: LGTM! Imports are appropriate for the new functionality.The
reandPathimports are correctly used in the newTestEfficiencyReportclass for pattern matching and file operations.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
22-25: LGTM! Timing parameter adjustments improve test reliability.The additional timing parameters and increased initial delay should help with test stability by providing more controlled timing for status checks.
snakemake_executor_plugin_slurm/__init__.py (6)
19-19: Import addition looks good.The pandas import is necessary for the DataFrame operations in the new efficiency reporting functionality.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 19-19: Unable to import 'pandas'
(E0401)
32-37: Import organization looks good.The new utility function imports are well-organized and necessary for the efficiency reporting functionality.
116-124: Settings field addition is well-implemented.The efficiency_report field follows the established pattern and provides clear documentation.
169-182: Shutdown method override is well-structured.The implementation correctly calls the parent shutdown method first, then performs cleanup and optional efficiency reporting. Good separation of concerns.
193-209: Logging improvements are appropriate.The change from warning to error level for file deletion failures is reasonable, and the error messages provide better context.
766-772: Error message formatting improvement is good.The multi-line error message is more readable and provides clearer guidance to users.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/tests.py (1)
40-62:⚠️ Potential issueCritical issues: Fix file extension and implement SLURM mocking.
Several problems need to be addressed:
File extension inconsistency: The pattern searches for
.csvfiles, but efficiency reports are generated as.logfiles based on the executor implementation.Missing SLURM environment mocking: This test will fail in CI environments without SLURM. Based on previous discussions, you need to mock
subprocess.runto simulatesacctcommand output.Missing documentation: Add a docstring to explain the test purpose.
Apply this fix for the file extension:
- pattern = re.compile(r"efficiency_report_[\w-]+\.csv") + pattern = re.compile(r"efficiency_report_[\w-]+\.log")- for filepath in search_dir.glob("efficiency_report_*.csv"): + for filepath in search_dir.glob("efficiency_report_*.log"):Add docstring and implement the mocking solution discussed in previous comments:
+ def test_simple_workflow(self, tmp_path): + """Test that efficiency reports are generated when enabled.""" + 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 + mock_result = MagicMock() + mock_result.stdout = mock_sacct_output + mock_result.returncode = 0 + + # Mock subprocess.run to return fake sacct data + def mock_subprocess_run(cmd, *args, **kwargs): + if isinstance(cmd, list) and len(cmd) > 0 and 'sacct' in cmd[0]: + return mock_result + else: + 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)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 40-40: Missing function or method docstring
(C0116)
🧹 Nitpick comments (2)
tests/tests.py (1)
28-39: Add documentation for the new test class.The class structure and configuration are correct, but the class and methods lack docstrings as indicated by static analysis.
+class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for validating SLURM efficiency report generation.""" __test__ = True + def get_executor(self) -> str: + """Return the executor type for efficiency report testing.""" return "slurm" + def get_executor_settings(self) -> Optional[ExecutorSettingsBase]: + """Configure executor settings to enable efficiency report generation.""" return ExecutorSettings( efficiency_report=True, init_seconds_before_status_checks=5, seconds_between_status_checks=5, )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
snakemake_executor_plugin_slurm/__init__.py (1)
774-794: Consider making efficiency threshold configurable.The efficiency threshold is hardcoded at 0.8 (80%). Consider making this configurable through the executor settings to allow users to adjust based on their workflow requirements.
- def create_efficiency_report(self, efficiency_threshold=0.8): + def create_efficiency_report(self): """ Fetch sacct job data for a Snakemake workflow and compute efficiency metrics. """ + # Get configurable efficiency threshold with sensible default + efficiency_threshold = getattr( + self.workflow.executor_settings, + 'efficiency_threshold', + 0.8 + )This would require adding the threshold as a new field in
ExecutorSettings.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 774-774: Either all return statements in a function should return an expression, or none of them should.
(R1710)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(6 hunks)tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (1)
ExecutorSettings(42-124)
🪛 Pylint (3.3.7)
snakemake_executor_plugin_slurm/__init__.py
[error] 19-19: Unable to import 'pandas'
(E0401)
[refactor] 774-774: Either all return statements in a function should return an expression, or none of them should.
(R1710)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 40-40: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (10)
tests/tests.py (2)
1-2: LGTM! Necessary imports for the new test functionality.The
reandpathlib.Pathimports are correctly added to support the efficiency report file pattern matching and path operations in the new test class.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
22-26: LGTM! Reasonable timing adjustments for test stability.The increased
init_seconds_before_status_checksfrom 1 to 2 seconds and the addition ofseconds_between_status_checks=5provide more reliable timing for test environments.snakemake_executor_plugin_slurm/__init__.py (8)
19-19: LGTM: Import additions support the new efficiency reporting feature.The pandas import and efficiency_report utility functions are appropriately added to support the new compute efficiency reporting functionality.
Also applies to: 32-37
🧰 Tools
🪛 Pylint (3.3.7)
[error] 19-19: Unable to import 'pandas'
(E0401)
116-124: LGTM: Efficiency report setting is well-structured.The new
efficiency_reportboolean field follows the established pattern for executor settings with appropriate metadata and sensible defaults.
169-182: LGTM: Well-structured shutdown method override.The shutdown method properly calls the parent implementation before performing custom cleanup operations. The conditional efficiency report generation based on the setting is appropriate.
193-193: LGTM: Improved logging with proper logger usage.The logging improvements using
self.loggerinstead of print statements and more descriptive error messages are excellent enhancements.Also applies to: 202-202, 207-209
766-772: LGTM: Minor formatting improvement to error message.The error message formatting improvement enhances readability without changing functionality.
850-859: LGTM: Zero-division protection implemented correctly.The memory usage calculation properly protects against division by zero as discussed in previous reviews, which is good practice for robustness against potential admin configuration variations.
796-841: LGTM: DataFrame processing follows established patterns.The DataFrame creation and processing logic is well-structured. Based on previous review discussions, column validation isn't needed since the sacct format is controlled, and the CPU efficiency calculation is mathematically sound.
864-880: LGTM: Comprehensive efficiency warning system.The warning system properly handles both cases (with and without comment column) and provides informative messages that help users identify low-efficiency jobs by rule name when available.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/tests.py (1)
41-63:⚠️ Potential issueCritical issue: Test will fail in environments without SLURM - implement mocking solution.
This test has the same issue identified in previous reviews: it will fail in CI/test environments that don't have the SLURM
sacctcommand available. Whensacctfails, thecreate_efficiency_reportmethod returns early without creating any efficiency report file.The previous reviewer provided a complete solution using subprocess mocking. Implement it:
def test_simple_workflow(self, tmp_path): + """Test that efficiency reports are generated correctly.""" + 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 # current working directory pattern = re.compile(r"efficiency_report_[\w-]+\.csv") report_found = False # Check both cwd and the tmp_path for the report file - # the CI seems lost. p = Path() for search_dir in [p.cwd(), tmp_path]: for filepath in search_dir.glob("efficiency_report_*.csv"): if pattern.match(filepath.name): report_found = True # Verify it's not empty assert ( filepath.stat().st_size > 0 ), f"Efficiency report {filepath} is empty" break if report_found: break assert report_found, "Efficiency report file not found"This solution mocks the
sacctcommand to return realistic test data, allowing the test to work in any environment.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 41-41: Missing function or method docstring
(C0116)
🧹 Nitpick comments (1)
tests/tests.py (1)
28-40: Add docstrings for the new test class and methods.The test class structure is correct for testing efficiency reports, but it's missing docstrings as flagged by static analysis.
Add docstrings to improve documentation:
class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for SLURM efficiency report generation functionality.""" __test__ = True def get_executor(self) -> str: + """Return the SLURM executor for efficiency report testing.""" return "slurm" def get_executor_settings(self) -> Optional[ExecutorSettingsBase]: + """Configure executor settings with efficiency reporting enabled.""" return ExecutorSettings( efficiency_report=True, init_seconds_before_status_checks=5, #seconds_between_status_checks=5, )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (1)
ExecutorSettings(42-124)
🪛 Pylint (3.3.7)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 41-41: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (2)
tests/tests.py (2)
1-2: LGTM - Necessary imports for the new test functionality.The
reandPathimports are appropriately added to support regex pattern matching and file system operations in the efficiency report tests.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
22-25: LGTM - Reasonable timing adjustment for test stability.Increasing the initial delay from 1 to 2 seconds before status checks provides more time for job initialization, which should improve test reliability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/tests.py (1)
41-58: Implement SLURM environment mocking to make the test work.Based on the past review comments, this test will fail because there's no SLURM environment in the test setup, so the
sacctcommand fails and no efficiency report file gets created.As suggested in previous reviews, implement subprocess mocking to simulate the SLURM environment:
def test_simple_workflow(self, tmp_path): + import subprocess + from unittest.mock import patch, MagicMock + + # Mock sacct output with the expected format: + # 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 + mock_result = MagicMock() + mock_result.stdout = mock_sacct_output + mock_result.returncode = 0 + + def mock_subprocess_run(cmd, *args, **kwargs): + if isinstance(cmd, list) and len(cmd) > 0 and 'sacct' in cmd[0]: + return mock_result + else: + return subprocess.run(cmd, *args, **kwargs) + + with patch('snakemake_executor_plugin_slurm.subprocess.run', side_effect=mock_subprocess_run): self.run_workflow("simple", tmp_path)Also verify the correct file extension - efficiency reports might be
.logfiles, not.csv:#!/bin/bash # Check what file extension the efficiency report actually uses rg "efficiency_report.*\." snakemake_executor_plugin_slurm/__init__.py🧰 Tools
🪛 Pylint (3.3.7)
[convention] 41-41: Missing function or method docstring
(C0116)
🧹 Nitpick comments (5)
tests/tests.py (1)
28-41: Add docstrings for the new test class and methods.The static analysis correctly identifies missing docstrings. Consider adding documentation to improve code clarity.
class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for SLURM efficiency report generation functionality.""" __test__ = True def get_executor(self) -> str: + """Return the executor type for efficiency report tests.""" return "slurm" def get_executor_settings(self) -> Optional[ExecutorSettingsBase]: + """Configure executor settings with efficiency report enabled.""" return ExecutorSettings( efficiency_report=True, init_seconds_before_status_checks=5, # seconds_between_status_checks=5, ) def test_simple_workflow(self, tmp_path): + """Test that efficiency report file is generated after workflow completion.""" self.run_workflow("simple", tmp_path)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 41-41: Missing function or method docstring
(C0116)
snakemake_executor_plugin_slurm/__init__.py (4)
774-794: Address inconsistent return behavior.The method returns
Noneon error but has no explicit return on success, which creates inconsistency. While the return value isn't currently used, consistent return behavior is good practice.Add an explicit return statement for consistency:
self.logger.info( f"Efficiency report for workflow {self.run_uuid} saved to {logfile}." ) + return logfile🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 774-774: Either all return statements in a function should return an expression, or none of them should.
(R1710)
774-774: Consider making efficiency threshold configurable.The efficiency threshold is hardcoded to 0.8 (80%). This should ideally be configurable by users through the executor settings.
Consider adding a configurable threshold parameter:
- def create_efficiency_report(self, efficiency_threshold=0.8): + def create_efficiency_report(self): """ Fetch sacct job data for a Snakemake workflow and compute efficiency metrics. """ + # Make efficiency threshold configurable, with sensible default + efficiency_threshold = getattr( + self.workflow.executor_settings, + 'efficiency_threshold', + 0.8 + )You would also need to add the corresponding field to
ExecutorSettings.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 774-774: Either all return statements in a function should return an expression, or none of them should.
(R1710)
881-881: Fix typo in comment.There's a typo in the comment: "customi" should be "custom".
- # we construct a path object to allow for a customi + # we construct a path object to allow for a custom
885-891: Consider adding validation for DataFrame operations.While the sacct command format is controlled (as noted in previous reviews), it would be good practice to add basic validation to ensure the DataFrame is not empty before processing, especially for robustness in edge cases.
Consider adding a check after DataFrame creation:
df = pd.DataFrame( (line.split("|") for line in lines), columns=[ "JobID", "JobName", "Comment", "Elapsed", "TotalCPU", "NNodes", "NCPUS", "MaxRSS", "ReqMem", ], ) + + if df.empty: + self.logger.warning( + f"No job data found for workflow {self.run_uuid}. " + "This may indicate that jobs haven't been processed by SLURM accounting yet." + ) + return None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(6 hunks)tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/utils.py (1)
delete_empty_dirs(26-48)snakemake_executor_plugin_slurm/efficiency_report.py (3)
time_to_seconds(5-17)parse_maxrss(20-30)parse_reqmem(33-50)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (1)
ExecutorSettings(42-124)
🪛 Pylint (3.3.7)
snakemake_executor_plugin_slurm/__init__.py
[error] 19-19: Unable to import 'pandas'
(E0401)
[refactor] 774-774: Either all return statements in a function should return an expression, or none of them should.
(R1710)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 41-41: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (7)
tests/tests.py (1)
22-25: LGTM: Timing adjustment looks reasonable.The increase in
init_seconds_before_status_checksfrom 1 to 2 seconds is a minor timing adjustment that should improve test stability.snakemake_executor_plugin_slurm/__init__.py (6)
19-19: Pandas import looks correct.The pandas import is properly added to support the new DataFrame operations in the efficiency reporting feature. The static analysis warning about being unable to import pandas is likely a false positive since pandas is being added as a dependency in the project.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 19-19: Unable to import 'pandas'
(E0401)
32-38: Clean import organization for new utilities.The imports are well-organized, bringing in the necessary utility functions and efficiency report parsing functions from the new modules.
116-124: Good addition of configurable efficiency reporting.The
efficiency_reportfield is properly implemented as a boolean flag with clear documentation. This allows users to opt-in to the efficiency reporting feature.
169-182: Well-structured shutdown method override.The implementation correctly calls the parent shutdown method first, then performs cleanup and conditional efficiency reporting. This follows good inheritance practices and ensures proper cleanup order.
850-857: Memory usage calculation handles zero division correctly.The code properly protects against division by zero in memory usage calculation by checking if
RequestedMem_MB > 0before dividing. This addresses the concern from previous reviews about potential division by zero issues.
862-862: Effective filtering of SLURM auxiliary jobs.The filtering logic correctly removes "batch" and "extern" jobs, which are SLURM internal job steps that users typically don't need in efficiency reports. This focuses the report on actual user workload.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/tests.py (1)
41-60:⚠️ Potential issueCritical issue: Test will fail without proper SLURM environment mocking.
Based on previous review discussions, this test will fail in environments without SLURM because the efficiency report generation depends on the
sacctcommand. The current implementation searches for files that won't be created ifsacctfails.You need to implement the mocking solution discussed in previous reviews. Here's the corrected implementation:
def test_simple_workflow(self, tmp_path): + import subprocess + from unittest.mock import patch, MagicMock + + # Mock sacct output with expected format: + # 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 + mock_result = MagicMock() + mock_result.stdout = mock_sacct_output + mock_result.returncode = 0 + + 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 - # current working directory pattern = re.compile(r"efficiency_report_[\w-]+\.csv") report_found = False - # as the directory is unclear, we need a path walk: - for root, dirs, files in os.walk("/tmp/pytest-of-runner/"): - for fname in files: - if pattern.match(fname): - report_found = True - report_path = os.path.join(root, fname) - # Verify it's not empty - assert ( - os.stat(report_path).st_size > 0 - ), f"Efficiency report {report_path} is empty" - break + # Check both current working directory and tmp_path for the report + from pathlib import Path + for search_dir in [Path.cwd(), tmp_path]: + for filepath in search_dir.glob("efficiency_report_*.csv"): + if pattern.match(filepath.name): + report_found = True + assert filepath.stat().st_size > 0, f"Efficiency report {filepath} is empty" + break + if report_found: + break + assert report_found, "Efficiency report file not found"Additional improvements:
- Remove hardcoded path: Replaced the environment-specific "/tmp/pytest-of-runner/" path with proper search in current directory and tmp_path
- Add SLURM mocking: Mock the
sacctcommand to return realistic test data- Fix unused variable: Use
_instead ofdirsin the loop (addresses static analysis warning)🧰 Tools
🪛 Ruff (0.11.9)
50-50: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
🪛 Pylint (3.3.7)
[convention] 41-41: Missing function or method docstring
(C0116)
[warning] 50-50: Unused variable 'dirs'
(W0612)
🧹 Nitpick comments (2)
tests/tests.py (2)
28-39: Add missing docstring for the new test class.The static analysis correctly flags missing class docstring. Consider adding documentation to explain the purpose of this test class.
class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for verifying efficiency report generation functionality.""" __test__ = True🧰 Tools
🪛 Pylint (3.3.7)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
50-50: Fix unused variable flagged by static analysis.The
dirsvariable inos.walk()is not used. Replace it with_to indicate it's intentionally unused.- for root, dirs, files in os.walk("/tmp/pytest-of-runner/"): + for root, _, files in os.walk("/tmp/pytest-of-runner/"):Note: This suggestion is part of the larger refactoring needed above.
🧰 Tools
🪛 Ruff (0.11.9)
50-50: Loop control variable
dirsnot used within loop bodyRename unused
dirsto_dirs(B007)
🪛 Pylint (3.3.7)
[warning] 50-50: Unused variable 'dirs'
(W0612)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (1)
ExecutorSettings(42-124)
🪛 Pylint (3.3.7)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 28-28: Missing class docstring
(C0115)
[convention] 31-31: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[convention] 41-41: Missing function or method docstring
(C0116)
[warning] 50-50: Unused variable 'dirs'
(W0612)
🪛 Ruff (0.11.9)
tests/tests.py
50-50: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (2)
tests/tests.py (2)
1-2: Imports added for filesystem operations and pattern matching.The new imports support the efficiency report file search functionality in the test.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
22-25: Timing adjustment for better test stability.Increasing the initial delay from 1 to 2 seconds should help with test reliability.
|
@coderabbitai: This line in the test |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@cmeesters Let me examine the current test implementation and the efficiency report creation code to suggest better alternatives to the hard-coded path. Based on my analysis, here are several better approaches to replace the hard-coded path: Recommended Solutions1. Use the actual log directory (Best approach)The efficiency report is saved in the configured log directory or current working directory. You should look there instead: def test_simple_workflow(self, tmp_path):
self.run_workflow("simple", tmp_path)
# Look in current working directory first
cwd = os.getcwd()
efficiency_files = []
# Search in current working directory
for file in os.listdir(cwd):
if re.match(r'efficiency_report_[\w-]+\.csv', file):
efficiency_files.append(os.path.join(cwd, file))
# If no files found in cwd, check the tmp_path directory tree
if not efficiency_files:
for root, _, files in os.walk(str(tmp_path)):
for file in files:
if re.match(r'efficiency_report_[\w-]+\.csv', file):
efficiency_files.append(os.path.join(root, file))
assert len(efficiency_files) > 0, "No efficiency report file found"
assert os.path.getsize(efficiency_files[0]) > 0, "Efficiency report file is empty"2. Use environment variables (Portable approach)import tempfile
def test_simple_workflow(self, tmp_path):
self.run_workflow("simple", tmp_path)
# Use system temporary directory
temp_dir = os.environ.get('TMPDIR', tempfile.gettempdir())
# Search for efficiency report files
found = False
for root, _, files in os.walk(temp_dir):
for file in files:
if re.match(r'efficiency_report_[\w-]+\.csv', file):
file_path = os.path.join(root, file)
assert os.path.getsize(file_path) > 0
found = True
break
if found:
break
assert found, f"No efficiency report found in {temp_dir}"3. Mock the file creation (Most reliable for testing)from unittest.mock import patch, mock_open
def test_simple_workflow(self, tmp_path):
mock_file = mock_open()
with patch("builtins.open", mock_file):
with patch("pandas.DataFrame.to_csv") as mock_to_csv:
self.run_workflow("simple", tmp_path)
# Verify that to_csv was called (efficiency report was created)
mock_to_csv.assert_called_once()
# Verify the filename pattern
call_args = mock_to_csv.call_args[0]
assert re.match(r'.*efficiency_report_[\w-]+\.csv', call_args[0])4. Configure explicit output directoryIf possible, modify the test setup to specify where the efficiency report should be written: def test_simple_workflow(self, tmp_path):
# Create a specific directory for test outputs
output_dir = tmp_path / "efficiency_reports"
output_dir.mkdir()
# If the executor supports configuring the output directory, use it
# Then search only in that directory
self.run_workflow("simple", tmp_path)
efficiency_files = list(output_dir.glob("efficiency_report_*.csv"))
assert len(efficiency_files) > 0
assert efficiency_files[0].stat().st_size > 0Why tmp_path might not have workedThe
The efficiency report creation logic needs to be aligned with where the test expects to find the file. I recommend approach #1 as it matches the actual file creation logic in your code, making the test more realistic and maintainable. 🏁 Script executed: Length of output: 20096 🏁 Script executed: Length of output: 866 🏁 Script executed: Length of output: 3496 🏁 Script executed: Length of output: 69 Perfect! Now I can see exactly what's happening. Looking at the efficiency report creation code, the file is saved to:
In your Recommended SolutionsOption 1: Search in Current Working Directory (Simplest)Since no def test_simple_workflow(self, tmp_path):
self.run_workflow("simple", tmp_path)
# The efficiency report is saved in current working directory
pattern = re.compile(r"efficiency_report_[\w-]+\.csv")
cwd = os.getcwd()
# Search current working directory
for fname in os.listdir(cwd):
if pattern.match(fname):
report_path = os.path.join(cwd, fname)
assert os.stat(report_path).st_size > 0, f"Efficiency report {report_path} is empty"
return # Test passed
assert False, f"Efficiency report file not found in {cwd}"Option 2: Configure logdir to use tmp_path (Most Controlled)Configure the test to save the efficiency report in a controlled location: def get_executor_settings(self) -> Optional[ExecutorSettingsBase]:
return ExecutorSettings(
efficiency_report=True,
logdir=Path("/tmp/test_efficiency_reports"), # Or use tmp_path somehow
init_seconds_before_status_checks=5,
)
def test_simple_workflow(self, tmp_path):
# Create the log directory
log_dir = Path("/tmp/test_efficiency_reports")
log_dir.mkdir(exist_ok=True)
self.run_workflow("simple", tmp_path)
pattern = re.compile(r"efficiency_report_[\w-]+\.csv")
# Search in the configured log directory
for fname in os.listdir(log_dir):
if pattern.match(fname):
report_path = log_dir / fname
assert report_path.stat().st_size > 0, f"Efficiency report {report_path} is empty"
return # Test passed
assert False, f"Efficiency report file not found in {log_dir}"Option 3: Hybrid Approach (Most Robust)Search in both possible locations: def test_simple_workflow(self, tmp_path):
self.run_workflow("simple", tmp_path)
pattern = re.compile(r"efficiency_report_[\w-]+\.csv")
# Check current working directory first
cwd = os.getcwd()
for fname in os.listdir(cwd):
if pattern.match(fname):
report_path = os.path.join(cwd, fname)
assert os.stat(report_path).st_size > 0, f"Efficiency report {report_path} is empty"
return # Test passed
# If not found in cwd, check if there's a configured logdir
settings = self.get_executor_settings()
if hasattr(settings, 'logdir') and settings.logdir:
for fname in os.listdir(settings.logdir):
if pattern.match(fname):
report_path = os.path.join(settings.logdir, fname)
assert os.stat(report_path).st_size > 0, f"Efficiency report {report_path} is empty"
return # Test passed
assert False, f"Efficiency report file not found in {cwd} or configured logdir"Option 4: Use Environment Variable for Temp DirectoryFor maximum portability across different CI environments: import tempfile
def test_simple_workflow(self, tmp_path):
# Temporarily change working directory to tmp_path
original_cwd = os.getcwd()
try:
os.chdir(tmp_path)
self.run_workflow("simple", tmp_path)
pattern = re.compile(r"efficiency_report_[\w-]+\.csv")
# Search in current directory (which is now tmp_path)
for fname in os.listdir("."):
if pattern.match(fname):
report_path = os.path.join(".", fname)
assert os.stat(report_path).st_size > 0, f"Efficiency report {report_path} is empty"
return # Test passed
assert False, f"Efficiency report file not found in {tmp_path}"
finally:
os.chdir(original_cwd)Why
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
snakemake_executor_plugin_slurm/__init__.py (1)
796-817:⚠️ Potential issueFix inconsistent return behavior.
The method returns
Noneon error but has no explicit return on success, making the return behavior inconsistent.Add an explicit return statement at the end of the method:
self.logger.info( f"Efficiency report for workflow {self.run_uuid} saved to {logfile}." ) + return logfileAlternatively, if no return value is needed, remove the
return Noneon error for consistency.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 796-796: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🧹 Nitpick comments (4)
tests/tests.py (2)
1-3: Add module docstring and consider organizing imports.The static analysis tool correctly identifies the missing module docstring. Consider adding a brief description of what this test module covers.
+""" +Test suite for the SLURM executor plugin, including workflow execution, +efficiency reporting, GPU/GRES string handling, and resource configuration. +""" import os import re from pathlib import Path🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
29-42: Add class and method docstrings.The static analysis tool correctly identifies missing docstrings for the new test class and methods.
class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for efficiency report generation functionality.""" __test__ = True def get_executor(self) -> str: + """Return the executor name for testing.""" return "slurm" def get_executor_settings(self) -> Optional[ExecutorSettingsBase]: + """Return executor settings with efficiency reporting enabled.""" return ExecutorSettings(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 29-29: Missing class docstring
(C0115)
[convention] 32-32: Missing function or method docstring
(C0116)
[convention] 35-35: Missing function or method docstring
(C0116)
snakemake_executor_plugin_slurm/__init__.py (2)
116-146: LGTM! Well-designed configuration fields with good defaults.The new ExecutorSettings fields are well-documented and provide appropriate configurability for the efficiency reporting feature. The 80% default threshold is reasonable for CPU efficiency warnings.
Consider clarifying in the
efficiency_report_pathhelp text that it should be a directory path, not a full file path, since the implementation appends the filename to this path.
904-904: Fix typo in comment.There's a typo in the comment that should be corrected for clarity.
- # we construct a path object to allow for a customi + # we construct a path object to allow for a custom
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/__init__.py(6 hunks)tests/tests.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
snakemake_executor_plugin_slurm/__init__.py (1)
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2025-06-03T17:10:51.258Z
Learning: In the snakemake-executor-plugin-slurm repository, the create_efficiency_report method in snakemake_executor_plugin_slurm/__init__.py contains temporary testing/debugging code that logs directory contents using os.listdir() and directory paths. This code was inserted for testing purposes only and should be removed from the production implementation.
🧬 Code Graph Analysis (2)
snakemake_executor_plugin_slurm/__init__.py (2)
snakemake_executor_plugin_slurm/utils.py (1)
delete_empty_dirs(26-48)snakemake_executor_plugin_slurm/efficiency_report.py (3)
time_to_seconds(5-17)parse_maxrss(20-30)parse_reqmem(33-50)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (1)
ExecutorSettings(42-146)
🪛 Pylint (3.3.7)
snakemake_executor_plugin_slurm/__init__.py
[error] 19-19: Unable to import 'pandas'
(E0401)
[refactor] 796-796: Either all return statements in a function should return an expression, or none of them should.
(R1710)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 29-29: Missing class docstring
(C0115)
[convention] 32-32: Missing function or method docstring
(C0116)
[convention] 35-35: Missing function or method docstring
(C0116)
[convention] 43-43: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (9)
tests/tests.py (1)
23-27: Minor timing adjustment looks good.The increase from 1 to 2 seconds for
init_seconds_before_status_checksis a reasonable change to improve test reliability.snakemake_executor_plugin_slurm/__init__.py (8)
18-19: LGTM! Pandas import is appropriate for the new efficiency reporting functionality.The pandas import is necessary for DataFrame operations in the efficiency report generation. The static analysis error is a false positive since pandas is properly added as a dependency.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 19-19: Unable to import 'pandas'
(E0401)
32-37: LGTM! Well-structured imports for the new efficiency reporting functionality.The imports from the local modules are appropriately organized and necessary for the efficiency reporting feature.
191-204: LGTM! Clean shutdown method implementation.The shutdown method override properly calls the parent method first, then performs the additional cleanup and optional efficiency reporting. This is a well-structured approach that follows good OOP practices.
215-215: LGTM! Improved logging consistency.Adding the trailing period improves consistency in log message formatting.
224-224: LGTM! Appropriate severity level for deletion failures.Changing from warning to error is appropriate since file deletion failures represent actual problems that should be highlighted.
229-231: LGTM! Improved error handling and messaging.The enhanced error message and appropriate error severity level provide better feedback when directory cleanup fails.
788-794: LGTM! Improved error message formatting.The enhanced formatting makes the error message clearer and more readable while maintaining the same important information.
796-921: LGTM! Comprehensive efficiency reporting implementation with excellent functionality.This is a well-implemented efficiency reporting feature that:
✅ Uses controlled sacct format ensuring column presence (following previous feedback)
✅ Properly handles missing Comment column with appropriate warnings
✅ Protects against division by zero in memory usage calculations
✅ Filters out batch/extern jobs appropriately
✅ Provides configurable efficiency threshold warnings
✅ Saves reports to configurable locationsThe implementation demonstrates good understanding of SLURM accounting data and provides valuable insights for workflow optimization.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 796-796: Either all return statements in a function should return an expression, or none of them should.
(R1710)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/tests.py (1)
43-72:⚠️ Potential issueCRITICAL: Implement proper SLURM environment mocking as discussed in previous reviews.
This test implementation still has the fundamental issues that were extensively discussed in previous reviews:
- Hardcoded paths: Using
/tmp/pytest-of-runner/makes the test fragile and non-portable- Missing SLURM mocking: The test will fail in CI environments without SLURM since the
sacctcommand is not mocked- Path inconsistencies: The search logic is disconnected from where the efficiency report is actually created
Based on the extensive previous review discussion, you need to implement the recommended mocking solution.
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 - - # as the directory is unclear, we need a path walk: - for root, _, files in os.walk("/tmp/pytest-of-runner/"): - for fname in files: - if pattern.match(fname): - report_found = True - report_path = os.path.join(root, 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 + from pathlib import Path + + # 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 current working directory + pattern = re.compile(r"efficiency_report_[\w-]+\.csv") + report_found = False + + # Check both cwd and the tmp_path for the report file + for search_dir in [Path.cwd(), tmp_path]: + for filepath in search_dir.glob("efficiency_report_*.csv"): + if pattern.match(filepath.name): + report_found = True + # Verify it's not empty + assert filepath.stat().st_size > 0, f"Efficiency report {filepath} is empty" + break + if report_found: + break + + assert report_found, "Efficiency report was not generated"This solution:
- ✅ Eliminates hardcoded paths
- ✅ Mocks the SLURM
sacctcommand as extensively discussed in previous reviews- ✅ Works in any environment (with or without SLURM)
- ✅ Tests the actual efficiency report generation logic
- ✅ Uses proper search locations instead of hardcoded paths
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 43-43: Missing function or method docstring
(C0116)
🧹 Nitpick comments (2)
tests/tests.py (2)
3-3: Remove unused import.The
Pathimport frompathlibis not used in the current implementation.-from pathlib import Path🧰 Tools
🪛 Ruff (0.11.9)
3-3:
pathlib.Pathimported but unusedRemove unused import:
pathlib.Path(F401)
🪛 Pylint (3.3.7)
[warning] 3-3: Unused Path imported from pathlib
(W0611)
29-42: Add missing docstrings for better code documentation.The new test class and its methods lack docstrings, which affects code maintainability and understanding.
class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for SLURM efficiency reporting functionality.""" __test__ = True def get_executor(self) -> str: + """Return the executor type for efficiency report testing.""" return "slurm" def get_executor_settings(self) -> Optional[ExecutorSettingsBase]: + """Configure executor settings to enable efficiency reporting.""" return ExecutorSettings( efficiency_report=True, init_seconds_before_status_checks=5, # efficiency_report_path=Path("/tmp/efficiency_report_test"), # seconds_between_status_checks=5, )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 29-29: Missing class docstring
(C0115)
[convention] 32-32: Missing function or method docstring
(C0116)
[convention] 35-35: Missing function or method docstring
(C0116)
📜 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
3-3: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
🪛 Pylint (3.3.7)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[warning] 3-3: Unused Path imported from pathlib
(W0611)
[convention] 29-29: Missing class docstring
(C0115)
[convention] 32-32: Missing function or method docstring
(C0116)
[convention] 35-35: Missing function or method docstring
(C0116)
[convention] 43-43: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/tests.py (1)
43-74:⚠️ Potential issueCritical issues remain unresolved - implement the previously recommended mocking solution.
This test still suffers from the same fundamental issues that were extensively discussed in previous reviews:
- Hardcoded paths: Using
/tmp/pytest-of-runner/makes tests fragile and environment-dependent- Missing SLURM mocking: The test will fail in CI environments without SLURM because
sacctcommand is unavailable- Debug code: Contains print statements and commented-out troubleshooting code
The comprehensive mocking solution was already provided in previous reviews but hasn't been implemented. As discussed, the root cause is that
sacctfails in test environments, so no efficiency report gets created.Implement the previously recommended solution:
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 the tmp_path directory for debugging - print(f"'tmp_path' is: {tmp_path}") - - # as the directory is unclear, we need a path walk: - for root, _, files in os.walk("/tmp/pytest-of-runner/"): - for fname in files: - if pattern.match(fname): - report_found = True - report_path = os.path.join(root, 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 + from pathlib import Path + + # 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 current working directory + pattern = re.compile(r"efficiency_report_[\w-]+\.csv") + report_found = False + + # Check both cwd and the tmp_path for the report file + for search_dir in [Path.cwd(), tmp_path]: + for filepath in search_dir.glob("efficiency_report_*.csv"): + if pattern.match(filepath.name): + report_found = True + # Verify it's not empty + assert filepath.stat().st_size > 0, f"Efficiency report {filepath} is empty" + break + if report_found: + break + + assert report_found, "Efficiency report was not generated"This solution:
- ✅ Eliminates all hardcoded paths
- ✅ Mocks the SLURM
sacctcommand as required- ✅ Works in any environment (with or without SLURM)
- ✅ Tests the actual efficiency report generation logic
- ✅ Removes debug code and commented sections
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 43-43: Missing function or method docstring
(C0116)
🧹 Nitpick comments (1)
tests/tests.py (1)
29-41: Add missing docstrings to address static analysis warnings.The test class and methods are missing docstrings, which would improve code documentation and address the pylint warnings.
+class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for SLURM efficiency report generation functionality.""" __test__ = True def get_executor(self) -> str: + """Return the executor name for testing.""" return "slurm" def get_executor_settings(self) -> Optional[ExecutorSettingsBase]: + """Return executor settings with efficiency reporting enabled.""" return ExecutorSettings(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 29-29: Missing class docstring
(C0115)
[convention] 32-32: Missing function or method docstring
(C0116)
[convention] 35-35: Missing function or method docstring
(C0116)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (1)
ExecutorSettings(42-146)
🪛 Pylint (3.3.7)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 29-29: Missing class docstring
(C0115)
[convention] 32-32: Missing function or method docstring
(C0116)
[convention] 35-35: Missing function or method docstring
(C0116)
[convention] 43-43: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (2)
tests/tests.py (2)
1-2: LGTM! Imports are appropriate for the test functionality.The
osandreimports are necessary for the file operations and regex pattern matching used in the efficiency report test.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
23-26: LGTM! Timing adjustment improves test stability.Increasing the initial delay from 1 to 2 seconds is a reasonable change to improve test reliability and avoid potential race conditions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/tests.py (1)
44-75:⚠️ Potential issueCritical: Implement the SLURM mocking solution from previous reviews.
This test implementation still has the same critical issues that were extensively discussed in previous reviews:
- Hardcoded paths: Using
/tmp/pytest-of-runner/makes tests fragile and non-portable- Missing SLURM mocking: Without SLURM environment, the
sacctcommand fails and no efficiency report is created- Test will fail in CI: As noted in previous reviews, this approach doesn't work in environments without SLURM
Please implement the mocking solution that was provided in previous reviews:
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 the tmp_path directory for debugging - print(f"'tmp_path' is: {tmp_path}") - - # as the directory is unclear, we need a path walk: - for root, _, files in os.walk("/tmp/pytest-of-runner/"): - for fname in files: - if pattern.match(fname): - report_found = True - report_path = os.path.join(root, 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 + from pathlib import Path + + # 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 current working directory + pattern = re.compile(r"efficiency_report_[\w-]+\.csv") + report_found = False + + # Check both cwd and the tmp_path for the report file + for search_dir in [Path.cwd(), tmp_path]: + for filepath in search_dir.glob("efficiency_report_*.csv"): + if pattern.match(filepath.name): + report_found = True + # Verify it's not empty + assert filepath.stat().st_size > 0, f"Efficiency report {filepath} is empty" + break + if report_found: + break + + assert report_found, "Efficiency report was not generated"This solution:
- ✅ Eliminates hardcoded paths
- ✅ Mocks the SLURM
sacctcommand as discussed in previous reviews- ✅ Works in any environment (with or without SLURM)
- ✅ Tests the actual efficiency report generation logic
- ✅ Uses proper search locations instead of hardcoded paths
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 44-44: Missing function or method docstring
(C0116)
🧹 Nitpick comments (1)
tests/tests.py (1)
30-42: Add docstrings for better documentation.The test class structure and settings are appropriate for testing efficiency report functionality. However, consider adding docstrings to improve code documentation as flagged by static analysis.
+class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase): + """Test class for SLURM efficiency report generation functionality.""" __test__ = True + def get_executor(self) -> str: + """Return the executor type for efficiency report testing.""" return "slurm" + def get_executor_settings(self) -> Optional[ExecutorSettingsBase]: + """Configure executor settings to enable efficiency reporting.""" return ExecutorSettings(🧰 Tools
🪛 Pylint (3.3.7)
[convention] 30-30: Missing class docstring
(C0115)
[convention] 33-33: Missing function or method docstring
(C0116)
[convention] 36-36: Missing function or method docstring
(C0116)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/tests.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/__init__.py (1)
ExecutorSettings(42-146)
🪛 Pylint (3.3.7)
tests/tests.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 30-30: Missing class docstring
(C0115)
[convention] 33-33: Missing function or method docstring
(C0116)
[convention] 36-36: Missing function or method docstring
(C0116)
[convention] 44-44: Missing function or method docstring
(C0116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testing
🔇 Additional comments (2)
tests/tests.py (2)
1-2: LGTM! Appropriate imports for the new test functionality.The added imports for
osandreare necessary for the filesystem operations and regex pattern matching used in the efficiency report test.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
24-27: LGTM! Minor timing adjustments for test stability.The increase in initialization delay from 1 to 2 seconds and the commented-out status check interval are reasonable adjustments for test reliability.
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