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

Improve typing coverage (4/n) #13445

Closed
52 tasks done
otaj opened this issue Jun 29, 2022 · 105 comments · Fixed by #14963
Closed
52 tasks done

Improve typing coverage (4/n) #13445

otaj opened this issue Jun 29, 2022 · 105 comments · Fixed by #14963
Labels
code quality good first issue Good for newcomers help wanted Open to be worked on let's do it! approved to implement

Comments

@otaj
Copy link
Contributor

otaj commented Jun 29, 2022

🚀 Typing coverage

Let's improve typing coverage of PyTorch Lightning together!

I'm creating a new issue in order to increase visibility. There are three older issues (#7037, #5023, #4698) which became stale over time.

Plan

Currently, there are 55 files which are excluded from mypy checks so that our CI does not fail. These files vastly differ in difficulty in order to make the typing complete. For this reason, we are introducing difficulty estimate for each file so that community members can choose to work on the files appropriate to their skill level.

Please, comment on this issue in order to reserve a particular file to work on. Once you do so, I will edit this top comment to avoid collisions. Once you think your work is finished, please open a PR referencing this issue which:

  • removes the corresponding line from pyproject.toml
  • and passes mypy checks with the corresponding line removed. You can test it locally by running mypy from root directory

If you are struggling with pushing it over the finish line, open the PR anyway and someone from our team will help you to get it there. 🚀

Please note, that it can happen that you may need to edit more than just one file. This is fine, but please keep in mind, that the goal of your PR will be to make the check passing for the chosen file. Also, please note that the difficulty is just an educated guess.

For those of you who are not familiar with the process of contributing a PR, we have prepared a simple guide that will walk you through the necessary steps. You can do it! 🚀 💪

List of files and guesstimated difficulty

Completed

Difficulty 1 of 3

Difficulty 2 of 3

Difficulty 3 of 3

~- [ ] pytorch_lightning/trainer/callback_hook.py @JustinGoheen #13807 ~

cc @Borda @justusschock @awaelchli @rohitgr7 @Borda @tchaton @aniketmaurya @kingjuno @alat-rights @carmocca @akihironitta @stancld as you were all involved in previous issues

@otaj otaj added feature Is an improvement or enhancement help wanted Open to be worked on good first issue Good for newcomers let's do it! approved to implement refactor code quality labels Jun 29, 2022
@otaj otaj pinned this issue Jun 29, 2022
@kingjuno
Copy link
Contributor

@otaj I will start working on these 3 for now.

  • pytorch_lightning/core/decorators.py
  • pytorch_lightning/profilers/advanced.py
  • pytorch_lightning/profilers/base.py

@jxtngx
Copy link
Member

jxtngx commented Jun 29, 2022

@otaj I can take:

  • pytorch_lightning/loggers/base.py
  • pytorch_lightning/loggers/csv_logs.py
  • pytorch_lightning/loggers/logger.py

@carmocca
Copy link
Member

A recommendation for contributors: Unless a file requires <10 lines of fixes, I strongly suggest you work on separate PRs per file if possible. Sometimes mypy requires you to update the typing annotations for a different file, but it does not mean you need to completely annotate all modified files.

@otaj
Copy link
Contributor Author

otaj commented Jun 29, 2022

@JustinGoheen @kingjuno Thank you very much, you were added to the list. However, I have to agree with @carmocca, keep one PR for fixing one file please.

@barufa
Copy link

barufa commented Jun 29, 2022

I would like to make my first contribution to lightning. I can take pytorch_lightning/demos/boring_classes.py and pytorch_lightning/demos/mnist_datamodule.py.

@otaj
Copy link
Contributor Author

otaj commented Jun 29, 2022

@barufa Thank you very much, added you to the list!

@donlapark
Copy link
Contributor

I would like to contribute as well. I can take pytorch_lightning/tuner/lr_finder.py.

@otaj
Copy link
Contributor Author

otaj commented Jun 30, 2022

@donlapark thank you as well. Added to the list:)

@puhuk
Copy link
Contributor

puhuk commented Jun 30, 2022

I will take pytorch_lightning/distributed/dist.py

@otaj
Copy link
Contributor Author

otaj commented Jun 30, 2022

@puhuk Thank you very much! You were added to the list!

@CyprienRicque
Copy link
Contributor

CyprienRicque commented Jun 30, 2022

Hello! I can work on:

@otaj
Copy link
Contributor Author

otaj commented Jun 30, 2022

@Cyprien-Ricque Awesome, thanks! Added your reservations to the master comment.

@gautierdag
Copy link
Contributor

Hi!
Just did a quick PR for pytorch_lightning/trainer/optimizers.py here: #13470

@ar90n
Copy link
Contributor

ar90n commented Jul 1, 2022

Hi! I want to take the followings.

  • pytorch_lightning/callbacks/finetuning.py
  • pytorch_lightning/tuner/batch_size_scaling.py

@otaj
Copy link
Contributor Author

otaj commented Jul 1, 2022

@gautierdag , thank you! I saw it was a fairly small PR, so it doesn't matter much, but we would still prefer in the future if you reserve it as to avoid collisions with other people who might already start working on that. But anyway, thank you :)

@ar90n, you were also added to the list, thank you :)

@gautierdag
Copy link
Contributor

gautierdag commented Jul 1, 2022

@otaj Cheers, I'll try to take on pytorch_lightning/loggers/wandb.py today :)

update: #13483

@otaj
Copy link
Contributor Author

otaj commented Jul 1, 2022

@gautierdag, perfect, thank you, added to the list!

@jxtngx
Copy link
Member

jxtngx commented Jul 1, 2022

@otaj @carmocca

two errors in loggers/base.py are attributed to None types being set as the return type for deprecation warnings; should these be left alone or set as the return type for the future

@jxtngx
Copy link
Member

jxtngx commented Aug 10, 2022

can I be removed from pytorch_lightning/trainer/supporters.py and pytorch_lightning/trainer/trainer.py please

@HeegonJin
Copy link

Can I be released from pytorch_lightning/callbacks/progress/rich_progress.py, if it's available?

@otaj
Copy link
Contributor Author

otaj commented Aug 11, 2022

@JustinGoheen, @HeegonJin, absolutely, though we're gonna be sad about that

@nninept
Copy link
Contributor

nninept commented Aug 12, 2022

Hi, I can take pytorch_lightning/callbacks/progress/rich_progress.py!

@otaj
Copy link
Contributor Author

otaj commented Aug 12, 2022

Absolutely, @nninept, thank you! 🎉 You were added to the list ⚡

@otaj
Copy link
Contributor Author

otaj commented Aug 12, 2022

Hi @nandwalritik, I've unassigned you from pytorch_lightning/profilers/pytorch.py file as there hasn't been any PR opened regarding in the last two weeks. Thank you for your contributions! ⚡

@Jungwon-Lee
Copy link
Contributor

Hi, I would like to take pytorch_lightning/trainer/trainer.py

@otaj
Copy link
Contributor Author

otaj commented Aug 15, 2022

Hi, @BongYang, thank you very much, you were added to the list! ⚡

@krishnakalyan3
Copy link
Contributor

I would like to take up pytorch_lightning/profilers/pytorch.py

@otaj
Copy link
Contributor Author

otaj commented Aug 22, 2022

Alright, @krishnakalyan3, assigned this to you! Thanks a lot ⚡

@donlapark
Copy link
Contributor

Hi! I would like to take pytorch_lightning/trainer/supporters.py.

@awaelchli
Copy link
Member

@donlapark Appreciated! Feel free to go ahead. It could be a challenging one, let us know if you need help!

@donlapark
Copy link
Contributor

donlapark commented Sep 10, 2022

A mypy check has just failed because there are some errors in tuner/lr_finder.py, trainer/connectors/callback_connector.py and trainer/connectors/callback_connector.py (most of them are just Unused "type: ignore"). Maybe we should fix those errors as well?

Edit: Nevermind, the error in tuner/lr_finder.py was caused by my PR on supporters.py—whoops.

@otaj
Copy link
Contributor Author

otaj commented Sep 13, 2022

Hi @nninept, I've unassigned you from pytorch_lightning/callbacks/progress/rich_progress.py file as there hasn't been any PR opened regarding in the last two weeks. Thank you for your contributions! ⚡

@donlapark
Copy link
Contributor

I would like to take rich_progress.py.

@otaj
Copy link
Contributor Author

otaj commented Sep 19, 2022

Awesome, @donlapark! Thank you very much, you were added to the list! ⚡

@otaj
Copy link
Contributor Author

otaj commented Oct 3, 2022

🎉 We did it! 🎉 We just closed last PR resolving typing in PyTorch portion of Lightning 🎉 Thanks to everyone, who contributed, namely @carmocca @nninept @JustinGoheen @CyprienRicque @gautierdag @krishnakalyan3 @ar90n @donlapark @alro923 @himkt @BongYang @HalestormAI @lijm1358 @nandwalritik 🎉

@otaj otaj unpinned this issue Oct 3, 2022
@Borda
Copy link
Member

Borda commented Oct 4, 2022

Shall we do the same with lightning Flow and Work... 🦦

@otaj
Copy link
Contributor Author

otaj commented Oct 4, 2022

I'm all for it! But that's gonna be an entirely different beast, since there's a lot more on the Flow and Work side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality good first issue Good for newcomers help wanted Open to be worked on let's do it! approved to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.