fix(cli): reject unparseable numeric flags in benchmark and chat - #161
Open
addyCooks wants to merge 1 commit into
Open
fix(cli): reject unparseable numeric flags in benchmark and chat#161addyCooks wants to merge 1 commit into
addyCooks wants to merge 1 commit into
Conversation
`nanotune train` validates its numeric flags before it uses them, but `benchmark` and `chat` parsed the identical class of flag with a bare Number.parseInt/parseFloat and no check at all. parseInt stops at the first character it cannot parse, so `--ctx-size 4096x` silently became 4096 and the run proceeded under a value the user never typed. Anything that failed outright became NaN and reached llama-server unchecked: String(NaN) is "NaN", so `--gpu-layers abc` spawned the real argument `-ngl NaN`, while JSON.stringify(NaN) is null, so a bad `--max-tokens`/`--temperature`/`--top-p` left the field out of the completion body and llama-server quietly used its own default. `--timeout` was worse still — setTimeout(fn, NaN) fires immediately, so a typo aborted every test the instant it started. Every numeric flag on both commands now goes through one shared parseNumericFlag() in chat-helpers.ts, which uses Number() rather than parseInt/parseFloat so trailing garbage fails instead of truncating, and rejects a fractional value where llama-server wants an integer. A rejected flag is reported as "Invalid value for --flag", matching train's existing message, and the command stops before any subprocess or network work: for chat ahead of the project check, for benchmark alongside the sampling flags, ahead of the base-model download and the server spawn. benchmark's flag wiring moves into resolveBenchmarkFlags() in benchmark-utils.ts, next to the sampling flags it already resolved there, which also drops the duplicate copy of chat's server-option builder and lets the whole path be unit-tested. Flags a `--preset` overrides are still parsed, so a typo alongside a preset is reported rather than silently discarded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
nanotune trainvalidates its numeric flags before it uses them, butbenchmarkandchatparsed the identical class of flag with a bareNumber.parseInt/parseFloatand no check at all.parseIntstops at the first character it cannot parse, so--ctx-size 4096xsilently became 4096 and the run proceeded under a value the user never typed. Anything that failed outright becameNaNand reached llama-server unchecked:String(NaN)is"NaN", so--gpu-layers abcspawned the real argument-ngl NaN, whileJSON.stringify(NaN)isnull, so a bad--max-tokens/--temperature/--top-pleft the field out of the completion body and llama-server quietly used its own default.--timeoutwas worse stillsetTimeout(fn, NaN)fires immediately, so a typo aborted every test the instant it started.Every numeric flag on both commands now goes through one shared
parseNumericFlag()inchat-helpers.ts, which usesNumber()rather thanparseInt/parseFloatso trailing garbage fails instead of truncating, and rejects a fractional value where llama-server wants an integer. A rejected flag is reported asInvalid value for --flag, matching train's existing message, and the command stops before any subprocess or network work: for chat ahead of the project check, for benchmark alongside the sampling flags, ahead of the base-model download and the server spawn.benchmark's flag wiring moves into
resolveBenchmarkFlags()inbenchmark-utils.ts, next to the sampling flags it already resolved there, which also drops the duplicate copy of chat's server-option builder and lets the whole path be unit-tested. Flags a--presetoverrides are still parsed, so a typo alongside a preset is reported rather than silently discarded.Closes #134.
Behavior changes
--ctx-size 4096.5is now rejected rather than truncated to 4096.--presetnow errors instead of being ignored (the preset still wins on values).Type of Change
Testing
Automated Tests
pnpm test:allcompletes successfully)Manual Testing
nanotune initnanotune datacommands (add/import/list/validate)nanotune trainnanotune exportnanotune benchmarkChecklist
pnpm format)