Skip to content

feat: pmix v2 #421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
huerni wants to merge 5 commits into
base: master
Choose a base branch
from
+12 −0
Open

feat: pmix v2 #421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
96f5a2d
feat
huerni Apr 8, 2025
8da135b
feat: merge master
huerni Mar 6, 2026
fd188bc
style: fix alignment of FlagMpi variable declaration in calloc cmd.go
huerni Mar 30, 2026
ba078c9
fix(calloc): remove unused MPI flag from calloc command
huerni Mar 31, 2026
39aebcc
fix: add MpiTypePmix constant and validate MPI type in MainCrun function
huerni Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/calloc/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ var (
FlagMailUser string
FlagComment string
FlagDependency string
FlagMpi string
Comment thread
coderabbitai[bot] marked this conversation as resolved.

FlagConfigFilePath string
FlagDebugLevel string
Expand Down
1 change: 1 addition & 0 deletions internal/crun/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,5 @@ func init() {
RootCmd.Flags().StringVarP(&FlagTaskProlog, "task-prolog", "", "", "Task prolog of the job")
RootCmd.Flags().StringVarP(&FlagTaskEpilog, "task-epilog", "", "", "Task epilog of the job")
RootCmd.Flags().StringVarP(&FlagSignal, "signal", "s", "", "Send signal when time limit within time seconds, format: [{R}:]<sig_num>[@sig_time]")
RootCmd.Flags().StringVarP(&FlagMpi, "mpi", "", "", "MPI used for the job")
}
8 changes: 8 additions & 0 deletions internal/crun/crun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ const (
const (
FlagIOForwardALL string = "all"
FlagIOForwardNONE string = "none"

MpiTypePmix string = "pmix"
)

type GlobalVariables struct {
Expand Down Expand Up @@ -1877,6 +1879,12 @@ func MainCrun(cmd *cobra.Command, args []string) error {
log.Debugf("X11 forwarding enabled (%v:%d). ", target, port)
}

if FlagMpi != MpiTypePmix {
return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: unsupported MPI type %s.", FlagMpi))
}

iaMeta.Mpi = FlagMpi
Comment on lines +1882 to +1886
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: default (no --mpi) now rejects every crun invocation.

FlagMpi is registered with an empty-string default (internal/crun/cmd.go line 169). With the current check, any crun run without --mpi fails with Invalid argument: unsupported MPI type . (note the empty value interpolated), i.e. the MPI feature is no longer opt-in — it is mandatory. This breaks all existing non-MPI interactive workloads.

Validate only when the user explicitly set the flag, e.g.:

🐛 Proposed fix 
-	if FlagMpi != MpiTypePmix {
-		return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: unsupported MPI type %s.", FlagMpi))
-	}
-
-	iaMeta.Mpi = FlagMpi
+	if FlagMpi != "" {
+		if FlagMpi != MpiTypePmix {
+			return util.NewCraneErr(util.ErrorCmdArg,
+				fmt.Sprintf("Invalid argument: unsupported MPI type %q (supported: %s).", FlagMpi, MpiTypePmix))
+		}
+		iaMeta.Mpi = FlagMpi
+	}

Alternatively, gate on cmd.Flags().Changed("mpi") which is the pattern used elsewhere in MainCrun.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if FlagMpi != MpiTypePmix {
return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: unsupported MPI type %s.", FlagMpi))
}
iaMeta.Mpi = FlagMpi
if FlagMpi != "" {
if FlagMpi != MpiTypePmix {
return util.NewCraneErr(util.ErrorCmdArg,
fmt.Sprintf("Invalid argument: unsupported MPI type %q (supported: %s).", FlagMpi, MpiTypePmix))
}
iaMeta.Mpi = FlagMpi
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@internal/crun/crun.go` around lines 1882 - 1886, The current check treats the
empty default FlagMpi as invalid and forces MPI to be required; update the
validation so it only runs when the user explicitly set the --mpi flag: either
skip validation when FlagMpi == "" (leave iaMeta.Mpi unset) or use the command
flag state (cmd.Flags().Changed("mpi")) before comparing FlagMpi to MpiTypePmix;
only assign iaMeta.Mpi when the flag was provided and validated.


termEnv, exits := syscall.Getenv("TERM")
if exits {
iaMeta.TermEnv = termEnv
Expand Down
2 changes: 2 additions & 0 deletions protos/PublicDefs.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,8 @@ message InteractiveJobAdditionalMeta {

// Task ID to forward crun input to
optional uint32 input_task_id = 8;

string mpi = 9;
}

message PodJobAdditionalMeta {
Expand Down
Loading