Skip to content
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

Make positional arguments required in the argument parser #12503

Closed
fschlatt opened this issue Mar 29, 2022 · 1 comment · Fixed by #12504
Closed

Make positional arguments required in the argument parser #12503

fschlatt opened this issue Mar 29, 2022 · 1 comment · Fixed by #12504
Labels
argparse (removed) Related to argument parsing (argparse, Hydra, ...) feature Is an improvement or enhancement
Milestone

Comments

@fschlatt
Copy link
Contributor

fschlatt commented Mar 29, 2022

🚀 Feature

Make positional arguments required for classes passed into the add_argparse_args function.

Motivation

I use the add_argparse_args function not only for the Trainer but also for additional classes required for my training script (e.g. the DataModule). Some arguments of the additional classes are positional and do not have suitable default values. In the current implementation, positional arguments receive a default value of inspect._empty, which usually leads to unforeseen errors somewhere further down the line.

Pitch

In the ptl argument parser utilities, modify the parser such that positional arguments of a class given to the add_argparse_args are marked as required in the resulting ArgumentParser.

cc @Borda

@fschlatt fschlatt added the needs triage Waiting to be triaged by maintainers label Mar 29, 2022
@awaelchli awaelchli added argparse (removed) Related to argument parsing (argparse, Hydra, ...) feature Is an improvement or enhancement and removed needs triage Waiting to be triaged by maintainers labels Apr 3, 2022
@awaelchli awaelchli added this to the 1.7 milestone Apr 3, 2022
@carmocca
Copy link
Contributor

carmocca commented Apr 4, 2022

Thanks for the suggestion!

Let me also shamelessly plug the LightningCLI which already takes care of this 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
argparse (removed) Related to argument parsing (argparse, Hydra, ...) feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants