fix(cli): accept negative float flag values in space form (EAI-8243) - #306
Open
r0x0r wants to merge 1 commit into
Open
fix(cli): accept negative float flag values in space form (EAI-8243)#306r0x0r wants to merge 1 commit into
r0x0r wants to merge 1 commit into
Conversation
The space form of negative-number float flags (e.g. `serve --temperature -1`) was rejected by clap with a confusing "unexpected argument '-1'" error, because clap parsed the leading-dash token as a flag rather than a value. Only the equals form (`--temperature=-1`) reached the range validator that reports the valid range. Enable `allow_negative_numbers` on the affected float flags (`--temperature` and `--top-p` on both `chat` and `serve`) so the space form reaches the value parser and both forms validate identically, surfacing a clear range-validation message instead of an unexpected argument error. Add regression tests covering both the space and equals forms for `--temperature` and `--top-p` on `chat` and `serve`. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com>
r0x0r
changed the title
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
1 participant
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.
Summary
rocm serve --temperature -1(space-separated form) was rejected by clap with the confusing errorunexpected argument '-1', because clap parsed the leading-dash token as a flag rather than a value. Only the equals form (--temperature=-1) reached the range validator that reports the valid range.This is the classic negative-number-as-flag gotcha, and it affected all float flags that accept a value — not just
--temperature. It applied to--temperatureand--top-pon bothchatandserve.Fix
Enable clap's
allow_negative_numberson the affected float flags so the space form reaches the value parser. Both forms now validate identically and surface a clear range-validation message instead of an unexpected-argument error.allow_negative_numbersis preferred overallow_hyphen_valueshere: it only accepts leading-dash tokens that parse as numbers, so it does not swallow the next flag when a value is missing (the tradeoff called out for--gpu-memory-utilization).Testing
--temperature -1,--top-p -0.5) and equals form (--temperature=-1,--top-p=-0.5) onchatandserve, asserting they reachValueValidation(the range validator) rather than being rejected as an unexpected argument.cargo test -p rocm --bin rocm(touched sampling tests pass)cargo clippy -p rocm --all-targets -- -D warningscargo fmt -p rocmBehavior is CLI argument parsing only; no GPU hardware is required to exercise it. The new unit tests assert the user-observable error kind for both flag forms.