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.

Sign up for GitHub

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

Jump to bottom

Type Hints for Trainer #912

Merged
merged 2 commits into from
Feb 23, 2020
Merged

Type Hints for Trainer #912

merged 2 commits into from
Feb 23, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Feb 21, 2020 •
edited
Loading

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
  • Did you read the contributor guideline?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

First PR for issue #887 suggested by @hadim
I added typehints to the Trainer class as a first proposal. If the core contributers are happy with the type hint style and docs, I am happy to add more type annotations to the rest of the codebase in future PRs.

This PR includes:

  • Type annotations in Trainer (mainly constructor)
  • Removed type annotations in doc string
  • Plugin that generates types in docs (see image below)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

typehint-autodoc

@pep8speaks
Copy link

pep8speaks commented Feb 21, 2020 •
edited
Loading

Hello @awaelchli! Thanks for updating this PR.

Line 314:101: E501 line too long (106 > 100 characters)
Line 362:101: E501 line too long (101 > 100 characters)
Line 515:101: E501 line too long (106 > 100 characters)
Line 553:101: E501 line too long (105 > 100 characters)

Comment last updated at 2020-02-23 02:14:22 UTC

@awaelchli awaelchli requested a review from Borda February 21, 2020 21:47
@awaelchli awaelchli changed the title Typehints for Trainer Type Hints for Trainer Feb 22, 2020
williamFalcon
williamFalcon reviewed Feb 22, 2020
View reviewed changes
pytorch_lightning/trainer/trainer.py
@@ -124,7 +129,7 @@ def __init__(

Trainer(logger=logger)

checkpoint_callback (:class:`CheckpointCallback`): Callback for checkpointing.
checkpoint_callback: Callback for checkpointing.
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the link to the class here?
these should be left how they were

Copy link
Contributor Author

@awaelchli awaelchli Feb 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

these get generated automatically now because of the type annotations. It still shows in the docs even though it is removed from the docstring. Also, it generates a clickable link (before it was not clickable).

Before:
before

After:
after

If this is not what you want, I will revert the changes. However, it would mean that in case an argument changes, the developer has to change the type in two places. It could lead to inconsistency.

Borda
Borda approved these changes Feb 22, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved
@awaelchli
Copy link
Contributor Author

awaelchli commented Feb 22, 2020 •
edited
Loading

I rebased and test fails. Is it the MNIST download problem? #859

@Borda
Copy link
Member

Borda commented Feb 22, 2020

@awaelchli mind rebase?

Adrian Wälchli and others added 2 commits February 23, 2020 00:10
typehints for trainer
e8c2623 
fix type links in docs


fix types in docs


type hints for trainer methods


fix fit docs


switch to comments


readability


added sphinx typehints extension


wip


remove typehints from docstring


more type annotations


fix spaces
@williamFalcon
Update trainer.py
9fbbebf
@williamFalcon williamFalcon merged commit da2f11a into Lightning-AI:master Feb 23, 2020
@awaelchli awaelchli mentioned this pull request Feb 25, 2020
Merged
5 tasks
@Borda Borda linked an issue Feb 26, 2020 that may be closed by this pull request
Add typing to the codebase #887
Closed
@Borda Borda added docs Documentation related feature Is an improvement or enhancement labels Mar 3, 2020
@awaelchli awaelchli deleted the typehintsv2 branch March 12, 2020 23:05
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
@williamFalcon @tullie
Type Hints for Trainer (Lightning-AI#912)
6f8e9d7 
* typehints for trainer 

fix type links in docs


fix types in docs


type hints for trainer methods


fix fit docs


switch to comments


readability


added sphinx typehints extension


wip


remove typehints from docstring


more type annotations


fix spaces

* Update trainer.py

Co-authored-by: William Falcon <waf2107@columbia.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williamFalcon williamFalcon williamFalcon left review comments

@Borda Borda Borda approved these changes

Assignees
No one assigned
Labels
docs Documentation related feature Is an improvement or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add typing to the codebase
4 participants
@awaelchli @pep8speaks @Borda @williamFalcon