Add cbatch/calloc/crun --mem-per-cpu #275
Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 full review |
✅ Actions performedFull review triggered. |
1daidai1
force-pushed
the
dev/mempercpu
branch
from
04bf4da to
31b916c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cbatch/cbatch.go (1)
125-139: Blocker:return false, nilis an invalid return inBuildCbatchJob+ enforce--memvs--mem-per-cpuexclusivity in scripts.
This won’t compile, and scripts can currently specify both flags.case "--mem": + if task.MemPerCpu != nil { + return nil, fmt.Errorf("invalid argument: --mem is mutually exclusive with --mem-per-cpu (in script)") + } memInByte, err := util.ParseMemStringAsByte(arg.val) if err != nil { return nil, fmt.Errorf("invalid argument: %s value '%s' in script: %w", arg.name, arg.val, err) } task.ReqResources.AllocatableRes.MemoryLimitBytes = memInByte task.ReqResources.AllocatableRes.MemorySwLimitBytes = memInByte case "--mem-per-cpu": + if task.ReqResources.GetAllocatableRes().GetMemoryLimitBytes() != 0 { + return nil, fmt.Errorf("invalid argument: --mem-per-cpu is mutually exclusive with --mem (in script)") + } memInBytePerCpu, err := util.ParseMemStringAsByte(arg.val) if err != nil { - log.Errorf("Invalid argument: %v in script: %v", arg.name, err) - return false, nil + return nil, fmt.Errorf("invalid argument: %s value '%s' in script: %w", arg.name, arg.val, err) } task.MemPerCpu = &memInBytePerCpuprotos/PublicDefs.proto (1)
175-184: Ensure protoc ≥ 3.15.0 and protoc-gen-go ≥ v1.22.0 before merging.
Addingoptional uint64 mem_per_cpu = 42requires minimum protoc 3.15.0 (or 3.12–3.14 with--experimental_allow_proto3_optional) and protoc-gen-go v1.22.0. In mixed-version deployments, older consumers will deserialize the field value correctly but cannot access presence semantics (has_xxx()), while older protoc versions will reject the optional keyword entirely during compilation. Verify all producer and consumer toolchains meet these minimums.
🧹 Nitpick comments (3)
internal/cbatch/cmd.go (1)
53-54: PreferStringVar(no shorthand) for--mem-per-cputo avoidStringVarP(..., "", "")ambiguity.
Current wiring works, butStringVar(&FlagMemPerCpu, "mem-per-cpu", "", "...")reads cleaner and avoids relying on how empty shorthands are handled. Also good call addingMarkFlagsMutuallyExclusive("mem","mem-per-cpu").Also applies to: 159-160
internal/crun/cmd.go (1)
122-124:--mem-per-cpuflag + mutual exclusion looks correct; considerStringVarfor readability.
Same as cbatch:StringVar(no shorthand) is clearer thanStringVarP(..., "", "").internal/calloc/cmd.go (1)
44-45: Flag wiring is fine; considerStringVar(no shorthand) for--mem-per-cpu.
UsingStringVar(&FlagMemPerCpu, "mem-per-cpu", "", "...")is clearer thanStringVarP(..., "", ""). Mutual exclusion with--memis a good addition.Also applies to: 107-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/calloc/calloc.go(1 hunks)internal/calloc/cmd.go(2 hunks)internal/cbatch/cbatch.go(2 hunks)internal/cbatch/cmd.go(2 hunks)internal/crun/cmd.go(2 hunks)internal/crun/crun.go(1 hunks)protos/PublicDefs.proto(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/crun/crun.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NamelessOIer
Repo: PKUHPC/CraneSched-FrontEnd PR: 256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.156Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
📚 Learning: 2025-04-24T16:05:47.156Z
Learnt from: NamelessOIer
Repo: PKUHPC/CraneSched-FrontEnd PR: 256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.156Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
Applied to files:
internal/calloc/cmd.gointernal/cbatch/cmd.gointernal/crun/cmd.gointernal/cbatch/cbatch.gointernal/calloc/calloc.go
🧬 Code graph analysis (4)
internal/calloc/cmd.go (1)
internal/crun/cmd.go (1)
FlagMemPerCpu(47-47)
internal/cbatch/cmd.go (5)
internal/calloc/cmd.go (2)
RootCmd(60-71)FlagMemPerCpu(44-44)internal/crun/cmd.go (2)
RootCmd(67-77)FlagMemPerCpu(47-47)internal/cqueue/cmd.go (1)
RootCmd(48-69)internal/ccon/cmd.go (1)
RootCmd(41-60)internal/cinfo/cmd.go (1)
RootCmd(41-57)
internal/cbatch/cbatch.go (4)
internal/util/string.go (1)
ParseMemStringAsByte(82-104)internal/calloc/cmd.go (1)
FlagMemPerCpu(44-44)internal/cbatch/cmd.go (1)
FlagMemPerCpu(53-53)internal/crun/cmd.go (1)
FlagMemPerCpu(47-47)
internal/calloc/calloc.go (5)
internal/calloc/cmd.go (1)
FlagMemPerCpu(44-44)internal/cbatch/cmd.go (1)
FlagMemPerCpu(53-53)internal/crun/cmd.go (1)
FlagMemPerCpu(47-47)internal/util/string.go (1)
ParseMemStringAsByte(82-104)internal/util/err.go (1)
ErrorCmdArg(36-36)
🪛 GitHub Actions: Go Code Quality Check
internal/crun/cmd.go
[error] 1-1: gofmt formatting check failed. The following files are not properly formatted. Run 'gofmt -w .' to fix the formatting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/crun/cmd.go (2)
44-47: RelocateFlagMemPerCputo the resource-flags group for better organization.
FlagMemPerCpuis semantically a memory resource flag (likeFlagMemat line 32) but is currently placed in the input/pty/exclusive group. For maintainability and clarity, move it to the first var block section (lines 27-42) immediately afterFlagMem.Apply this diff to relocate the flag:
FlagTime string FlagMem string + FlagMemPerCpu string FlagPartition string FlagJob stringAnd remove it from the current location:
FlagInput string FlagPty bool FlagExclusive bool - FlagMemPerCpu string FlagX11 bool
121-121: UseStringVarinstead ofStringVarPfor consistency withFlagMem.Since
--mem-per-cpuhas no short form, useStringVar(likeFlagMemat line 94) instead ofStringVarPwith an empty short form.Apply this diff:
- RootCmd.Flags().StringVarP(&FlagMemPerCpu, "mem-per-cpu", "", "", "Maximum amount of real memory per CPU, support GB(G, g), MB(M, m), KB(K, k) and Bytes(B), default unit is MB") + RootCmd.Flags().StringVar(&FlagMemPerCpu, "mem-per-cpu", "", "Maximum amount of real memory per CPU, support GB(G, g), MB(M, m), KB(K, k) and Bytes(B), default unit is MB")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/calloc/calloc.go(1 hunks)internal/cbatch/cbatch.go(2 hunks)internal/crun/cmd.go(2 hunks)internal/crun/crun.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/calloc/calloc.go
- internal/crun/crun.go
- internal/cbatch/cbatch.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T16:05:47.156Z
Learnt from: NamelessOIer
Repo: PKUHPC/CraneSched-FrontEnd PR: 256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.156Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
Applied to files:
internal/crun/cmd.go
🧬 Code graph analysis (1)
internal/crun/cmd.go (1)
internal/cbatch/cmd.go (1)
FlagMemPerCpu(53-53)
🔇 Additional comments (1)
internal/crun/cmd.go (1)
122-122: LGTM! Mutual exclusivity correctly enforced.The mutual exclusivity between
--memand--mem-per-cpuis properly implemented, preventing users from specifying both flags simultaneously.
L-Xiafeng
force-pushed
the
dev/mempercpu
branch
from
2100db7 to
c4e9268
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/crun/cmd.go (1)
47-51: Formatting issue already flagged in previous review.The var block alignment and spacing issue has already been identified in a previous review comment. Please run
gofmt -wto resolve the formatting errors.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/calloc/calloc.gointernal/calloc/cmd.gointernal/cbatch/cbatch.gointernal/cbatch/cmd.gointernal/crun/cmd.gointernal/crun/crun.goprotos/PublicDefs.proto
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cbatch/cmd.go
- internal/calloc/calloc.go
- internal/cbatch/cbatch.go
- internal/calloc/cmd.go
- protos/PublicDefs.proto
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-24T16:05:47.156Z
Learnt from: NamelessOIer
Repo: PKUHPC/CraneSched-FrontEnd PR: 256
File: internal/cqueue/cqueue.go:350-356
Timestamp: 2025-04-24T16:05:47.156Z
Learning: In cqueue.go, the function ProcessAllocMemPerNode should use task.AllocatedResView.AllocatableRes.MemoryLimitBytes (allocated resources) rather than task.ReqResView.AllocatableRes.MemoryLimitBytes (requested resources) to correctly display allocated memory per node.
Applied to files:
internal/crun/crun.gointernal/crun/cmd.go
🧬 Code graph analysis (1)
internal/crun/crun.go (5)
internal/crun/cmd.go (1)
FlagMemPerCpu(50-50)internal/calloc/cmd.go (1)
FlagMemPerCpu(44-44)internal/cbatch/cmd.go (1)
FlagMemPerCpu(53-53)internal/util/string.go (1)
ParseMemStringAsByte(82-104)internal/util/err.go (2)
NewCraneErr(60-65)ErrorCmdArg(36-36)
🪛 GitHub Actions: Go Code Quality Check
internal/crun/crun.go
[error] 1186-1186: undefined: task
🪛 GitHub Check: go-check
internal/crun/crun.go
[failure] 1186-1186:
undefined: task
🔇 Additional comments (1)
internal/crun/cmd.go (1)
124-125: Flag definition and mutual exclusivity correctly implemented.The
--mem-per-cpuflag is properly defined with appropriate help text, and the mutual exclusivity with the--memflag is correctly enforced usingMarkFlagsMutuallyExclusive.
| if FlagMemPerCpu != "" { | ||
| memInBytePerCpu, err := util.ParseMemStringAsByte(FlagMemPerCpu) | ||
| if err != nil { | ||
| return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: %s.", err)) | ||
| } | ||
| task.MemPerCpu = &memInBytePerCpu | ||
| } |
There was a problem hiding this comment.
Fix the critical compilation error: undefined variable task.
The code references task.MemPerCpu on line 1186, but the variable task is not defined in this scope. This causes the compilation failure reported by the pipeline. Following the pattern established for FlagMem handling (lines 1161-1180), you need to distinguish between jobMode and stepMode and set the appropriate field.
🔎 Proposed fix
if FlagMemPerCpu != "" {
memInBytePerCpu, err := util.ParseMemStringAsByte(FlagMemPerCpu)
if err != nil {
return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: %s.", err))
}
- task.MemPerCpu = &memInBytePerCpu
+ if jobMode {
+ job.MemPerCpu = &memInBytePerCpu
+ } else {
+ step.MemPerCpu = &memInBytePerCpu
+ }
}📝 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 FlagMemPerCpu != "" { | |
| memInBytePerCpu, err := util.ParseMemStringAsByte(FlagMemPerCpu) | |
| if err != nil { | |
| return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: %s.", err)) | |
| } | |
| task.MemPerCpu = &memInBytePerCpu | |
| } | |
| if FlagMemPerCpu != "" { | |
| memInBytePerCpu, err := util.ParseMemStringAsByte(FlagMemPerCpu) | |
| if err != nil { | |
| return util.NewCraneErr(util.ErrorCmdArg, fmt.Sprintf("Invalid argument: %s.", err)) | |
| } | |
| if jobMode { | |
| job.MemPerCpu = &memInBytePerCpu | |
| } else { | |
| step.MemPerCpu = &memInBytePerCpu | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Go Code Quality Check
[error] 1186-1186: undefined: task
🪛 GitHub Check: go-check
[failure] 1186-1186:
undefined: task
🤖 Prompt for AI Agents
In internal/crun/crun.go around lines 1181 to 1187, the code references an
undefined variable `task` when setting MemPerCpu; mirror the FlagMem handling:
check jobMode vs stepMode and assign the parsed memInBytePerCpu to the correct
struct field (Job.MemPerCpu when jobMode is true, Step.MemPerCpu when jobMode is
false), using a pointer to the parsed value and returning the same error type on
parse failure; ensure the variable names and pointer assignment follow the
FlagMem pattern so compilation succeeds.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.