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

Lightning CLI should use fail_untyped=True #18285

Open
cristianregep opened this issue Aug 11, 2023 · 4 comments
Open

Lightning CLI should use fail_untyped=True #18285

cristianregep opened this issue Aug 11, 2023 · 4 comments
Assignees
Labels
3rd party Related to a 3rd-party breaking change Includes a breaking change help wanted Open to be worked on lightningcli pl.cli.LightningCLI refactor

Comments

@cristianregep
Copy link

cristianregep commented Aug 11, 2023

Outline & Motivation

Use fail_untyped=True here https://github.com/Lightning-AI/lightning/blob/97020bf8d7a88ca5195534b8585a5ef53f1ce6cb/src/lightning/pytorch/cli.py#L135 and here https://github.com/Lightning-AI/lightning/blob/97020bf8d7a88ca5195534b8585a5ef53f1ce6cb/src/lightning/pytorch/cli.py#L139

Pitch

I have spent quite a bit of time with the same issue described here #15741 . The person there also reported spending "several days". I finally figured it out when I created an isolated example with just jsonargparse which told me "Types as a string and from __future__ import annotations is currently not supported" . That was because CLI has fail_untyped=True by default . I do believe it would be better to have that as default to ease debugging. If not by default, maybe allow users to set that themselves.

I do know that from version 4.22 of jsonargparse this specific example is no longer an issue omni-us/jsonargparse#120 , but my point is more about the general principle of having fail_untyped=True

Additional context

No response

cc @justusschock @awaelchli @Borda @carmocca @mauvilsa

@cristianregep cristianregep added needs triage Waiting to be triaged by maintainers refactor labels Aug 11, 2023
@awaelchli awaelchli added lightningcli pl.cli.LightningCLI and removed needs triage Waiting to be triaged by maintainers labels Aug 11, 2023
@carmocca
Copy link
Contributor

This cannot change because it would be a breaking change for CLI users who don't want to type their code. Many Python users are also against typing their scripts so it would make the LightningCLI opinionated.

However, maybe that specific error should be shown regardless of the value of fail_untyped

@carmocca carmocca added 3rd party Related to a 3rd-party breaking change Includes a breaking change labels Aug 12, 2023
@cristianregep
Copy link
Author

Thanks @carmocca for the quick reply. That make sense, a lot of people may be using the cli without typed code . What if the constructor of LightningArgumentParser allowed you to set fail_untyped (and by default it would be false as to not introduce a breaking change).

@carmocca
Copy link
Contributor

We'd accept this contribution if you want to open a PR. The LightningCLI already exposes parser_kwargs

@awaelchli awaelchli added the help wanted Open to be worked on label Aug 26, 2023
@cristianregep
Copy link
Author

Thanks. I will submit a PR this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party Related to a 3rd-party breaking change Includes a breaking change help wanted Open to be worked on lightningcli pl.cli.LightningCLI refactor
Projects
None yet
Development

No branches or pull requests

3 participants