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

Deprecate LightningModule.model_size #8495

Merged

Conversation

roshikouhai
Copy link
Contributor

@roshikouhai roshikouhai commented Jul 20, 2021

What does this PR do?

This PR deprecates the model_size property in LightningModule as discussed and agreed in #8343

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

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)
  • Did you list all the breaking changes introduced by this pull request?

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

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Jul 20, 2021

Hello @roshikouhai! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-30 13:19:42 UTC

@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #8495 (f8a6182) into master (5789e9f) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #8495   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files         168     168           
  Lines       13968   13975    +7     
======================================
+ Hits        12337   12344    +7     
  Misses       1631    1631           

Copy link
Contributor

@ananthsub ananthsub left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @roshikouhai !

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.

LGTM !

pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
@mergify mergify bot added ready PRs ready to be merged has conflicts labels Jul 21, 2021
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

if so, pls replace all usage in the codebase...

pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
@mergify mergify bot removed the ready PRs ready to be merged label Jul 21, 2021
@roshikouhai roshikouhai force-pushed the fix/deprecate-model-size-prop-lm branch from 24c5437 to f7c5025 Compare July 22, 2021 04:05
@roshikouhai
Copy link
Contributor Author

Rebased on top of #8500. I tried updating test_quantization.py to use the new get_model_size() helper function but as @calebrob6 states, it currently doesn't work for models with sparse tensors which the tests have. Not sure if I should leave it be or find a way to implement the sparse tensors in this PR.

@calebrob6
Copy link
Contributor

@roshikouhai, an alternate way to do get_model_size(...) is:

def get_model_size(self, model):
    with io.BytesIO() as f:
        torch.save(model.state_dict(), f)
        f.seek(0, io.SEEK_END)
        size_mb = f.tell() / 1e6
    return size_mb

I preferred to not do it this way as this will use quite a bit of RAM. Would you like me to look into how to implement getting the size of the sparse tensors as well?

@Borda Borda changed the title [deprecate] Deprecate LightningModule.model_size [blocked by #8500] Deprecate LightningModule.model_size Jul 22, 2021
@ananthsub
Copy link
Contributor

@roshikouhai @calebrob6 - as the primary motivation for the PR is to remove this function off the core LightningModule API, what do you think about this sequencing?

  1. Define the utility function which re-uses the exact same implementation and deprecate this off the core API and point users to the new utility
  2. Later (e.g. for v1.5) investigate improvements we can make to implementation of the new utility to avoid file saving while supporting sparse tensors

@roshikouhai
Copy link
Contributor Author

Later (e.g. for v1.5) investigate improvements we can make to implementation of the new utility to avoid file saving while supporting sparse tensors

Given the goal of this PR is to deprecate model_size from LightningModule, I vote towards this option and allow @calebrob6 the time he needs to implement the model_size for sparse tensors.

@roshikouhai roshikouhai force-pushed the fix/deprecate-model-size-prop-lm branch from c40a88a to 36b82f6 Compare July 22, 2021 22:45
@mergify mergify bot removed the has conflicts label Jul 22, 2021
pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/model_helpers.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/model_helpers.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/model_helpers.py Outdated Show resolved Hide resolved
tests/deprecated_api/test_remove_1-6.py Outdated Show resolved Hide resolved
@roshikouhai roshikouhai force-pushed the fix/deprecate-model-size-prop-lm branch 3 times, most recently from f664973 to ec3240b Compare July 28, 2021 00:15
tests/utilities/test_memory.py Outdated Show resolved Hide resolved
@ananthsub ananthsub changed the title [blocked by #8500] Deprecate LightningModule.model_size Deprecate LightningModule.model_size Jul 28, 2021
@roshikouhai roshikouhai force-pushed the fix/deprecate-model-size-prop-lm branch 2 times, most recently from 24df7b6 to 2f20b65 Compare July 28, 2021 17:20
@roshikouhai roshikouhai force-pushed the fix/deprecate-model-size-prop-lm branch from 2f20b65 to 0fde7c6 Compare July 29, 2021 01:03
@mergify mergify bot removed the has conflicts label Jul 29, 2021
@ananthsub
Copy link
Contributor

@Borda do you see any remaining issues? is this good to go?

@Borda Borda added deprecation Includes a deprecation refactor labels Jul 29, 2021
@Borda Borda added this to the v1.5 milestone Jul 29, 2021
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

fixed the remaining

pytorch_lightning/core/lightning.py Outdated Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label Jul 29, 2021
@roshikouhai roshikouhai force-pushed the fix/deprecate-model-size-prop-lm branch from c068089 to 390c2c1 Compare July 29, 2021 23:06
Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

Pushed a commit to call the new function from the old one

@carmocca carmocca enabled auto-merge (squash) July 30, 2021 12:58
@carmocca carmocca merged commit ba80534 into Lightning-AI:master Jul 30, 2021
@roshikouhai roshikouhai deleted the fix/deprecate-model-size-prop-lm branch July 30, 2021 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Includes a deprecation ready PRs ready to be merged refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants