Skip to content

Fix ModelPruning(make_pruning_permanent=True) buffers getting removed when saved during training#6073

Merged
carmocca merged 11 commits intomasterfrom
pruning-modelcheckpoint-hotfix
Mar 3, 2021
Merged

Fix ModelPruning(make_pruning_permanent=True) buffers getting removed when saved during training#6073
carmocca merged 11 commits intomasterfrom
pruning-modelcheckpoint-hotfix

Conversation

@carmocca
Copy link
Copy Markdown
Contributor

@carmocca carmocca commented Feb 19, 2021

What does this PR do?

During training, every time the model is saved via ModelCheckpoint, if make_pruning_permanent, the buffers would get removed. This means that the buffers would not accumulate and the pruning amounts are incorrect.

Before submitting

  • [n/a] 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

  • 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

@carmocca carmocca added bug Something isn't working priority: 0 High priority task callback labels Feb 19, 2021
@carmocca carmocca added this to the 1.2.x milestone Feb 19, 2021
@carmocca carmocca self-assigned this Feb 19, 2021
@carmocca carmocca mentioned this pull request Feb 19, 2021
Merged
10 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 19, 2021 •
edited
Loading

Codecov Report

Merging #6073 (faae25c) into master (352e8f0) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #6073   +/-   ##
======================================
- Coverage      93%     93%   -0%     
======================================
  Files         159     159           
  Lines       11362   11370    +8     
======================================
- Hits        10605   10581   -24     
- Misses        757     789   +32

SeanNaren
SeanNaren reviewed Feb 19, 2021
View reviewed changes
@carmocca carmocca mentioned this pull request Feb 19, 2021
Closed
tchaton
tchaton approved these changes Feb 22, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM !

carmocca
carmocca commented Feb 22, 2021
View reviewed changes
@carmocca @tchaton
Apply suggestions from code review
706e848 
Co-authored-by: chaton <thomas@grid.ai>
Base automatically changed from add-checkpoint-to-callback-on_save_checkpoint to master February 25, 2021 15:48
@carmocca carmocca changed the title [Blocked by #6072] Fix ModelPruning(make_pruning_permanent=True) buffers getting removed when saved during training Fix ModelPruning(make_pruning_permanent=True) buffers getting removed when saved during training Feb 25, 2021
@mergify mergify bot removed the has conflicts label Feb 25, 2021
SkafteNicki
SkafteNicki reviewed Feb 26, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

smaller comments

@carmocca carmocca added the ready to be merged PRs ready to be merged label Mar 1, 2021
@mergify mergify bot added the has conflicts label Mar 1, 2021
@mergify mergify bot removed the has conflicts label Mar 1, 2021
@carmocca carmocca enabled auto-merge (squash) March 2, 2021 13:23
tchaton
tchaton approved these changes Mar 3, 2021
View reviewed changes
Copy link
Copy Markdown
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.

LGTM !

@carmocca carmocca merged commit 4a8422c into master Mar 3, 2021
@carmocca carmocca deleted the pruning-modelcheckpoint-hotfix branch March 3, 2021 12:29
tchaton added a commit that referenced this pull request Mar 9, 2021
@carmocca @tchaton
Fix ModelPruning(make_pruning_permanent=True) buffers getting removed…
cf9e408 
… when saved during training (#6073)

Co-authored-by: chaton <thomas@grid.ai>
lexierule pushed a commit that referenced this pull request Mar 9, 2021
@carmocca @tchaton @lexierule
Fix ModelPruning(make_pruning_permanent=True) buffers getting removed…
68dd140 
… when saved during training (#6073)

Co-authored-by: chaton <thomas@grid.ai>
This was referenced Mar 12, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Merged
@slumericanBx slumericanBx mentioned this pull request Dec 27, 2023
Open
@slumericanBx slumericanBx mentioned this pull request Jan 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SkafteNicki SkafteNicki SkafteNicki left review comments

@tchaton tchaton tchaton approved these changes

@justusschock justusschock justusschock approved these changes

@awaelchli awaelchli Awaiting requested review from awaelchli

@Borda Borda Awaiting requested review from Borda

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

+1 more reviewer

@SeanNaren SeanNaren SeanNaren approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carmocca carmocca

Labels

bug Something isn't working callback priority: 0 High priority task ready to be merged PRs ready to be merged

Projects

None yet

Milestone

pl:1.2.x

Development

Successfully merging this pull request may close these issues.

5 participants

@carmocca @SeanNaren @tchaton @justusschock @SkafteNicki