Skip to content

Conversation

@LeeChanHyuk
Copy link

@LeeChanHyuk LeeChanHyuk commented Jul 27, 2022

What does this PR do?

Fixes mypy typing errors in pytorch_lightning/profilers/base.py in #13445.

[Mypy error]

src/pytorch_lightning/profilers/base.py:60: error: Function is missing a type annotation  [no-untyped-def]

Does your PR introduce any breaking changes? If yes, please list them.

None

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

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:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

I had fun doing this contribution. Thank you.

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Jul 27, 2022
@otaj otaj mentioned this pull request Jul 27, 2022
52 tasks
Copy link
Contributor

@otaj otaj left a comment

Choose a reason for hiding this comment

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

Hi @LeeChanHyuk. Thank you for your PR. However, you're not supposed to run mypy <file> but only mypy - the difference is that the latter takes into account whole codebase and not only things imported by the particular file.

When run like this on master branch with "pytorch_lightning.profilers.base" commented out from pyproject.toml, only one error pops up

src/pytorch_lightning/profilers/base.py:60: error: Function is missing a type annotation  [no-untyped-def]
Found 1 errors in 1 file (checked 240 source files)

The reference check is the one happening on our CI, there you can see the errors

@github-actions github-actions bot removed the pl Generic label for PyTorch Lightning package label Jul 28, 2022
@LeeChanHyuk LeeChanHyuk reopened this Jul 28, 2022
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Jul 28, 2022
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #13879 (21c2bb2) into master (c391170) will decrease coverage by 11%.
The diff coverage is 75%.

@@            Coverage Diff            @@
##           master   #13879     +/-   ##
=========================================
- Coverage      86%      75%    -11%     
=========================================
  Files         332      332             
  Lines       26049    26916    +867     
=========================================
- Hits        22328    20130   -2198     
- Misses       3721     6786   +3065     

@LeeChanHyuk
Copy link
Author

Hi @LeeChanHyuk. Thank you for your PR. However, you're not supposed to run mypy <file> but only mypy - the difference is that the latter takes into account whole codebase and not only things imported by the particular file.

When run like this on master branch with "pytorch_lightning.profilers.base" commented out from pyproject.toml, only one error pops up

src/pytorch_lightning/profilers/base.py:60: error: Function is missing a type annotation  [no-untyped-def]
Found 1 errors in 1 file (checked 240 source files)

The reference check is the one happening on our CI, there you can see the errors

@otaj
Thank you for your kind comment.
I followed your instruction for mypy, but I got the test errors on my code.
I am a beginner at this task, so could you please tell me what should I do to fix the test error?
Thank you.

@LeeChanHyuk LeeChanHyuk requested a review from otaj July 29, 2022 01:13
@otaj
Copy link
Contributor

otaj commented Jul 29, 2022

@LeeChanHyuk, no worries about being a beginner, we all started somehow 💜

First of all, the error would be solved perfectly fine if you did this:

def __init__(self, *args: Any, **kwargs: Any):
    ...

However! Since I have not been taking too hard of a look at particular pieces of code that should be fixed in the typing issue before, I just now realized, that we can safely close this PR. If you take a look, this class (BaseProfiler) is deprecated and will be removed in 1.8 release - but we have a 1.7 release on Monday and right after the release, we can remove all the things that are scheduled to be removed in 1.8 release. I'm sorry about assigning this work when the parts of code are deprecated, I didn't notice it.

So, I am closing this PR for now, and on Monday, right after 1.7 release, there will be a call for action for removing features scheduled to be removed in 1.8. Please, sign up there.

Thank you for your contribution ⚡

@otaj otaj closed this Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pl Generic label for PyTorch Lightning package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants