Skip to content

Fix typing for Logger files #11370

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

Closed
daniellepintz opened this issue Jan 9, 2022 · 11 comments
Closed

Fix typing for Logger files #11370

daniellepintz opened this issue Jan 9, 2022 · 11 comments
Assignees
Labels
good first issue Good for newcomers help wanted Open to be worked on logger Related to the Loggers refactor
Milestone

Comments

@daniellepintz
Copy link
Contributor

daniellepintz commented Jan 9, 2022

Proposed refactor

Fix typing for these files:
https://github.com/PyTorchLightning/pytorch-lightning/blob/7eab379da2fdca542849ed4ad313d0851c2271e3/pyproject.toml#L61-L68

Motivation

As part of #7037 we want to add typing to the entire Lightning codebase.

Additional Context

Make sure you do one PR per file.
Start by removing the file from pyproject.toml. To test mypy locally make sure you have it installed and then simply run mypy.

cc @Borda @justusschock @awaelchli @akihironitta @rohitgr7 @edward-io @ananthsub @kamil-kaczmarek @Raalsky @Blaizzy

@daniellepintz daniellepintz added good first issue Good for newcomers help wanted Open to be worked on labels Jan 11, 2022
@ananthsub ananthsub added the logger Related to the Loggers label Jan 22, 2022
@learn2phoenix
Copy link

Could I take this?

@daniellepintz
Copy link
Contributor Author

Yup go for it! Just assigned it to you :)

@daniellepintz
Copy link
Contributor Author

@learn2phoenix are you still planning to work on this issue?

@learn2phoenix
Copy link

@daniellepintz Sorry, have been ultra busy. I will push the PR this weekend.

@daniellepintz
Copy link
Contributor Author

No worries! Just checking in. If you do, please do one PR per file :)

@learn2phoenix
Copy link

learn2phoenix commented Feb 17, 2022

@daniellepintz Working on this today, so our expectation should be no errors from mypy on these files or is there more to it? Also, what is generally done if stub files for a module are missing?

@learn2phoenix
Copy link

some of these files have name conflicts with library names or other modules. Would changing the names of these loggers/* files be an acceptable solution? If so, is there any specific naming convention that needs to be used or would something like <current_name>_log.py be ok?

@daniellepintz
Copy link
Contributor Author

Yes the expectation is no errors from mypy on these files.

Which files have name conflicts? These conflicts show up on mypy?

@learn2phoenix
Copy link

yes, they do show up like the following:
wandb.py: error: Source file found twice under different module names: "pytorch_lightning.loggers.wandb" and "wandb"

Also, mypy reports the stubs to be missing for some modules like torch_xla, comet_ml and says that they wouldn't be analyzed. Are we ignoring these errors?

@daniellepintz
Copy link
Contributor Author

I see! @awaelchli wdyt?

@awaelchli
Copy link
Contributor

This was completed as part of #13445

@awaelchli awaelchli added this to the pl:1.8 milestone Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Open to be worked on logger Related to the Loggers refactor
Projects
None yet
Development

No branches or pull requests

4 participants