feat: exit on unknown args #421
Conversation
Signed-off-by: ChanderG <mail@chandergovind.org>
ChanderG
requested review from
anhuong,
Ssukriti,
aluu317,
fabianlim and
kmehant
as code owners
|
Thanks for making a pull request! 😃 |
| ) = parser.parse_args_into_dataclasses(return_remaining_strings=True) | ||
|
|
||
| peft_method = additional.peft_method | ||
| exp_metadata = additional.exp_metadata | ||
| if leftover: | ||
| logging.error("Extra un-recognized arguments found: %s", leftover) | ||
| sys.exit(USER_ERROR_EXIT_CODE) |
There was a problem hiding this comment.
Wouldn't be better to log and continue?
There was a problem hiding this comment.
Yes - that is also ok. I would prefer immediate exit - since there are usually a wall of log output and this can get missed out - but we can go with whatever is the consensus.
There was a problem hiding this comment.
Sorry for the delay in reviewing this, there are two issues here -- to pass in accelerate launch arguments via JSON for kube deployments, the user must pass in a new field called accelerate_launch_args as described here. This is interpreted in the accelerate_launch.py script and then ignored in SFTTrainer. This change would break this behavior. I can see how being warned about misspellings or additional arguments is helpful. We can start this off as warning to not break current behavior or this PR can address the issue with accelerate_launch_args by removing them in the accelerate_launch.py script
| ) = parser.parse_args_into_dataclasses(return_remaining_strings=True) | ||
|
|
||
| peft_method = additional.peft_method | ||
| exp_metadata = additional.exp_metadata | ||
| if leftover: | ||
| logging.error("Extra un-recognized arguments found: %s", leftover) | ||
| sys.exit(USER_ERROR_EXIT_CODE) |
There was a problem hiding this comment.
Sorry for the delay in reviewing this, there are two issues here -- to pass in accelerate launch arguments via JSON for kube deployments, the user must pass in a new field called accelerate_launch_args as described here. This is interpreted in the accelerate_launch.py script and then ignored in SFTTrainer. This change would break this behavior. I can see how being warned about misspellings or additional arguments is helpful. We can start this off as warning to not break current behavior or this PR can address the issue with accelerate_launch_args by removing them in the accelerate_launch.py script
Description of the change
Logs and errors out in case of unknown input args.
Related issue number
Closes #420
How to verify the PR
Simply run with some extra/flags with typos etc.
Was the PR tested
Manually.