Refactor tools to manage Slurm jobs#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the tools to manage SLURM jobs, adding safeguards for destructive operations and fixing several corner case issues. The refactoring extracts common functionality into utility modules and improves consistency across the codebase.
- Introduces a new
removejobscommand with the same safeguard pattern ascanceljobs(requires--commitflag for destructive actions) - Refactors job search logic into a shared utility function
- Fixes regex patterns for detecting SBATCH directives and adds detection of unsupported scheduler directives
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| stepup/queue/utils.py | New utility module providing shared search_jobs function for finding slurmjob.log files |
| stepup/queue/removejobs.py | New tool to remove directories of failed/cancelled jobs with --commit safeguard |
| stepup/queue/sbatch.py | Refactored job state handling (renamed "steps" to "states"), improved regex patterns, added unsupported directive detection, and extracted DONE_STATES constant |
| stepup/queue/canceljobs.py | Refactored to use shared utilities, added --commit and --all flags for better control |
| stepup/queue/actions.py | Updated function call to match new signature of read_jobid_cluster_status |
| tests/test_sbatch.py | Added tests for regex patterns matching SBATCH directives |
| pyproject.toml | Added path dependency and updated stepup version requirement to 3.2.0 |
| .github/requirements-old.txt | Updated to match pyproject.toml dependencies |
| docs/usage.md | Updated documentation for new commands and fixed example code bug |
| docs/changelog.md | Documented all changes for upcoming release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
stepup/queue/sbatch.py:136
- The read_log function appends all stripped lines including empty ones. If a log file has trailing or intermediate empty lines, they will be included in the returned list. This can cause issues in functions that expect all lines to be non-empty status entries, such as read_status which will fail with a ValueError when trying to parse an empty line.
Consider filtering out empty lines after stripping by changing line 136 to only append non-empty lines, for example: if line: lines.append(line)
for line in f:
line = line.strip()
lines.append(line)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This PR refactors tools to manage SLURM jobs and fixes several issues in corner cases.