Add prime slurm command for managing Slurm clusters - #893
Conversation
New prime slurm command group backed by the platform's new /api/v1/slurm-clusters/* API - list, get, connect/ssh, members, add/remove-member, sudo, rename, delete, accounting, utilization. connect/ssh shells straight into the cluster's login node, same pattern as prime pods connect, instead of just printing the ssh command.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e78c70419
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"{username}@{cluster.ssh_host}", | ||
| ] | ||
| try: | ||
| subprocess.run(ssh_command) |
There was a problem hiding this comment.
When ssh exits nonzero—for example because authentication fails or the host is unreachable—subprocess.run() returns a result rather than raising because check is false by default. The CalledProcessError handler is therefore unreachable, and prime slurm connect reports a successful exit status to scripts despite the failed connection; inspect the return code or run with check=True.
Useful? React with 👍 / 👎.
| console.print(f"[bold]Accounting for the last {rollup.days} day(s)[/bold]") | ||
| console.print(f"Total jobs: {rollup.total_jobs}") | ||
|
|
||
| if rollup.gpu_hours_by_user: |
There was a problem hiding this comment.
When accounting is available and the API returns throughput or queue_wait data, the default table output never displays either collection: after printing the total it proceeds directly to GPU-hours and outcomes. This makes the command's documented throughput and queue-wait rollups accessible only through JSON and can even reduce a populated response to just Total jobs; add table output for both series.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5e78c70. Configure here.
- connect/ssh now exits with the actual ssh return code instead of always reporting success - subprocess.run defaults check to False, so a failed SSH auth or unreachable host previously exited 0 - accounting table output now renders throughput and queue-wait, not just GPU-hours and outcomes
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bab508f4bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except Exception as e: | ||
| console.print(f"[red]Unexpected error:[/red] {str(e)}") | ||
| raise typer.Exit(1) |
There was a problem hiding this comment.
When the cluster is unavailable, the key is missing, or ssh returns nonzero, typer.Exit is raised inside this try and then immediately caught here because it subclasses Exception. Consequently routine failures print a misleading Unexpected error message, and the newly added raise typer.Exit(result.returncode) always becomes exit code 1 instead of propagating the SSH status; handle typer.Exit separately or move these exits outside the generic handler.
Useful? React with 👍 / 👎.
typer.Exit subclasses Exception, so the raise typer.Exit(result.returncode) added last round - along with the not-connectable and missing-key exits - was being caught by the generic except Exception handler right below it: the real exit code got stomped to 1 and a bogus "Unexpected error: <code>" line printed on top of the real message. Catch typer.Exit explicitly and re-raise before the generic handler runs. Verified end-to-end: a failing ssh (fake binary exiting 255) now exits prime slurm connect with 255, no spurious message.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Matches the backend now exposing both fields. Table view truncates long key lines to 60 chars (full value always in --output json); get-member detail view (_print_member) prints uid and every full key on its own line.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Not core to finding a cluster's connection info - drops the Free/Total GPUs and Nodes columns from list, the node/health section from get, and the utilization command entirely.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Drops get, connect/ssh, sudo, rename, delete, and accounting - this is meant to be safe to hand a scoped token to (a human admin or an automated agent) whose job is member management, not infra control. Keeps list, members, add-member, remove-member.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7845ebfec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| console.print(f"Sudo: {'yes' if m.sudo else 'no'}") | ||
| console.print(f"Status: {m.status}") | ||
| for key in m.ssh_authorized_keys: | ||
| console.print(f"SSH Key: {key}") |
There was a problem hiding this comment.
When a valid authorized-key comment contains Rich syntax such as [/prod] or [laptop], this passes the server value to Console.print as markup. A mismatched closing tag raises MarkupError after the POST has already succeeded, causing add-member to exit with failure despite creating the member, while ordinary tags are silently removed from the displayed key; the members table has the same parsing issue. Render these values as Text or disable/escape markup.
Useful? React with 👍 / 👎.
SSH key comments, linked-user email/name, and cluster display names are all free text someone else set - server-controlled from this CLI's perspective. Rich treats [...] as markup by default, so a key comment like "[/prod]" throws a MarkupError after the mutation already succeeded, and ordinary bracketed text gets silently swallowed as styling instead of displayed. Escapes all four spots that render this kind of value: the members table's SSH-key and linked-user columns, the list table's cluster name, and add-member's SSH-key printout.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 714f236a27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for c in clusters: | ||
| table.add_row( | ||
| c.id, | ||
| escape(c.display_name), |
There was a problem hiding this comment.
When --plain is used and a cluster name contains Rich-style brackets (for example [prod]), escape() adds literal backslashes that remain in the output because PrimeConsole.print re-renders tables with markup=False (utils/plain.py). The same corruption affects SSH-key comments and linked-user values on lines 126 and 129, so plain output no longer faithfully represents the server data; pass these cells as Text objects rather than pre-escaped strings.
Useful? React with 👍 / 👎.
escape() only round-trips correctly through Rich's normal markup parser - the --plain path renders tables through a second, separate markup=False console that doesn't understand \[ as an escaped literal, so escaped cells came out with the backslash still in them. Text objects carry literal content with no markup parsing in either path. The single non-table SSH-key printout keeps escape() - that one already round-trips correctly in both modes.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
prime slurmcommand group:list,members,add-member,remove-member/api/v1/slurm-clusters/*API (see companion PR)Notes
Note
Medium Risk
New admin-facing surface that grants or revokes SSH access to Slurm clusters; impact depends on platform authorization and correct key handling, though scope is limited to member roster APIs.
Overview
Adds a new Compute command group,
prime slurm, for listing team Slurm clusters and managing who has SSH access—scoped to roster operations only (no cluster lifecycle).A new
SlurmClustersClienttalks to/slurm-clusters/{team_id}endpoints with Pydantic models for cluster summaries and members. Commands includelistandmembers(table or--output json), plus adminadd-member(repeatable--ssh-key, optional--link-user) andremove-member(confirmation unless-y). Team context comes from config or--team-id, matching other CLI commands.main.pyregisters the typer app under Compute, and cluster status colors are added for Rich tables onprime slurm list.Reviewed by Cursor Bugbot for commit e8d998d. Bugbot is set up for automated code reviews on this repo. Configure here.