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

Trainer parser error: argument --fast_dev_run: invalid int value: 'True' #5069

Closed
nathanpainchaud opened this issue Dec 10, 2020 · 6 comments · Fixed by #7240
Closed

Trainer parser error: argument --fast_dev_run: invalid int value: 'True' #5069

nathanpainchaud opened this issue Dec 10, 2020 · 6 comments · Fixed by #7240
Labels
bug Something isn't working help wanted Open to be worked on priority: 1 Medium priority task
Milestone

Comments

@nathanpainchaud
Copy link
Contributor

🐛 Bug

When generating a parser for Trainer arguments with Trainer.add_argparse_args, the type of fast_dev_run is no longer interpreted correctly (was working correctly on PL 1.08, but no longer works with PL 1.1).

When interpreting an argument like follows:

python my_script.py --fast_dev_run=True [...]

now raises the following error:

error: argument --fast_dev_run: invalid int value: 'True'

I assume this regression was introduced when changing the type of fast_dev_run from bool to Union[int, bool] as per #4629, which messes up type inference when building the parser.

Here is a Colab Notebook reproducing the issue.

Expected behavior

Even though fast_dev_run now can be an int, the automatically generated trainer CLI should continue supporting the boolean options.

Environment

Lightning 1.1

@nathanpainchaud nathanpainchaud added bug Something isn't working help wanted Open to be worked on labels Dec 10, 2020
@Borda Borda added the priority: 0 High priority task label Dec 10, 2020
@Borda Borda added this to the 1.1.x milestone Dec 10, 2020
@Borda
Copy link
Member

Borda commented Dec 10, 2020

I guess that moving to #4492 would solve the problem...

@mauvilsa
Copy link
Contributor

I do think that #4492 would already fix this. @nathanpainchaud would you mind trying out the new LightningCLI concept with any use case you have?

@nathanpainchaud
Copy link
Contributor Author

@mauvilsa I think the LightningCLI API is an interesting concept, but I don't think in my case it's gonna be trivial to transfer over to it. I structured my CLI using subcommands to be able to use the same scripts to launch multiple types of models on different datasets. I also have mixins that programmatically add their own custom arguments to the parser (through hooks).

From what I gather glancing at the docs, it seems to me that the LightningCLI targets use cases where you have a specific script for each model/dataset pair. Can you confirm my basic understanding, or is there some way to programmatically compose CLIs that I haven't seen?

@edenlightning edenlightning added priority: 1 Medium priority task and removed priority: 0 High priority task labels Dec 14, 2020
@mauvilsa
Copy link
Contributor

@nathanpainchaud my comment was not for you to transfer all your current project to use LightningCLI. It was more maybe try for a single model checking that all the parameters that you use are working, including fast_dev_run. Basically play around with LightningCLI. Also I was hoping to get some feedback about LightningCLI which in fact you already did. Indeed being able to easily change models and datasets is a very common use case. Note that LightningCLI is not finalized and this kind of feedback is what is needed to improve it further.

I have thought a bit about the changing models and datasets via config file. But haven't yet concluded what would be the best way to approach this. Will let you know when it is more clear.

@Borda Borda modified the milestones: 1.1.x, 1.2 Feb 8, 2021
@edenlightning edenlightning modified the milestones: 1.2, 1.2.x Feb 8, 2021
@mauvilsa
Copy link
Contributor

mauvilsa commented Apr 7, 2021

@nathanpainchaud LightningCLI is now merged to master. A single cli can be implemented to support multiple models and datasets. Though from what you said it is not simple to change your current code to use it. As alternative you could consider changing your code to jsonargparse which is what LightningCLI internally uses. In jsonargparse there is support for subcommands though they work a bit differently than in argparse. But still I would think it wouldn't be a big change. If your code is open source you could share the link to take a look.

@awaelchli
Copy link
Contributor

awaelchli commented Apr 17, 2021

it has changed to an int, we have to use python my_script.py --fast_dev_run=x. where x is the number of steps (x=1 for True)

@Borda Borda modified the milestones: 1.2.x, 1.3 Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Open to be worked on priority: 1 Medium priority task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants