-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
store_true for bools in the trainer's argparse parser #1168
Comments
Hi! thanks for your contribution!, great first issue! |
Just a quick thought.
The two could then also be made mutually exclusive, see here. |
@awaelchli I like your solution a lot more than mine. However, again, from a high level I'm not sure if it makes the interface more confusing. Let's see if others have an opinion too. Cool stuff though, I totally forgot you could do that! |
it looks nice but it would need some args synthesis on how to make negation... |
Not a core contributor, but my two cents would be to only create argparse arguments for the opposite of the default for boolean values. In order to have a consistent naming scheme, I'd probably go for something simple like just adding # show_progress_bar is True by default, so create an arg to make it False
parser.add_argument("--no_show_progress_bar", dest="show_progress_bar", action="store_false")
# fast_dev_run is False by default, so create an arg to make it True
parser.add_argument("--fast_dev_run", action="store_true") This might be a touch confusing to a user at first, but the nice thing is the argparse help (i.e. running the script with
|
@joeddav Are you working on this? If not, I'll take care of it 😄 |
@nateraw Oops sorry, no I'm not – feel free to tackle it as I currently don't have the bandwidth 😊 |
I see, when the argument type is bool, we can use the action="store_true" to set the default value False, if we want to set the default value True, we can explicitly specify the command line arguments and not need the value.: such as: |
🚀 Feature
Do we want to have
action='store_true'
for any args in the default trainer argument parser?Motivation
Currently, the bool parameters must be passed like:
It would be nice to just pass that as a flag:
However, once I started looking into it, I noticed why this is not the case. You might have
True
values that you'd like to set toFalse
. If you useaction='store_false'
, you're left with a confusing flag. For example, this doesn't intuitively feel like it would be turning off the progress bar.Pitch
We could set all
bool
type parameters with a default ofFalse
to useaction='store_true'
. Truly not sure if this makes the interface more confusing to an end user. I find it more intuitive, but I'd like to hear others' thoughts.Alternatives
action='store_true'
(i.e.fast_dev_run
)show_progress_bar
that default toTrue
to something likehide_progress_bar
and then do what was suggested in the "Pitch".Additional context
Here's a quick fork where I did what I suggested in the Pitch
The text was updated successfully, but these errors were encountered: