Replace various key=value string parsers with a single utility#569
Merged
Replace various key=value string parsers with a single utility#569
Conversation
2cb80bd to
5d724ce
Compare
f21b3f0 to
55745f2
Compare
Signed-off-by: Samuel Monson <smonson@redhat.com>
Signed-off-by: Samuel Monson <smonson@redhat.com>
Signed-off-by: Samuel Monson <smonson@redhat.com>
Signed-off-by: Samuel Monson <smonson@redhat.com>
dbutenhof
reviewed
May 5, 2026
Collaborator
dbutenhof
left a comment
There was a problem hiding this comment.
Minor comments from a first pass without getting deep into the algorithms ... but it looks good.
Signed-off-by: Samuel Monson <smonson@redhat.com>
Signed-off-by: Samuel Monson <smonson@redhat.com>
dbutenhof
previously approved these changes
May 5, 2026
Signed-off-by: Samuel Monson <smonson@redhat.com>
jaredoconnell
previously approved these changes
May 6, 2026
Collaborator
jaredoconnell
left a comment
There was a problem hiding this comment.
Looks good to me. And basic data args are working for me.
Collaborator
|
A test is failing. It looks like maybe you changed the error message under that condition. |
Signed-off-by: Samuel Monson <smonson@redhat.com>
dbutenhof
approved these changes
May 6, 2026
Collaborator
dbutenhof
left a comment
There was a problem hiding this comment.
This looks great. Minor issue: you added the external get/set methods in the arg string parser class, which aren't used, and aren't tested. If we're going to have them, they should be tested.
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.
Summary
Replaces various config string handlers with a new
ArgStringformat.Details
Replaces the dataset config parser with a more advance "arg_string" parser with support for nested and list config structures. For example:
Renders to
{ "prefix_buckets": [ {"bucket_weight": 10, "prefix_count": 10}, {"bucket_weight": 20, "prefix_count": 4}, ] }Also added some glue logic to allow this format anywhere on the CLI that supports JSON. This code should be considered temporary as it will mostly be replaced through the rest of the CLI refactor.
Also open to suggestions for a better name then "arg_string"; a 2-4 letter acronym like JSON or YAML would be good.
Test Plan
The following benchmark should run without errors and have ~384 ISL per request (make sure to check startup logs confirm
backend-kwargsandwarmupare configured):guidellm benchmark run \ --profile concurrent \ --data "prompt_tokens=256,output_tokens=256,prefix_buckets[0].bucket_weight=10,prefix_buckets[0].prefix_tokens=128,prefix_buckets[0].prefix_count=10" \ --request-format /v1/completions \ --backend-kwargs '{"timeout":120}' \ --warmup percent=0.1,value=20 \ --max-seconds 30 \ --rate 30Use of AI
## WRITTEN BY AI ##)