-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix mypy errors attributed to pytorch_lightning.loggers.logger.py
#13541
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
Fix mypy errors attributed to pytorch_lightning.loggers.logger.py
#13541
Conversation
update for recent type hint merges
|
@awaelchli @carmocca et al regarding logger.merge_dicts: I've looked at the references and the hierarchy of logger.merge_dicts; it seems to only exist to fulfill a test, and for backwards compatibility in loggers.base.merge_dicts. Neither base.merge_dicts or logger.merge_dicts is used by the base Logger, or any of the third party loggers. The way logger.merge_dicts is implemented violated it's own type hints, causing the following errors:
I updated the |
|
|
||
| @wraps(fn) | ||
| def experiment(self): | ||
| def experiment(self: Callable) -> Union[ExperimentWriter, DummyExperiment]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't annotate self. self has always the pseudo type "Self"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remove that annotation, mypy complains:
pytorch_lightning/loggers/logger.py:42: error: Function is missing a type annotation for one or more arguments.
Rather than annotate Self, I commented with # type: ignore[no-untyped-def] to exclude the error in the latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This self for the local function definiton is not necessarily the "self" from the actual instance.
Co-authored-by: Adrian Wälchli <[email protected]>
pytorch_lightning.loggers.logger.py
akihironitta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JustinGoheen Other than my comments below, it looks good to me!
Also, the following PRs are waiting on checks, is there anything more I need to do before the remaining checks can run?
Let's wait for reviews from others!
|
Oh, things started failing as |
|
@otaj I had set a conditional import of ExperimentWriter; I removed that condition in favor of a hardcoded import. |
|
@JustinGoheen Great :) |
…ightning-AI#13541) Co-authored-by: Adrian Wälchli <[email protected]> Co-authored-by: Akihiro Nitta <[email protected]>
What does this PR do?
Fix mypy errors attributed to pytorch_lightning.loggers.logger.py for issue #13445
Fixes #<issue_number>
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list: