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

store_true for bools in the trainer's argparse parser #1168

Closed
nateraw opened this issue Mar 17, 2020 · 10 comments
Closed

store_true for bools in the trainer's argparse parser #1168

nateraw opened this issue Mar 17, 2020 · 10 comments
Assignees
Labels
discussion In a discussion stage feature Is an improvement or enhancement help wanted Open to be worked on
Milestone

Comments

@nateraw
Copy link
Contributor

nateraw commented Mar 17, 2020

🚀 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:

python main.py --fast_dev_run True

It would be nice to just pass that as a flag:

python main.py --fast_dev_run

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 to False. If you use action='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.

python main.py --show_progress_bar

Pitch

We could set all bool type parameters with a default of False to use action='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

  1. We could ignore this and leave it the way it is
  2. We could explicitly set a few parameters to use action='store_true' (i.e. fast_dev_run)
  3. We could flip bool parameters like show_progress_bar that default to True to something like hide_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

@nateraw nateraw added feature Is an improvement or enhancement help wanted Open to be worked on labels Mar 17, 2020
@github-actions
Copy link
Contributor

Hi! thanks for your contribution!, great first issue!

@awaelchli
Copy link
Member

awaelchli commented Mar 17, 2020

Just a quick thought.
We could have "virtual" arguments like "hide_progress_bar" like so:

parser = argparse.ArgumentParser()
parser.add_argument("--show_progress_bar", default=True, action="store_true")
parser.add_argument("--hide_progress_bar", dest="show_progress_bar", action="store_false")

The two could then also be made mutually exclusive, see here.
There would be no need to change the Trainer args as it only introduces new args in the parser.
This is another alternative to the ones suggested by OP.

@nateraw
Copy link
Contributor Author

nateraw commented Mar 18, 2020

@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!

@Borda Borda added the discussion In a discussion stage label Mar 18, 2020
@Borda Borda added this to the 0.7.2 milestone Mar 18, 2020
@Borda
Copy link
Member

Borda commented Mar 18, 2020

it looks nice but it would need some args synthesis on how to make negation...
see #1147 and we can start with adding action='store_true' for all bool types
Any thoughts @PyTorchLightning/core-contributors?

@joeddav
Copy link
Contributor

joeddav commented Mar 22, 2020

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 no_ in front of the argument name for arguments which are by default true:

# 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 -h) would make it really clear, esp. with easily-automatable help messages:

>>> python run_training.py -h
usage: run_training.py [-h] [--no_show_progress_bar] [--fast_dev_run]

optional arguments:
  -h, --help            show this help message and exit
  --no_show_progress_bar
                        set show_progress_bar=False in the Trainer
  --no_logger           set logger=False in the Trainer
  --fast_dev_run        set fast_dev_run=True in the Trainer

@Borda
Copy link
Member

Borda commented Mar 22, 2020

@joeddav sounds reasonable to me... mind sending a PR after #1147 is merged?

@Borda Borda modified the milestones: 0.7.2, 0.7.3 Apr 8, 2020
@nateraw
Copy link
Contributor Author

nateraw commented Apr 14, 2020

@joeddav Are you working on this? If not, I'll take care of it 😄

@joeddav
Copy link
Contributor

joeddav commented Apr 14, 2020

@nateraw Oops sorry, no I'm not – feel free to tackle it as I currently don't have the bandwidth 😊

@Borda Borda modified the milestones: 0.7.4, 0.7.5 Apr 24, 2020
@Borda Borda modified the milestones: 0.7.6, 0.8.0, 0.7.7 May 12, 2020
@Borda
Copy link
Member

Borda commented May 15, 2020

closed with #1822, #1842

@Borda Borda closed this as completed May 15, 2020
@Borda Borda modified the milestones: 0.7.7, 0.8.0 May 26, 2020
@lvjiujin
Copy link

lvjiujin commented Sep 8, 2021

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:
python argparse_check.py --do_train
here 'do_train' will be True.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion In a discussion stage feature Is an improvement or enhancement help wanted Open to be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants