-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
Fix boolean nested params, add dict format support, and enhance plotting for vllm bench sweep #29025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Fix boolean nested dot-notation params to use =true/false syntax - Fix nested dict parameter values to be JSON serialized - Add support for dict format in --bench-params and --serve-params - Use name field from dict format as benchmark name Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
The 'name' field is used for identification in dict format but should not be passed as a command-line parameter. Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
ProExpertProg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@copilot also use |
- Converted boolean and string/numeric tests to parametrized tests - Simplified dict comprehension in read_from_dict - Extracted normalize_cmd_kv_pair method - Added name() method to ParameterSweepItem - Updated serve.py to use name() method Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
|
@copilot also use _benchmark_name to store the name instead of name, and assert they are all unique (if provided via the list of records format). |
- Changed field name from 'name' to '_benchmark_name' throughout codebase - Added validation to ensure all _benchmark_name values are unique - Updated name() method to use _benchmark_name - Updated apply_to_cmd() and as_text() to skip _benchmark_name - Added comprehensive tests for uniqueness validation - Updated all existing tests to use _benchmark_name Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
Done in cb81309. Changed to use |
ProExpertProg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Changed test_nested_dict_value to use compilation_config instead of env - Made it parametrized with multiple field combinations - Tests cudagraph_mode (full/piecewise), mode (2/3), use_inductor_graph_partition (True/False) Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
Signed-off-by: Luka Govedič <luka.govedic@gmail.com>
|
@copilot add tests for filtering including the "inf" string edge case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ProExpertProg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }) | ||
|
|
||
| # DataFrame with "inf" as string (edge case) | ||
| self.df_inf_string = pd.DataFrame({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove the string test cases as we now coerce the inf/nan to float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 59d9982. Removed the string test cases (df_inf_string, test_equal_to_inf_string, test_not_equal_to_inf_string, test_inf_string_mixed_dataframe) since inf/nan values are now coerced to floats when loading data.
… fig_height - Removed try/except import guard for pytest and pandas in test_plot_filters.py - Renamed fig_width to fig_height to correctly reflect that it controls the height parameter of seaborn's FacetGrid - Updated help text from "Width" to "Height" Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
Since inf/nan string values are now converted to floats when loading
data, the string-based test cases are no longer needed. Updated tests
to only use float('inf') and removed:
- df_inf_string DataFrame
- test_equal_to_inf_string
- test_not_equal_to_inf_string
- test_inf_string_mixed_dataframe
Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
ProExpertProg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Refactored test_unique_benchmark_names_validation to use pytest.raises - Refactored test_unique_benchmark_names_multiple_duplicates to use pytest.raises - Cleaner and more idiomatic pytest error testing Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com>
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: Luka Govedič <luka.govedic@gmail.com>
…ing for vllm bench sweep (vllm-project#29025) Signed-off-by: Luka Govedič <luka.govedic@gmail.com> Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com> Co-authored-by: Luka Govedič <luka.govedic@gmail.com> Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com> Signed-off-by: Xingyu Liu <charlotteliu12x@gmail.com>
…ing for vllm bench sweep (vllm-project#29025) Signed-off-by: Luka Govedič <luka.govedic@gmail.com> Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ProExpertProg <11367180+ProExpertProg@users.noreply.github.com> Co-authored-by: Luka Govedič <luka.govedic@gmail.com> Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Purpose
This PR implements key enhancements to
vllm bench sweep serveand plotting:1. Fixed Boolean Nested Dot-Notation Parameters
compilation_config.use_inductor_graph_partition: false→--no-compilation-config.use_inductor_graph_partition❌compilation_config.use_inductor_graph_partition: false→--compilation-config.use_inductor_graph_partition=false✓2. Fixed Nested Dictionary Values
3. Dict Format Support
_benchmark_namefield internally (not passed as CLI parameter)_benchmark_namevalues are unique4. Enhanced Plot Options
--no-error-barsflag to disable error bars (default: enabled)--fig-heightto control subplot height in inches (default: 6.4)--fig-dpito control figure resolution (default: 300)--fig-nameparameter to customize figure prefix (default: "FIGURE")5. Improved Data Handling
Backward Compatibility
✓ All changes are backward compatible - existing list format continues to work
Test Plan
Test Result
All unit tests pass. Verified:
=true/falsesyntax--no-prefix (backward compatible)_benchmark_nameuniqueness validation works correctly (raises ValueError on duplicates)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.