Added Vulture dead code checker #5654

aniketmaurya · 2021-01-26T08:37:57Z

What does this PR do?

Resolves #5649
I have integrated Vulture to CI for dead code checking.

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 update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read 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
Check that target branch and milestone match!

Did you have fun?

Make sure you had fun coding 🙃

anshajk · 2021-01-26T08:52:16Z

.github/workflows/code-formatting.yml

        run: |
          mypy
+
+  python-vulture:


I've been following the issue for which this PR has been raised and I wonder if it makes sense to have this as a separate workflow instead of adding it in the code formatting workflow.

Borda

does it report any issue now?

.github/workflows/code-formatting.yml

pyproject.toml

Borda · 2021-01-26T09:01:35Z

@aniketmaurya thx for the quick PR, mind rebase it on feat 1.2

codecov · 2021-01-26T09:18:42Z

Codecov Report

Merging #5654 (0966902) into master (e4ba06c) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #5654    +/-   ##
=======================================
- Coverage      93%     88%    -5%     
=======================================
  Files         199     199            
  Lines       13008   13008            
=======================================
- Hits        12044   11425   -619     
- Misses        964    1583   +619

aniketmaurya · 2021-01-26T13:43:44Z

does it report any issue now?

Hi @Borda, You mean in the GitHub actions? Yes, there are few dead code blocks detected by it.

Borda · 2021-01-26T13:51:44Z

does it report any issue now?

Hi @Borda, You mean in the GitHub actions? Yes, there are few dead code blocks detected by it.

ok, soo it shall be fixed within the PR :]
but fist can you rebase it in release/1.2-dev?

aniketmaurya · 2021-01-26T15:02:56Z

does it report any issue now?

Hi @Borda, You mean in the GitHub actions? Yes, there are few dead code blocks detected by it.

ok, soo it shall be fixed within the PR :]
but fist can you rebase it in release/1.2-dev?

@Borda I think I messed up something during hard reset to 1.2 :(

Borda · 2021-01-26T15:09:29Z

@aniketmaurya fixed :] can you fix the code so the check pass :]

aniketmaurya · 2021-01-26T15:48:34Z

@aniketmaurya fixed :] can you fix the code so the check pass :]

Thank you @Borda
There are a total of 37 unused variables detected in different modules. Should I check and remove them or add them to whitelist? I am not familiar with all of the modules.

rohitgr7 · 2021-01-26T19:36:36Z

Hi @RJ722, mind review the vulture workflow CI here?

aniketmaurya · 2021-03-25T17:40:01Z

@aniketmaurya mind resolving conflicts?

Done @edenlightning !

carmocca · 2021-04-26T13:34:28Z

I'm not sure we should integrate this checker into Lightning, considering we have a lot of dynamism in the way we call hooks so this checker will often produce false positives.

The number of items you already had to whitelist makes me think this is not that useful for this project.

Co-authored-by: Rahul Jha <[email protected]>

for more information, see https://pre-commit.ci

pyproject.toml

ethanwharris · 2021-06-01T13:58:29Z

I agree with @carmocca, seems there are a lot of false positives (and a long whitelist) so I'm not sure there is much benefit to adding this.

Borda · 2021-06-01T15:18:16Z

I do not have any preference on used tool, but still thinking that we shall address dead code in our codebase...

.github/workflows/code-formatting.yml

pls re-review

…tning into aniketmaurya/master

awaelchli · 2021-06-02T10:05:25Z

pytorch_lightning/profiler/pytorch.py

                self._handles[module_name] = [pre_forward_handle, post_forward_handle]

-    def __exit__(self, exc_type: Any, exc_value: Any, traceback: Any) -> None:
+    def __exit__(self, type: Any, value: Any, traceback: Any) -> None:


type is a built-in keyword.
better to avoid name clash here

Vulture picks up exc_type because it doesn't match the parent signature. We have a few options:

leave like this, matches the parent

convert to _, __, ___ - standard for unused variables

add to whitelist (already a bit big)

.github/workflows/code-formatting.yml

aniketmaurya requested review from Borda, SeanNaren, awaelchli, justusschock, tchaton and williamFalcon as code owners January 26, 2021 08:37

anshajk reviewed Jan 26, 2021

View reviewed changes

Borda reviewed Jan 26, 2021

View reviewed changes

.github/workflows/code-formatting.yml Outdated Show resolved Hide resolved

.github/workflows/code-formatting.yml Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

Borda added feature Is an improvement or enhancement ci Continuous Integration labels Jan 26, 2021

Borda added this to the 1.2 milestone Jan 26, 2021

Borda changed the base branch from master to release/1.2-dev January 26, 2021 09:01

Borda requested review from ananyahjha93, edenlightning and teddykoker as code owners January 26, 2021 09:01

mergify bot added the has conflicts label Jan 26, 2021

Aniket Maurya and others added 5 commits January 26, 2021 16:08

integrated vulture CI

37e1340

added vulture in workflows

9c6f77c

added vulture in workflows

539e321

vulture logs verbose set false

a402896

Apply suggestions from code review

8f4f05a

mergify bot removed the has conflicts label Jan 26, 2021

carmocca modified the milestones: 1.2.x, 1.3 Mar 29, 2021

Borda modified the milestones: v1.3, v1.4 Apr 27, 2021

Apply suggestions from code review

5f4d533

Co-authored-by: Rahul Jha <[email protected]>

Borda requested a review from kaushikb11 as a code owner May 11, 2021 21:56

[pre-commit.ci] auto fixes from pre-commit.com hooks

21dc1f3

for more information, see https://pre-commit.ci

Borda reviewed May 11, 2021

View reviewed changes

pyproject.toml Show resolved Hide resolved

mergify bot added the has conflicts label Jun 1, 2021

ethanwharris and others added 3 commits June 2, 2021 10:14

Merge branch 'master' into master

9e446e0

Update whitelist

e233e2f

Sort

b597beb

mergify bot removed the has conflicts label Jun 2, 2021

ethanwharris approved these changes Jun 2, 2021

View reviewed changes

ethanwharris added 2 commits June 2, 2021 10:40

Updates

4bdbb82

Updates

77b2cd4

Borda reviewed Jun 2, 2021

View reviewed changes

.github/workflows/code-formatting.yml Outdated Show resolved Hide resolved

Apply suggestions from code review

6b53480

Borda approved these changes Jun 2, 2021

View reviewed changes

Borda requested review from awaelchli and removed request for teddykoker June 2, 2021 09:43

ethanwharris added 2 commits June 2, 2021 10:47

Updates

aa8449b

Merge branch 'master' of https://github.com/aniketmaurya/pytorch-ligh…

0966902

…tning into aniketmaurya/master

awaelchli approved these changes Jun 2, 2021

View reviewed changes

ethanwharris merged commit 0bad218 into Lightning-AI:master Jun 2, 2021

Added Vulture dead code checker #5654

Added Vulture dead code checker #5654

Uh oh!

Conversation

aniketmaurya commented Jan 26, 2021 • edited by Borda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

PR review

Did you have fun?

Uh oh!

anshajk Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

Borda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Borda commented Jan 26, 2021

Uh oh!

codecov bot commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aniketmaurya commented Jan 26, 2021

Uh oh!

Borda commented Jan 26, 2021

Uh oh!

aniketmaurya commented Jan 26, 2021

Uh oh!

Borda commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aniketmaurya commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rohitgr7 commented Jan 26, 2021

Uh oh!

aniketmaurya commented Mar 25, 2021

Uh oh!

carmocca commented Apr 26, 2021

Uh oh!

Uh oh!

ethanwharris commented Jun 1, 2021

Uh oh!

Borda commented Jun 1, 2021

Uh oh!

Uh oh!

awaelchli Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

ethanwharris Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

aniketmaurya commented Jan 26, 2021 •

edited by Borda

Loading

codecov bot commented Jan 26, 2021 •

edited

Loading

Borda commented Jan 26, 2021 •

edited

Loading

aniketmaurya commented Jan 26, 2021 •

edited

Loading