Adding utility function for calculating model size in bytes #8500

calebrob6 · 2021-07-21T02:19:47Z

What does this PR do?

Adding a utility function for computing the size of a model per discussion in #8343

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

No breaking changes.

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 · 2021-07-21T02:19:50Z

Hello @calebrob6! 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-08-06 09:26:23 UTC

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

codecov · 2021-07-21T02:21:41Z

Codecov Report

Merging #8500 (4ca4579) into master (6701526) will decrease coverage by 4%.
The diff coverage is 100%.

❗ Current head 4ca4579 differs from pull request most recent head 415c0bd. Consider uploading reports for the commit 415c0bd to get more accurate results

@@           Coverage Diff            @@
##           master   #8500     +/-   ##
========================================
- Coverage      93%     89%     -4%     
========================================
  Files         180     169     -11     
  Lines       15895   14084   -1811     
========================================
- Hits        14726   12474   -2252     
- Misses       1169    1610    +441

ananthsub · 2021-07-21T05:42:30Z

pytorch_lightning/utilities/model_helpers.py

@@ -69,3 +71,28 @@ def is_overridden(
    parent_code = parent_attr.__code__

    return instance_code != parent_code
+
+
+def get_model_size(model: nn.Module) -> int:


this could be under https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/utilities/memory.py

get_model_size_bytes is explicit that the return will be in bytes

Should utility be part of the training_type_plugin as shared tensors won't provide the right value ?

ananthsub · 2021-07-21T05:44:42Z

tests/utilities/test_model_helpers.py

+    model = BoringModel()
+
+    size_bytes = get_model_size(model)
+
+    # The BoringModel has a fully connected layer of size 32x2 with a bias resulting in
+    # 67 weights. Each weight is a float32 -- 4 bytes, therefore we expect a size of
+    # 264.
+    assert size_bytes == 264


prefer defining a module to test directly here vs importing it. if someone changes the BoringModel changes for any reason, this test will fail

The BoringModel shouldn't change :) If it does, lot of test will fail :)

ananthsub · 2021-07-21T05:45:29Z

tests/utilities/test_model_helpers.py

+    class BoringSparseModel(BoringModel):
+
+        def __init__(self):
+            super().__init__()
+            self.layer = nn.Parameter(torch.ones(32).to_sparse())


same as above

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

awaelchli · 2021-07-27T21:43:11Z

@calebrob6 I merged master and added a changelog entry.
Ready for review?
This will then unblock #8495

calebrob6 · 2021-07-28T00:25:28Z

Hey @awaelchli, I just requested a review, although I have not changed anything about the code since the above comments.

I think (from #8495) that this code should be merged as-is as an utility function, the model_size() property will be deprecated, then I will implement a new version of this utility function that deals with sparse tensors.

awaelchli · 2021-07-28T08:06:57Z

Yesterday #8495 was labelled as "blocked by 8500".

@roshikouhai @ananthsub @calebrob6 can you confirm in which order these two PRs should be reviewed and merged?

Borda · 2021-07-28T08:18:30Z

can you confirm in which order these two PRs should be reviewed and merged?

I think this shall be merged before deprecating size attribute

tests/utilities/test_model_helpers.py

Co-authored-by: Adrian Wälchli <[email protected]>

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

tchaton · 2021-11-01T14:03:34Z

Dear @calebrob6,

Thanks for putting the effort with this PR. The Lightning Team decided to go with the ByteCounter approach for v1.5. in this PR: #10123

Closing this PR.

Best,
T.C

calebrob6 added 2 commits July 21, 2021 02:15

Adding utility method for getting model size and corresponding tests

f303126

Adding another line of documentation

4895500

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

7c0e5fd

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

calebrob6 added 3 commits July 21, 2021 02:31

Fixing formatting

3e4b8dc

Fixed formatting

21e00d2

Fixed formatting

c85d5a5

calebrob6 mentioned this pull request Jul 21, 2021

Deprecate the model_size property on the LightningModule #8343

Closed

ananthsub reviewed Jul 21, 2021

View reviewed changes

ananthsub mentioned this pull request Jul 21, 2021

Deprecate LightningModule.model_size #8495

Merged

12 tasks

awaelchli and others added 2 commits July 27, 2021 23:36

Merge branch 'master' into model_size

36be4da

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

1b58b85

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

awaelchli added this to the v1.5 milestone Jul 27, 2021

awaelchli added the feature Is an improvement or enhancement label Jul 27, 2021

awaelchli added 2 commits July 27, 2021 23:40

update changelog

7d7baf4

update changelog

16561c7

calebrob6 marked this pull request as ready for review July 28, 2021 00:21

calebrob6 requested review from awaelchli, Borda, carmocca, justusschock, kaushikb11, SeanNaren and williamFalcon as code owners July 28, 2021 00:21

Borda approved these changes Jul 28, 2021

View reviewed changes

mergify bot added the has conflicts label Jul 28, 2021

Merge branch 'master' into model_size

ee29df3

mergify bot removed the has conflicts label Jul 29, 2021

awaelchli approved these changes Aug 6, 2021

View reviewed changes

tests/utilities/test_model_helpers.py Outdated Show resolved Hide resolved

mergify bot added the ready PRs ready to be merged label Aug 6, 2021

add match

4ca4579

Co-authored-by: Adrian Wälchli <[email protected]>

mergify bot added the has conflicts label Aug 16, 2021

Merge branch 'master' into model_size

c45c272

Borda requested a review from rohitgr7 as a code owner October 20, 2021 13:22

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

415c0bd

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

tchaton closed this Nov 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding utility function for calculating model size in bytes #8500

Adding utility function for calculating model size in bytes #8500

calebrob6 commented Jul 21, 2021 •

edited by awaelchli

Loading

pep8speaks commented Jul 21, 2021 •

edited

Loading

codecov bot commented Jul 21, 2021 •

edited

Loading

ananthsub Jul 21, 2021

tchaton Jul 21, 2021

ananthsub Jul 21, 2021

tchaton Jul 21, 2021

ananthsub Jul 21, 2021

awaelchli commented Jul 27, 2021

calebrob6 commented Jul 28, 2021

awaelchli commented Jul 28, 2021

Borda commented Jul 28, 2021

tchaton commented Nov 1, 2021

Adding utility function for calculating model size in bytes #8500

Adding utility function for calculating model size in bytes #8500

Conversation

calebrob6 commented Jul 21, 2021 • edited by awaelchli Loading

What does this PR do?

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

Before submitting

PR review

Did you have fun?

pep8speaks commented Jul 21, 2021 • edited Loading

Comment last updated at 2021-08-06 09:26:23 UTC

codecov bot commented Jul 21, 2021 • edited Loading

Codecov Report

ananthsub Jul 21, 2021

Choose a reason for hiding this comment

tchaton Jul 21, 2021

Choose a reason for hiding this comment

ananthsub Jul 21, 2021

Choose a reason for hiding this comment

tchaton Jul 21, 2021

Choose a reason for hiding this comment

ananthsub Jul 21, 2021

Choose a reason for hiding this comment

awaelchli commented Jul 27, 2021

calebrob6 commented Jul 28, 2021

awaelchli commented Jul 28, 2021

Borda commented Jul 28, 2021

tchaton commented Nov 1, 2021

calebrob6 commented Jul 21, 2021 •

edited by awaelchli

Loading

pep8speaks commented Jul 21, 2021 •

edited

Loading

codecov bot commented Jul 21, 2021 •

edited

Loading