fix: efficiency report jobsteps #338
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ightly to allow unit testing of parsing sacct output.
WalkthroughThe efficiency-report logic was split into data fetching and parsing: Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant create_efficiency_report
participant get_sacct_data
participant parse_sacct_data
Note right of get_sacct_data #eef2ff: runs `sacct --parsable2`, returns lines or None
Note right of parse_sacct_data #f7f7e6: parses lines, collapses steps, computes efficiencies
Caller->>create_efficiency_report: call(e_threshold, run_uuid, e_report_path, logger)
create_efficiency_report->>get_sacct_data: get_sacct_data(run_uuid, logger)
get_sacct_data-->>create_efficiency_report: lines or None
alt lines present
create_efficiency_report->>parse_sacct_data: parse_sacct_data(lines, e_threshold, run_uuid, logger)
parse_sacct_data-->>create_efficiency_report: DataFrame (steps collapsed, efficiencies)
create_efficiency_report-->>Caller: DataFrame saved as CSV (report)
else no data
create_efficiency_report-->>Caller: None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
snakemake_executor_plugin_slurm/efficiency_report.py (1)
7-7: Remove temporary debugging code from productionThe comment indicates this
osimport is temporary. Based on previous learnings, debugging code that logs directory contents should be removed from production implementation.Apply this diff to remove the debugging code:
-import os # only temporarily needed for printf debugging import numpy as np# 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())}")Also applies to: 216-216
🧹 Nitpick comments (1)
snakemake_executor_plugin_slurm/efficiency_report.py (1)
194-195: Consider handling empty results separately from errorsThe condition
lines is None or not linestreats both error cases (None) and empty results (empty list) the same way. An empty list might be a valid result from sacct (e.g., if jobs haven't started yet or have been purged). Consider distinguishing between these cases for better error reporting.- if lines is None or not lines: - return None + if lines is None: + return None # Error occurred + + if not lines: + logger.info(f"No job data available for workflow {run_uuid} yet.") + return None # Valid but empty result
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
snakemake_executor_plugin_slurm/efficiency_report.py(4 hunks)tests/tests.py(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
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: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
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.292Z
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.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#193
File: .github/workflows/post_to_mastodon.yml:26-26
Timestamp: 2025-01-20T09:13:26.443Z
Learning: In the snakemake-executor-plugin-slurm repository, release PRs follow the naming pattern "chore(main): release X.Y.Z" where X.Y.Z is the version number.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#187
File: .github/workflows/post_to_mastodon.yml:0-0
Timestamp: 2025-01-17T17:27:32.446Z
Learning: In the mastodon publishing workflow for snakemake-executor-plugin-slurm, the PR_TITLE environment variable is required by the post_to_mastodon.sh script and should be preserved.
📚 Learning: 2025-06-24T12:41:31.837Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#221
File: tests/tests.py:62-70
Timestamp: 2025-06-24T12:41:31.837Z
Learning: In the snakemake-executor-plugin-slurm efficiency report feature, the executor is responsible for creating the efficiency_report_path directory when generating efficiency reports. Tests should not manually check for directory existence but should rely on the executor to properly create necessary directories as part of its normal operation.
Applied to files:
tests/tests.pysnakemake_executor_plugin_slurm/efficiency_report.py
📚 Learning: 2024-12-11T14:17:08.749Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#178
File: snakemake_executor_plugin_slurm/__init__.py:0-0
Timestamp: 2024-12-11T14:17:08.749Z
Learning: In the `snakemake-executor-plugin-slurm` project, when handling exceptions in `snakemake_executor_plugin_slurm/__init__.py`, prefer concise error messages and avoid unnecessary verbosity or exception chaining when it's not necessary.
Applied to files:
tests/tests.pysnakemake_executor_plugin_slurm/efficiency_report.py
📚 Learning: 2025-02-18T14:53:09.504Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#173
File: snakemake_executor_plugin_slurm/__init__.py:29-31
Timestamp: 2025-02-18T14:53:09.504Z
Learning: The snakemake-executor-plugin-slurm tests require system dependencies (gcc, python3-dev) to build the datrie package, which is a dependency of snakemake.
Applied to files:
tests/tests.py
📚 Learning: 2025-06-03T17:10:51.292Z
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.292Z
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.
Applied to files:
tests/tests.pysnakemake_executor_plugin_slurm/efficiency_report.py
📚 Learning: 2025-03-31T11:52:05.373Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#249
File: snakemake_executor_plugin_slurm/submit_string.py:21-29
Timestamp: 2025-03-31T11:52:05.373Z
Learning: In the snakemake-executor-plugin-slurm project, account and partition parameters are validated in the submit function before reaching the get_submit_command function. By design, these parameters are either empty strings or already properly formatted with SLURM parameter tags, making additional validation in get_submit_command unnecessary.
Applied to files:
tests/tests.pysnakemake_executor_plugin_slurm/efficiency_report.py
📚 Learning: 2025-05-26T12:22:07.005Z
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.
Applied to files:
tests/tests.pysnakemake_executor_plugin_slurm/efficiency_report.py
📚 Learning: 2025-01-13T09:54:22.950Z
Learnt from: cmeesters
PR: snakemake/snakemake-executor-plugin-slurm#0
File: :0-0
Timestamp: 2025-01-13T09:54:22.950Z
Learning: PR #173 (adding gres resource specification) depends on PR #28 in snakemake-executor-plugin-slurm-jobstep repository, as changes were required in the cpu-settings function of the jobstep-Executor module.
Applied to files:
tests/tests.py
📚 Learning: 2025-03-10T15:20:51.829Z
Learnt from: johanneskoester
PR: snakemake/snakemake-executor-plugin-slurm#173
File: docs/further.md:96-96
Timestamp: 2025-03-10T15:20:51.829Z
Learning: PR #173 in snakemake-executor-plugin-slurm implements GPU job support by adding resources: `gres` for generic resource specifications (e.g., 'gpu:1'), `gpu`/`gpus` for specifying GPU counts, and `gpu_model`/`gpu_manufacturer` for specifying GPU types, allowing users to request GPU resources directly rather than having to use slurm_extra.
Applied to files:
tests/tests.pysnakemake_executor_plugin_slurm/efficiency_report.py
🧬 Code Graph Analysis (1)
tests/tests.py (1)
snakemake_executor_plugin_slurm/efficiency_report.py (1)
parse_sacct_data(78-184)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: testing
🔇 Additional comments (2)
snakemake_executor_plugin_slurm/efficiency_report.py (1)
127-149: Well-implemented job step handling logicThe refactored logic correctly addresses the issue by:
- Filtering out batch/extern entries that represent SLURM overhead
- Separating main jobs from job steps
- Inheriting metadata (RuleName, RequestedMem_MB) from main jobs to their corresponding job steps
- Using only job steps in the final dataset since they contain actual resource usage
This approach resolves the confusion with multiple entries per job and ensures consistent field population.
tests/tests.py (1)
47-53: Good test coverage for job step handlingThe assertions effectively verify the core functionality:
- Job step collapsing (2 rows from 6 input lines)
- RuleName inheritance from main jobs to job steps
- RequestedMem_MB inheritance
- Actual memory usage captured from job steps
This provides good validation of the refactored parsing logic.
|
@cademirch thank for this PR! Considering the other discussion about whether we continue relying exclusively on SLURM with accounting databases, I feel we need a logical switch for people without a DB, in which case the efficiency option should not be present. Let me add this and update the docs accordingly. With a little luck, I can test and amend some time next week (and also have a closer look on your other PR). |
|
Minor note here, I think that the efficiency report should be written to the .snakemake directory. Its been cluttering up my workdirs when testing stuff |
This is why we have |
cmeesters
approved these changes
Aug 21, 2025
cmeesters
pushed a commit
that referenced
this pull request
Aug 21, 2025
🤖 I have created a release *beep* *boop* --- ## [1.6.1](v1.6.0...v1.6.1) (2025-08-21) ### Bug Fixes * efficiency report jobsteps ([#338](#338)) ([a4cbe36](a4cbe36)) * gpu tasks are unset if <= 0 ([#347](#347)) ([564e0f7](564e0f7)) * updated poetry ([#343](#343)) ([58d471d](58d471d)) ### Documentation * added notes about ntasks per gpu ([#346](#346)) ([adcd86e](adcd86e)) --- 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 - Bug Fixes - Corrected efficiency report job step handling. - Prevented invalid GPU task settings by unsetting values ≤ 0. - Documentation - Added guidance on configuring tasks per GPU. - Chores - Updated packaging tooling and version to 1.6.1. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Ah I was not aware of this. Great. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #337.
This PR makes sure for each job in sacct output, only the jobstep is reported and that each record has rulename and memrequested. Also refactors efficiency report slightly to allow unit testing of parsing sacct output.
Summary by CodeRabbit
Refactor
Tests