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

Fix Pruning callback and add a few features #5825

Merged
merged 30 commits into from
Feb 10, 2021
Merged

Conversation

carmocca
Copy link
Contributor

@carmocca carmocca commented Feb 5, 2021

What does this PR do?

Many test fixes
Many refactors

TODO:

  • Make lottery a Callable + tests
  • Support iterative pruning + tests
  • Save pruned model on checkpoint
  • Check docs look good
  • Sparsity history and tracking
  • Allow resampling
  • Full model statistics, more verbose level
  • Support loading non-permanent pruned checkpoint
  • Second chance? edit: left for a future PR (maybe?)
  • CHANGELOG edit: No need since pruning hasn't been released yet

Before submitting

  • Was this discussed/approved via a GitHub issue?: Discussed offline
  • 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

  • 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 🙃

@carmocca carmocca added bug Something isn't working feature Is an improvement or enhancement callback labels Feb 5, 2021
@carmocca carmocca added this to the 1.2 milestone Feb 5, 2021
@pep8speaks
Copy link

pep8speaks commented Feb 5, 2021

Hello @carmocca! Thanks for updating this PR.

Line 394:13: W503 line break before binary operator
Line 395:13: W503 line break before binary operator

Comment last updated at 2021-02-10 14:42:43 UTC

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #5825 (0f3d113) into release/1.2-dev (774e9bd) will decrease coverage by 0%.
The diff coverage is 96%.

@@               Coverage Diff                @@
##           release/1.2-dev   #5825    +/-   ##
================================================
- Coverage               83%     83%    -0%     
================================================
  Files                  181     181            
  Lines                12929   12796   -133     
================================================
- Hits                 10771   10612   -159     
- Misses                2158    2184    +26     

pytorch_lightning/callbacks/pruning.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/pruning.py Outdated Show resolved Hide resolved
@mergify mergify bot added the has conflicts label Feb 5, 2021
@mergify mergify bot removed the has conflicts label Feb 9, 2021
@Borda
Copy link
Member

Borda commented Feb 9, 2021

it would be much easier to have each of the 5 features as a separate PR

@carmocca
Copy link
Contributor Author

carmocca commented Feb 9, 2021

it would be much easier to have each of the 5 features as a separate PR

Consider this as a Pruning make-over more than adding individual features. The TODO above was just for me to track pending things

@Borda
Copy link
Member

Borda commented Feb 9, 2021

it would be much easier to have each of the 5 features as a separate PR

Consider this as a Pruning make-over more than adding individual features. The TODO above was just for me to track pending things

yes but it now because there is some code already, so remove all in one PR and then do this, now the reading with seeing past state is hard...

@carmocca carmocca mentioned this pull request Feb 10, 2021
12 tasks
@carmocca carmocca added the ready PRs ready to be merged label Feb 10, 2021
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Hey @carmocca, incredible work there !

@tchaton tchaton enabled auto-merge (squash) February 10, 2021 13:53
@carmocca
Copy link
Contributor Author

Line 394:13: W503 line break before binary operator
Line 395:13: W503 line break before binary operator

This is a false positive. This rule should be ignored:

https://github.com/PyTorchLightning/pytorch-lightning/blob/388eeea52d3abe5e8c08197ed06dc9ca9eeefe1c/setup.cfg#L83

pyproject.toml Outdated Show resolved Hide resolved
@carmocca carmocca mentioned this pull request Feb 10, 2021
23 tasks
pyproject.toml Outdated Show resolved Hide resolved
@carmocca carmocca requested a review from Borda February 10, 2021 14:43
@carmocca carmocca mentioned this pull request Feb 10, 2021
7 tasks
@tchaton tchaton merged commit a028171 into release/1.2-dev Feb 10, 2021
@tchaton tchaton deleted the pruning-fix branch February 10, 2021 15:03
@carmocca carmocca self-assigned this Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working callback feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants