feat: Add not support args message #378
Conversation
WalkthroughAdds many placeholder (not-implemented) exported CLI flag variables across calloc, cbatch, and crun; introduces filtering of unsupported cbatch script args and helper functions that print "not yet supported" messages for Slurm wrapper commands. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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)
🧰 Additional context used
🧬 Code graph analysis (6)
internal/calloc/calloc.go (3)
internal/calloc/cmd.go (6)
FlagNTasks(59-59)FlagDependency(60-60)FlagMemPerCpu(61-61)FlagNoKill(62-62)FlagQuiet(63-63)FlagVerbose(64-64)internal/cbatch/cmd.go (3)
FlagNTasks(70-70)FlagDependency(79-79)FlagMemPerCpu(83-83)internal/crun/cmd.go (3)
FlagNTasks(65-65)FlagDependency(72-72)FlagVerbose(73-73)
internal/crun/crun.go (1)
internal/crun/cmd.go (14)
FlagNTasks(65-65)FlagMultiProg(66-66)FlagOversubscribe(67-67)FlagCpuBind(68-68)FlagDeadline(69-69)FlagWait(70-70)FlagMpi(71-71)FlagDependency(72-72)FlagVerbose(73-73)FlagError(74-74)FlagKillOnBadExit(75-75)FlagExtraNodeInfo(76-76)FlagNTasksPerCore(77-77)FlagConstraint(78-78)
internal/cbatch/cmd.go (7)
internal/calloc/cmd.go (4)
RootCmd(66-77)FlagNTasks(59-59)FlagDependency(60-60)FlagMemPerCpu(61-61)internal/crun/cmd.go (4)
RootCmd(80-90)FlagNTasks(65-65)FlagDependency(72-72)FlagWait(70-70)internal/cqueue/cmd.go (1)
RootCmd(48-69)internal/cinfo/cmd.go (1)
RootCmd(41-57)internal/ccon/flags.go (1)
Flags(109-121)internal/ccontrol/cmd.go (1)
FlagPriority(42-42)internal/cacctmgr/cmd.go (1)
FlagPriority(86-86)
internal/cbatch/cbatch.go (1)
internal/cbatch/cmd.go (20)
FlagNTasks(70-70)FlagArray(71-71)FlagNoRequeue(72-72)FlagParsable(73-73)FlagGpusPerNode(74-74)FlagNTasksPerSocket(75-75)FlagWckey(77-77)FlagCpuFreq(78-78)FlagDependency(79-79)FlagTasks(80-80)FlagTasksPerNode(81-81)FlagPriority(82-82)FlagMemPerCpu(83-83)FlagThreadsPerCore(84-84)FlagDistribution(85-85)FlagInput(86-86)FlagSocketsPerNode(87-87)FlagCoresPerSocket(88-88)FlagRequeue(89-89)FlagWait(90-90)
internal/calloc/cmd.go (2)
internal/crun/cmd.go (3)
FlagNTasks(65-65)FlagDependency(72-72)FlagVerbose(73-73)internal/ccon/flags.go (1)
Flags(109-121)
internal/crun/cmd.go (1)
internal/calloc/cmd.go (3)
FlagNTasks(59-59)FlagDependency(60-60)FlagVerbose(64-64)
Nativu5
left a comment
Nativu5
left a comment
There was a problem hiding this comment.
最好打印到 stderr，这样不容易破坏用户逻辑
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
internal/cbatch/cbatch.go (1)
361-417: LGTM—stderr output is correct.The warnings are correctly emitted to stderr, addressing the past review comment. This ensures machine-parseable stdout (job IDs, JSON) remains uncontaminated.
internal/cbatch/cmd.go (2)
69-89: Several flag types should be bool instead of string.Flags like
FlagNoRequeue,FlagParsable,FlagRequeue, andFlagWait(lines 72, 73, 88, 89) are typically boolean in Slurm but are declared asstring. This requires changing both the declaration here and the registration ininternal/cwrapper/slurm.golines 314-333.See the review comment on
internal/cwrapper/slurm.golines 314-333 for the complete fix.
69-89:FlagSignalis declared but never registered.
FlagSignal(line 76) is declared but not registered ininternal/cwrapper/slurm.golines 314-333, so it won't be parsed from the command line.Add registration in
internal/cwrapper/slurm.gosbatch function:cmd.Flags().StringVar(&cbatch.FlagNTasksPerSocket, "ntasks-per-socket", "", "") +cmd.Flags().StringVar(&cbatch.FlagSignal, "signal", "", "Not yet supported by Crane (ignored).") cmd.Flags().StringVar(&cbatch.FlagWckey, "wckey", "", "")And add warning emission in
internal/cbatch/cbatch.goaround line 417:if FlagWait != "" { fmt.Fprintln(os.Stderr, "The feature --wait is not yet supported by Crane, the use is ignored.") } +if FlagSignal != "" { + fmt.Fprintln(os.Stderr, "The feature --signal is not yet supported by Crane, the use is ignored.") +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cbatch/cbatch.go(2 hunks)internal/cbatch/cmd.go(1 hunks)internal/cwrapper/slurm.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/cbatch/cbatch.go (2)
internal/cbatch/cmd.go (19)
FlagNTasks(70-70)FlagArray(71-71)FlagNoRequeue(72-72)FlagParsable(73-73)FlagGpusPerNode(74-74)FlagNTasksPerSocket(75-75)FlagWckey(77-77)FlagCpuFreq(78-78)FlagDependency(79-79)FlagTasksPerNode(80-80)FlagPriority(81-81)FlagMemPerCpu(82-82)FlagThreadsPerCore(83-83)FlagDistribution(84-84)FlagInput(85-85)FlagSocketsPerNode(86-86)FlagCoresPerSocket(87-87)FlagRequeue(88-88)FlagWait(89-89)internal/ccontrol/cmd.go (1)
FlagPriority(42-42)
internal/cwrapper/slurm.go (3)
internal/cbatch/cmd.go (19)
FlagNTasks(70-70)FlagDependency(79-79)FlagMemPerCpu(82-82)FlagArray(71-71)FlagNoRequeue(72-72)FlagParsable(73-73)FlagGpusPerNode(74-74)FlagNTasksPerSocket(75-75)FlagWckey(77-77)FlagCpuFreq(78-78)FlagTasksPerNode(80-80)FlagPriority(81-81)FlagThreadsPerCore(83-83)FlagDistribution(84-84)FlagInput(85-85)FlagSocketsPerNode(86-86)FlagCoresPerSocket(87-87)FlagRequeue(88-88)FlagWait(89-89)internal/crun/cmd.go (15)
FlagNTasks(65-65)FlagDependency(72-72)FlagVerbose(73-73)FlagInput(44-44)FlagWait(70-70)FlagMultiProg(66-66)FlagOversubscribe(67-67)FlagCpuBind(68-68)FlagDeadline(69-69)FlagMpi(71-71)FlagError(74-74)FlagKillOnBadExit(75-75)FlagExtraNodeInfo(76-76)FlagNTasksPerCore(77-77)FlagConstraint(78-78)internal/calloc/cmd.go (6)
FlagNTasks(59-59)FlagDependency(60-60)FlagMemPerCpu(61-61)FlagNoKill(62-62)FlagQuiet(63-63)FlagVerbose(64-64)
🔇 Additional comments (2)
internal/cwrapper/slurm.go (2)
236-242: Verify that warnings are emitted when these flags are used.Ensure that
callocemits warnings to stderr when these not-yet-supported flags are provided, similar to howcbatchhandles them in lines 361-417 ofinternal/cbatch/cbatch.go.#!/bin/bash # Check if calloc.go emits warnings for not-supported flags rg -nP --type=go -C5 'FlagNTasks|FlagDependency|FlagMemPerCpu|FlagNoKill|FlagQuiet|FlagVerbose' internal/calloc/
709-723: Warnings are correctly emitted for all unsupported flags.Verification confirms that
crunproperly emits warnings to stderr when these flags are provided. All 14 flags (ntasks, multi-prog, oversubscribe, cpu-bind, deadline, wait, mpi, dependency, verbose, error, kill-on-bad-exit, extra-node-info, ntasks-per-core, and constraint) have corresponding warning checks ininternal/crun/crun.go(lines 1118–1159) that output"The feature --X is not yet supported by Crane, the use is ignored."The implementation matches the cbatch pattern and properly handles these not-yet-supported flags.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/cwrapper/slurm.go (3)
238-244: Add descriptive help text and fix flag types for boolean options.These flags still have empty help text, and
--no-kill,--quiet, and--verboseare boolean flags in Slurm but registered asStringVar. This issue was previously flagged but remains unresolved.The current implementation forces users to pass values like
--quiet=trueinstead of just--quiet, and makes the help output useless.Please refer to the previous review comment for the recommended fix pattern. Note that changing to
BoolVarrequires updating the variable types ininternal/calloc/cmd.gofromstringtobool.
316-334: Add descriptive help text and fix flag types for boolean options.These flags have empty help text, and
--no-requeue,--parsable,--requeue, and--waitare boolean flags but registered asStringVar. This issue was previously identified but not addressed.Refer to the previous review comment for the fix pattern. Changing to
BoolVarrequires updating variable types ininternal/cbatch/cmd.go.
710-724: Add descriptive help text and fix flag types for boolean options.Empty help text and incorrect types for boolean flags (
--oversubscribe,--verbose,--kill-on-bad-exit,--extra-node-info,--wait) were previously reported but remain unresolved.See the earlier review comment for guidance. Variable types in
internal/crun/cmd.gomust be updated accordingly.
🧹 Nitpick comments (2)
internal/cwrapper/slurm.go (2)
741-784: Update logic when boolean flags are fixed.This function currently checks string non-emptiness (
!= ""), but when the flags are corrected to useBoolVar(as flagged in the earlier comments), these conditions must change to simply check the boolean value.Example change needed after flag type fixes:
- if crun.FlagVerbose != "" { + if crun.FlagVerbose {Additionally, the three message-printing functions share substantial code duplication. Consider extracting a helper function that accepts a map of flag-value pairs and their corresponding messages.
786-805: Update logic when boolean flags are fixed.When the flag types are changed to
BoolVar, update the conditions from string checks to boolean checks.Example:
- if calloc.FlagQuiet != "" { + if calloc.FlagQuiet {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cbatch/cbatch.go(1 hunks)internal/cwrapper/slurm.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cbatch/cbatch.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cwrapper/slurm.go (3)
internal/cbatch/cmd.go (18)
FlagNTasks(70-70)FlagDependency(79-79)FlagMemPerCpu(81-81)FlagArray(71-71)FlagNoRequeue(72-72)FlagParsable(73-73)FlagGpusPerNode(74-74)FlagNTasksPerSocket(75-75)FlagWckey(77-77)FlagCpuFreq(78-78)FlagPriority(80-80)FlagThreadsPerCore(82-82)FlagDistribution(83-83)FlagInput(84-84)FlagSocketsPerNode(85-85)FlagCoresPerSocket(86-86)FlagRequeue(87-87)FlagWait(88-88)internal/crun/cmd.go (15)
FlagNTasks(65-65)FlagDependency(72-72)FlagVerbose(73-73)FlagInput(44-44)FlagWait(70-70)FlagMultiProg(66-66)FlagOversubscribe(67-67)FlagCpuBind(68-68)FlagDeadline(69-69)FlagMpi(71-71)FlagError(74-74)FlagKillOnBadExit(75-75)FlagExtraNodeInfo(76-76)FlagNTasksPerCore(77-77)FlagConstraint(78-78)internal/calloc/cmd.go (6)
FlagNTasks(59-59)FlagDependency(60-60)FlagMemPerCpu(61-61)FlagNoKill(62-62)FlagQuiet(63-63)FlagVerbose(64-64)
🔇 Additional comments (2)
internal/cwrapper/slurm.go (2)
227-228: Placement of message function is appropriate.The call to
PrintSallocIgnoreDummyArgsMessage()is correctly positioned after flag parsing and before command execution, ensuring users see the warnings at the right time.
700-700: LGTM!The placement of
PrintSrunIgnoreDummyArgsMessage()is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cbatch/cbatch.go(1 hunks)internal/cwrapper/slurm.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cwrapper/slurm.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cbatch/cbatch.go (1)
internal/util/err.go (2)
WrapCraneErr(67-73)ErrorCmdArg(36-36)
|
按照 AI 的意见修改下，以及能不能把这些参数隐藏到 Wrapper 里，不要让用户在使用鹤思自己的命令的时候产生困惑？ |
Nativu5
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/cwrapper/slurm.go (3)
238-244: Add descriptive help text and fix flag types for boolean options.These issues were previously flagged but remain unaddressed:
- Empty help text makes
salloc --helpoutput useless for these flags--no-kill,--quiet, and--verboseare boolean flags in Slurm but registered asStringVar, requiring values like--quiet=trueinstead of just--quietAs noted in the PR comments, consider whether these parameters should be hidden entirely to avoid user confusion when using CraneSched's own commands.
Based on past review feedback.
316-334: Fix critical flag shorthand syntax and address previously flagged issues.Critical issue found:
- Lines 325 and 334: Flag shorthands
"-d"and"-W"should be"d"and"W"respectively. Cobra'sStringVarPexpects the shorthand without the leading dash. This will cause flag parsing to fail.Additionally, these previously flagged issues remain unaddressed:
- Empty help text makes
sbatch --helpoutput not useful--no-requeue,--parsable,--requeue, and--waitshould be boolean flags usingBoolVar, notStringVar🔎 Proposed fix for flag shorthand syntax
- cmd.Flags().StringVarP(&cbatch.FlagDependency, "dependency", "-d", "", "") + cmd.Flags().StringVarP(&cbatch.FlagDependency, "dependency", "d", "", "") - cmd.Flags().StringVarP(&cbatch.FlagWait, "wait", "-W", "", "") + cmd.Flags().StringVarP(&cbatch.FlagWait, "wait", "W", "", "")Based on past review feedback for the duplicate issues.
710-724: Add descriptive help text and fix flag types for boolean options.These previously flagged issues remain unaddressed:
- Empty help text makes
srun --helpoutput not useful--oversubscribe,--verbose,--kill-on-bad-exit,--extra-node-info, and--waitshould be boolean flags usingBoolVar, notStringVarAs suggested in the PR comments, consider hiding these parameters to prevent confusion.
Based on past review feedback.
🧹 Nitpick comments (3)
internal/cwrapper/slurm.go (3)
741-784: Consider refactoring for maintainability and type safety.The function correctly prints warnings to stderr, but has maintainability concerns:
- The repetitive structure (14 identical if-blocks) violates DRY principles
- The string comparison
!= ""works for current string types but will break if flags are changed to boolean (as recommended in other comments)- The function is exported but only used within this package
🔎 Suggested refactor for better maintainability
After fixing flag types, consider a table-driven approach:
func PrintSrunIgnoreDummyArgsMessage() { type flagCheck struct { value *string name string } checks := []flagCheck{ {&crun.FlagNTasks, "--ntasks/-n"}, {&crun.FlagMultiProg, "--multi-prog"}, // ... etc } for _, check := range checks { if *check.value != "" { fmt.Fprintf(os.Stderr, "The feature %s is not yet supported by Crane, the use is ignored.\n", check.name) } } }For boolean flags, use separate handling or a unified approach with interface{}.
786-805: Similar refactoring opportunity as PrintSrunIgnoreDummyArgsMessage.This function has the same maintainability concerns noted for
PrintSrunIgnoreDummyArgsMessage(lines 741-784): repetitive structure and type safety issues when flags are converted to boolean.
807-862: Consider consolidating all three Print functions.This function (and the two similar ones at lines 741-784 and 786-805) follow an identical pattern. Consider consolidating them into a single parameterized function to eliminate code duplication across ~120 lines.
🔎 Suggested approach for consolidation
Create a generic helper:
func printIgnoredFlags(flags map[string]string) { for name, value := range flags { if value != "" { fmt.Fprintf(os.Stderr, "The feature %s is not yet supported by Crane, the use is ignored.\n", name) } } } func PrintSbatchIgnoreArgsMessage() { printIgnoredFlags(map[string]string{ "--ntasks/-n": cbatch.FlagNTasks, "--array/-a": cbatch.FlagArray, // ... etc }) }This reduces duplication and makes adding new flags simpler.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/cbatch/cbatch.gointernal/cwrapper/slurm.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/cbatch/cbatch.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/cwrapper/slurm.go (3)
internal/calloc/cmd.go (6)
FlagNTasks(59-59)FlagDependency(60-60)FlagMemPerCpu(61-61)FlagNoKill(62-62)FlagQuiet(63-63)FlagVerbose(64-64)internal/cbatch/cmd.go (18)
FlagNTasks(70-70)FlagDependency(79-79)FlagMemPerCpu(81-81)FlagArray(71-71)FlagNoRequeue(72-72)FlagParsable(73-73)FlagGpusPerNode(74-74)FlagNTasksPerSocket(75-75)FlagWckey(77-77)FlagCpuFreq(78-78)FlagPriority(80-80)FlagThreadsPerCore(82-82)FlagDistribution(83-83)FlagInput(84-84)FlagSocketsPerNode(85-85)FlagCoresPerSocket(86-86)FlagRequeue(87-87)FlagWait(88-88)internal/crun/cmd.go (15)
FlagNTasks(65-65)FlagDependency(72-72)FlagVerbose(73-73)FlagInput(44-44)FlagWait(70-70)FlagMultiProg(66-66)FlagOversubscribe(67-67)FlagCpuBind(68-68)FlagDeadline(69-69)FlagMpi(71-71)FlagError(74-74)FlagKillOnBadExit(75-75)FlagExtraNodeInfo(76-76)FlagNTasksPerCore(77-77)FlagConstraint(78-78)
🔇 Additional comments (3)
internal/cwrapper/slurm.go (3)
227-228: Appropriate placement for user-facing warning.The call to print ignored args is correctly placed early in the execution path, before validation and the actual command execution. This ensures users see the warning message immediately.
307-307: Function placement is correct.The call to print ignored args is appropriately placed before command execution. The function name spelling is correct.
700-700: Appropriate placement for user warning.The call is correctly placed to warn users early about ignored flags.
2 participants
Summary by CodeRabbit
Chores
User-facing changes
✏️ Tip: You can customize this high-level summary in your review settings.