Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded package-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant CLI as CLI (crun)
participant Main as MainCrun
participant Proto as InteractiveJobAdditionalMeta
participant Server as Server
User->>CLI: run `crun --mpi="<value>"`
CLI->>CLI: parse flags (sets FlagMpi)
CLI->>Main: invoke MainCrun (reads FlagMpi)
Main->>Main: validate FlagMpi == "pmix"
Main->>Proto: set iaMeta.Mpi = FlagMpi
Main->>Server: send serialized InteractiveJobAdditionalMeta (includes mpi)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/calloc/cmd.go`:
- Line 54: The file contains a gofmt formatting issue around the FlagMpi
declaration; run gofmt -w on the source file that declares FlagMpi to fix
formatting (or use your editor's Go formatting) so the file compiles cleanly and
the pipeline passes; ensure the FlagMpi line and surrounding code are properly
formatted by gofmt.
- Line 126: The --mpi flag (FlagMpi) is registered but never copied into the job
metadata before submit: in the calloc command flow where InteractiveMeta is
initialized to nil (function calloc / variable iaMeta), retrieve or create the
InteractiveMeta struct and set iaMeta.Mpi = FlagMpi (same as crun does) before
calling the submission routine so the value is propagated to the job controller;
ensure you update the same variable used downstream (iaMeta / InteractiveMeta)
so the metadata contains the FlagMpi value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e310a670-e045-467d-8cf0-4ea50cde4d13
📒 Files selected for processing (4)
internal/calloc/cmd.gointernal/crun/cmd.gointernal/crun/crun.goprotos/PublicDefs.proto
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/calloc/cmd.go`:
- Line 54: Add a CLI flag binding for the previously declared FlagMpi in
calloc's init() so the --mpi option actually works: in the init() function of
the calloc command register FlagMpi with the root command flags (e.g.,
rootCmd.Flags().StringVar(&FlagMpi, "mpi", <default>, "<help>") analogous to how
FlagMpi is registered in crun's init), ensuring you use the same variable name
FlagMpi and an appropriate default and help string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/calloc/cmd.go (1)
53-53:⚠️ Potential issue | 🟠 MajorRegister
FlagMpiininit()socalloc --mpiis usable.Line 53 introduces
FlagMpi, butinit()never binds it viaRootCmd.Flags().StringVar...; as a result,calloc --mpiwill not be recognized.Suggested fix
func init() { @@ RootCmd.Flags().StringVarP(&FlagDependency, "dependency", "d", "", "Conditions for job to execute") + RootCmd.Flags().StringVar(&FlagMpi, "mpi", "", "MPI type/version for interactive job") RootCmd.Flags().StringVarP(&FlagSignal, "signal", "s", "", "Send signal when time limit within time seconds, format: [{R}:]<sig_num>[`@sig_time`]") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/calloc/cmd.go` at line 53, The new package-level variable FlagMpi is never registered with the CLI, so the --mpi flag is not recognized; in init() register it by calling RootCmd.Flags().StringVar(&FlagMpi, "mpi", "", "enable/use MPI implementation (e.g. mpich|openmpi)") (or StringVarP if a short option is desired), ensuring the binding uses the FlagMpi variable and RootCmd.Flags() so calloc --mpi is parsed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/calloc/cmd.go`:
- Line 53: The new package-level variable FlagMpi is never registered with the
CLI, so the --mpi flag is not recognized; in init() register it by calling
RootCmd.Flags().StringVar(&FlagMpi, "mpi", "", "enable/use MPI implementation
(e.g. mpich|openmpi)") (or StringVarP if a short option is desired), ensuring
the binding uses the FlagMpi variable and RootCmd.Flags() so calloc --mpi is
parsed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad5406f2-7d17-4007-927c-a44756dc7fcf
📒 Files selected for processing (4)
internal/calloc/cmd.gointernal/crun/cmd.gointernal/crun/crun.goprotos/PublicDefs.proto
✅ Files skipped from review due to trivial changes (2)
- internal/crun/cmd.go
- internal/crun/crun.go
🚧 Files skipped from review as they are similar to previous changes (1)
- protos/PublicDefs.proto
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/crun/crun.go (1)
1880-1880: Consider validatingFlagMpiagainst supported MPI types before accepting it.
iaMeta.Mpi = FlagMpiforwards the raw CLI string without validation. If specific values (e.g.,pmix,pmi2,none) are supported, validating here enables fast-fail with a clear CLI error instead of deferring to backend rejection. Note that the--mpiflag is currently marked as unsupported in the slurm wrapper (internal/cwrapper/slurm.go:1299-1300), so validation can be added when this feature is fully enabled.Regarding
calloc:FlagMpiexists ininternal/calloc/cmd.gobut is not exposed as a CLI flag, so there's no inconsistency—onlycrunhas the flag wired up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crun/crun.go` at line 1880, The assignment iaMeta.Mpi = FlagMpi accepts the raw CLI string without validation; update the code around the iaMeta.Mpi assignment to validate FlagMpi against the allowed MPI types (e.g., "pmix", "pmi2", "none" or your project's canonical list) and return a clear CLI error if the value is unsupported; specifically, in the block that sets iaMeta.Mpi and references FlagMpi, check membership in the allowed set, normalize/case-fold as needed, and abort with a user-facing error message when invalid to fail fast before backend submission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/crun/crun.go`:
- Line 1880: The assignment iaMeta.Mpi = FlagMpi accepts the raw CLI string
without validation; update the code around the iaMeta.Mpi assignment to validate
FlagMpi against the allowed MPI types (e.g., "pmix", "pmi2", "none" or your
project's canonical list) and return a clear CLI error if the value is
unsupported; specifically, in the block that sets iaMeta.Mpi and references
FlagMpi, check membership in the allowed set, normalize/case-fold as needed, and
abort with a user-facing error message when invalid to fail fast before backend
submission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e115dafd-a1ff-49c6-b1e0-630152e4191d
📒 Files selected for processing (4)
internal/calloc/cmd.gointernal/crun/cmd.gointernal/crun/crun.goprotos/PublicDefs.proto
✅ Files skipped from review due to trivial changes (2)
- internal/crun/cmd.go
- internal/calloc/cmd.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/crun/crun.go (1)
71-73: Consider a set of supported MPI types for extensibility.If additional MPI implementations (e.g.
pmi2,openmpi) are likely to be supported later, a single constant plus a hard-coded equality check will not scale cleanly. A small set/slice would also let you surface the supported list in the error message.♻️ Sketch
- MpiTypePmix string = "pmix" + MpiTypePmix string = "pmix" +) + +var SupportedMpiTypes = map[string]struct{}{ + MpiTypePmix: {}, +} + +const ( + _mpiTypesEnd = ""(or simply keep the constant and switch the check to a map lookup.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/crun/crun.go` around lines 71 - 73, The code currently defines only a single MPI constant (MpiTypePmix) which doesn't scale; replace the single-constant check with a reusable collection of supported types (e.g. a slice var SupportedMpiTypes = []string{"pmix"} or a map var supportedMpiTypes = map[string]struct{}{"pmix":{}}) and update any validation logic to check membership against that collection (instead of equality to MpiTypePmix) and to use the collection to construct clearer error messages listing supported MPI types; look for uses of MpiTypePmix in internal/crun/crun.go (validation/selection logic) and change them to consult the new SupportedMpiTypes/supportedMpiTypes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/crun/crun.go`:
- Around line 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.
---
Nitpick comments:
In `@internal/crun/crun.go`:
- Around line 71-73: The code currently defines only a single MPI constant
(MpiTypePmix) which doesn't scale; replace the single-constant check with a
reusable collection of supported types (e.g. a slice var SupportedMpiTypes =
[]string{"pmix"} or a map var supportedMpiTypes =
map[string]struct{}{"pmix":{}}) and update any validation logic to check
membership against that collection (instead of equality to MpiTypePmix) and to
use the collection to construct clearer error messages listing supported MPI
types; look for uses of MpiTypePmix in internal/crun/crun.go
(validation/selection logic) and change them to consult the new
SupportedMpiTypes/supportedMpiTypes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if FlagMpi != MpiTypePmix { | ||
| return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: unsupported MPI type %s.", FlagMpi)) | ||
| } | ||
|
|
||
| iaMeta.Mpi = FlagMpi |
There was a problem hiding this comment.
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.
| 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.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes / Behavior