-
Notifications
You must be signed in to change notification settings - Fork 234
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
Add additional Cli parsing #474
Conversation
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.
Addressed most of the comments.
Should be a lot cleaner now.
…s, updated version option behavior, cleaned up code
80849d1
to
d660e9d
Compare
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.
Looking good! Only minor comments since last update.
# TODO: necessary? | ||
# parser.add_argument( | ||
# "--output-length", | ||
# type=int, | ||
# default=128, | ||
# required=False, | ||
# help="The output length (tokens) to use for benchmarking LLMs. (Default: 128)", | ||
# ) |
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.
Assuming we'll clean up these comments blocks and unused dataset_args and stuff in later follow-up PRs
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.
LGTM, assuming other commented/unused code will be cleanup in follow-up PRs
* Adding subprocess call and more arg parsing * Add url option * Remove input length option * Update help messages, mistakes in args, removed sync and async options, updated version option behavior, cleaned up code * Refactored code to clean things up * Use metavar and dest in cli to help with url option
* Adding subprocess call and more arg parsing * Add url option * Remove input length option * Update help messages, mistakes in args, removed sync and async options, updated version option behavior, cleaned up code * Refactored code to clean things up * Use metavar and dest in cli to help with url option
* Adding subprocess call and more arg parsing * Add url option * Remove input length option * Update help messages, mistakes in args, removed sync and async options, updated version option behavior, cleaned up code * Refactored code to clean things up * Use metavar and dest in cli to help with url option
* Adding subprocess call and more arg parsing * Add url option * Remove input length option * Update help messages, mistakes in args, removed sync and async options, updated version option behavior, cleaned up code * Refactored code to clean things up * Use metavar and dest in cli to help with url option
Modifying parser code to fit the args elevated to PA.
Handle args that need massaging between PA core and GenAI-PA.
Make the subprocess call.