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

Type Hints for Lightning Core #946

Merged
merged 38 commits into from
Mar 12, 2020
Merged

Type Hints for Lightning Core #946

merged 38 commits into from
Mar 12, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Feb 25, 2020

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?

Follow up on #912 and discussed in #887.
Not high priority.

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 🙃

@awaelchli awaelchli changed the title Type Hints for Lightning Core Type Hints for Lightning Core [WIP] Feb 26, 2020
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.

LGTH, nice addition! just when you update docs, pls use napoleon

pytorch_lightning/core/hooks.py Outdated Show resolved Hide resolved
pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
pytorch_lightning/core/memory.py Outdated Show resolved Hide resolved
pytorch_lightning/core/saving.py Outdated Show resolved Hide resolved
@Borda Borda added docs Documentation related feature Is an improvement or enhancement labels Feb 26, 2020
@awaelchli
Copy link
Contributor Author

awaelchli commented Feb 26, 2020

Let's wait until #833 is merged because I did minor documentation changes and would like to check if there are new warnings.

@Borda Borda changed the title Type Hints for Lightning Core [WIP] [blocked by #833] Type Hints for Lightning Core Feb 26, 2020
@pep8speaks
Copy link

pep8speaks commented Feb 27, 2020

Hello @awaelchli! Thanks for updating this PR.

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

Comment last updated at 2020-03-12 16:47:21 UTC

@Borda Borda changed the title [blocked by #833] Type Hints for Lightning Core Type Hints for Lightning Core Feb 27, 2020
@Borda Borda added the ready PRs ready to be merged label Feb 27, 2020
@awaelchli
Copy link
Contributor Author

awaelchli commented Feb 27, 2020

@hanbyul-kim @Borda
I get many warnings when compiling the docs. Funny thing, the number of warnings changes everytime I run make clean and make html (inside the docs folder).

Most of them are import problems. I tried changing my path variable, didn't help.

WARNING: autodoc: failed to import module 'trainer.training_loop' from module 'pytorch_lightning'; the following exception was raised:
cannot import name 'LightningModule' from 'pytorch_lightning' (/home/adrian/repositories/pytorch-lightning/pytorch_lightning/__init__.py)

It is also happening on master. Should I open an issue or do you know a quick fix?

@Borda
Copy link
Member

Borda commented Feb 27, 2020

I get many warnings when compiling the docs. Funny thing, the number of warnings changes everytime I run make clean and make html (inside the docs folder).

if does not compile always everything, it uses a cache so if you clean in between compilations you will have the same number...

Most of them are import problems. I tried changing my path variable, didn't help.

have you rebased on master after #833 there was most of the imports resolved

It is also happening on master. Should I open an issue or do you know a quick fix?

@awaelchli That would be great! 🤖
Just if you go for it, please make this PR as wip so Will does not merge it in meantime lol

@awaelchli awaelchli mentioned this pull request Mar 2, 2020
5 tasks
@awaelchli awaelchli changed the title Type Hints for Lightning Core Type Hints for Lightning Core [WIP] Mar 6, 2020
@awaelchli awaelchli changed the title Type Hints for Lightning Core [WIP] Type Hints for Lightning Core Mar 7, 2020
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.

add it to chnagelog

@Borda
Copy link
Member

Borda commented Mar 11, 2020

@williamFalcon @hadim @luiscape ^^

@luiscape
Copy link
Contributor

It looks rather good; I like it. Maybe an appropriate next step would be to add a type checker such as mypy as as CI step?

@Borda
Copy link
Member

Borda commented Mar 11, 2020

hey there, we have added GPU CI test, so could we kindly ask to rebase/merge master which will trigger these tests so we do not need to test it manually... Thx for your understanding 🤖

@Borda
Copy link
Member

Borda commented Mar 11, 2020

It looks rather good; I like it. Maybe an appropriate next step would be to add a type checker such as mypy as as CI step?

good idea, do you want to send a PR? :]

@williamFalcon williamFalcon merged commit 3c2fd56 into Lightning-AI:master Mar 12, 2020
@awaelchli awaelchli deleted the module-typehints branch March 12, 2020 23:04
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
* first pass for LightningModule typehints

* fix return types

* add missing types

* add type annotations to grads.py

* add type annotations to hooks.py

* add type annotation to memory.py

* proper docstring quotation marks

* add type annotations to saving.py

* fix cyclic import problem

* fix cyclic import problem

* add missing whitespace

* finish type hints for load_from_ methods

* docs: prepare_data does not return anything

* fix auto types in docs

* revert typehint for trainer in hook

* remove unnecessary return docs

* some fixes for memory docs

* revert typing for args kwargs

* added all missing None return types

* remove unused import

* add more details to dict/list return types

* fix line too long

* optimize imports

* linted

* Revert "linted"

This reverts commit 8555961.

* remove whitespace

* update

* update

* update

* update

* update

* changelog

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: William Falcon <waf2107@columbia.edu>
@Borda Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants