Replace Argparse with ConfigCommand #266
Merged
Conversation
This file contains 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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
debermudez
reviewed
Feb 11, 2025
genai-perf/genai_perf/inputs/converters/openai_chat_completions_converter.py
Show resolved
Hide resolved
| "or 'https'." | ||
| ) | ||
|
|
||
| valid_localhost = parsed_url.hostname in ["localhost", "127.0.0.1"] |
There was a problem hiding this comment.
Technically, [::1] is also a valid localhost value, when using IPv6.
There was a problem hiding this comment.
My goal here was parity with the checks we currently have in place (in argparse). I'm reluctant to change this check on this branch/in this story.
debermudez
reviewed
Feb 11, 2025
There was a problem hiding this comment.
clean up the codeQL issues and fix the Value Error bug that @nicolasnoble highlighted. Then this is good to go.
nv-braf
added a commit
that referenced
this pull request
Feb 12, 2025
* Ported over logic infer logic from parser * Added unit testing for inferred input/endpoint logic * Completed porting of all inferred and checked fields from parser * Adding missing goodput parsing * Initial changes to get profile working * CLI unit tests passing * Fixing CLI tests that I missed * Telemetry tests passing * Test artifacts passing * Telemetry data collector tests passing * Passing common unit tests * Fixing console exporter unit tests * Fixing CSV exporter unit tests * Partial for for JSON exporter unit tests * Adding library function to convert BaseConfig into a JSON readable dictionary * JSON Exporter unit tests passing * Fixing Tokenizer unit tests * PA Config unit tests passing * Fixing Results and RunConfig unit tests * All unit tests passing * Fixing codeql issues * Moving generation of artifact directory into PA Config generator class * Fixing exporters to use PA config * Progress on analyze. Need to add checkpoint support to base config class. * Fixed GAP config generator. All unit tests passing * Getting analyze working with CLI * Analyze working with config file * fixing codeql * Fixed PA config to work with multiple sweep parameters * Fixing message when config found in checkpoint * Refactoring path method * Removing commented out lines * Fixing issue around specifing url/server metrics url * Changes based on Elias' PR * Fixing codeql issue * Missing check for None
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.
Replaces the use of Argparse throughout GAP with the ConfigCommand class. Here is a high level overview of the changes:
These are the sections of the code that underwent larger changes: