Fix SLURM submission with additional information #449
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
WalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pysqa/wrapper/slurm.py(1 hunks)
🔇 Additional comments (1)
pysqa/wrapper/slurm.py (1)
50-56: Good fix: handle sbatch --parsable outputs with semicolon.Splitting off the semicolon-suffixed part aligns with Slurm’s parsable output format. This resolves failures like "12345;cluster".
Comment on lines
+50
to
+56
| return int( | ||
| queue_submit_output.splitlines()[-1] | ||
| .rstrip() | ||
| .lstrip() | ||
| .split()[-1] | ||
| .split(";")[0] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make parsing resilient (blank lines, “Submitted…” form, array jobs).
Current logic can break on:
- Trailing blank line (IndexError).
- Array IDs like "12345_1" or "12345_[1-10]" (ValueError on int()).
- It can be simplified using strip()/partition() and made future-proof with a regex for leading digits.
Proposed refactor:
- Use last non-empty line, take last token, strip semicolon-suffix, then extract leading digits.
- Raise a clear error when parsing fails.
Apply this diff within the method:
- return int(
- queue_submit_output.splitlines()[-1]
- .rstrip()
- .lstrip()
- .split()[-1]
- .split(";")[0]
- )
+ lines = [ln.strip() for ln in queue_submit_output.splitlines() if ln.strip()]
+ if not lines:
+ raise ValueError("Empty sbatch output; cannot parse Slurm job ID.")
+ last = lines[-1]
+ token = last.split()[-1] # works for "Submitted batch job 12345" too
+ token = token.partition(";")[0] # remove parsable suffix like ";cluster"
+ m = re.match(r"^(\d+)", token)
+ if not m:
+ raise ValueError(f"Unable to parse Slurm job ID from: {last!r}")
+ return int(m.group(1))Additionally, add the import at the top of the file:
import reTests to cover:
- "12345"
- "12345;cluster"
- "Submitted batch job 12345"
- "12345_1" and "12345_[1-10]"
- Output ending with a blank line
🤖 Prompt for AI Agents
In pysqa/wrapper/slurm.py around lines 50 to 56, the current parsing logic for
extracting job IDs from queue_submit_output is fragile and can fail on blank
lines, array job IDs, or different output formats. Refactor the code to first
find the last non-empty line, then extract the last token, strip any trailing
semicolon, and use a regex to extract only the leading digits from that token.
Add error handling to raise a clear exception if parsing fails. Also, add
"import re" at the top of the file. Write tests covering various output cases
including plain IDs, IDs with semicolons, "Submitted batch job" lines, array job
IDs, and outputs ending with blank lines.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit