Skip to content

LightningCLI documentation improvements #8303

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

Merged
merged 3 commits into from
Jul 8, 2021
Merged

LightningCLI documentation improvements #8303

merged 3 commits into from
Jul 8, 2021

Conversation

mauvilsa
Copy link
Contributor

@mauvilsa mauvilsa commented Jul 6, 2021

What does this PR do?

It includes several improvements to the documentation of LightningCLI. There is now a section that explains the use of command line arguments as discussed in #8093. It also explains the use of defaults for parameters that have a class as type hint. And includes some notes about reproducibility.

The version of jsonargparse is also bumped. With this now it is possible to configure subclasses via individual command line arguments without having to create a dictionary.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • [n/a] Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

@mauvilsa mauvilsa changed the title Cli documentation LightningCLI documentation improvements Jul 6, 2021
@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #8303 (fa88582) into master (c4353ea) will decrease coverage by 4%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #8303    +/-   ##
=======================================
- Coverage      92%     88%    -4%     
=======================================
  Files         213     214     +1     
  Lines       13791   13914   +123     
=======================================
- Hits        12732   12227   -505     
- Misses       1059    1687   +628     

@mauvilsa
Copy link
Contributor Author

mauvilsa commented Jul 6, 2021

@jlperla

Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@carmocca carmocca added this to the v1.4 milestone Jul 6, 2021
@carmocca carmocca added argparse (removed) Related to argument parsing (argparse, Hydra, ...) docs Documentation related feature Is an improvement or enhancement labels Jul 6, 2021
@jlperla
Copy link

jlperla commented Jul 6, 2021

Given the above python train.py --optimizer.class_path=torch.optim.Adam --optimizer.init_args.lr=0.01 I assume that if I define other defaults in a separate file that these will override them. If so, documenting it might be very valuable. Would it be something liike

python trainer.py --config default_config.yaml -optimizer.class_path=torch.optim.Adam --optimizer.init_args.lr=0.01

If so, then it would be great to show people that (with the example a config that would have the optimizer stuff setup that they could edit).

Then the thing I really would love to see in in the docs is something telling me how to make that default config file a default, so I don't need to pass it in each time! Then I can just register ./default_config.yaml as the default in my subclassed Lightning Parser somewhere, make sure it is in that folder, then just go

python trainer.py --optimizer.class_path=torch.optim.Adam --optimizer.init_args.lr=0.01

or to use all of the defaults!

python trainer.py

where I can then still use any other config file with python trainer.py --config another_config.yml

@mauvilsa
Copy link
Contributor Author

mauvilsa commented Jul 7, 2021

Given the above python train.py --optimizer.class_path=torch.optim.Adam --optimizer.init_args.lr=0.01 I assume that if I define other defaults in a separate file that these will override them. If so, documenting it might be very valuable. Would it be something liike

python trainer.py --config default_config.yaml -optimizer.class_path=torch.optim.Adam --optimizer.init_args.lr=0.01

This is already explained. There is a whole section for it with examples.

Then the thing I really would love to see in in the docs is something telling me how to make that default config file a default, so I don't need to pass it in each time!

Will add this.

@mauvilsa mauvilsa requested a review from ananthsub as a code owner July 7, 2021 17:43
@pep8speaks
Copy link

pep8speaks commented Jul 7, 2021

Hello @mauvilsa! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-08 06:33:52 UTC

@mauvilsa
Copy link
Contributor Author

mauvilsa commented Jul 7, 2021

Then the thing I really would love to see in in the docs is something telling me how to make that default config file a default, so I don't need to pass it in each time!

Will add this.

Done

@mergify mergify bot added the has conflicts label Jul 7, 2021
@kaushikb11
Copy link
Contributor

@mauvilsa I believe there has been a bad merge.

@mergify mergify bot removed the has conflicts label Jul 7, 2021
@mauvilsa
Copy link
Contributor Author

mauvilsa commented Jul 7, 2021

@mauvilsa I believe there has been a bad merge.

I have no idea what I did. But now it is fixed.

@awaelchli awaelchli added the ready PRs ready to be merged label Jul 7, 2021
@kaushikb11 kaushikb11 merged commit 7d3452a into Lightning-AI:master Jul 8, 2021
@mauvilsa mauvilsa deleted the cli_documentation branch July 8, 2021 07:22
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, ...) docs Documentation related feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants