Add `enable_model_summary` flag and deprecate `weights_summary` #9699

ananthsub · 2021-09-25T05:40:39Z

What does this PR do?

Fixes #9043
Adds enable_model_summary flag to match enable_progress_bar flag being added here: #9664

This PR also deprecates the Trainer weights_summary flag and property which dictated the mode for display. Users are expected to customize this with the callback. This deprecation was required because we needed to support arbitrary module depths for summarization (int) while the current mode only supported a few fixed modes ("top" vs "full"). This customization is much easier with the ModelSummary callbacks @kaushikb11 introduced.

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 list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
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 🙃

pytorch_lightning/trainer/connectors/callback_connector.py

rohitgr7

LG!. Just some minor comments.

docs/source/common/trainer.rst

codecov · 2021-09-30T08:15:27Z

Codecov Report

Merging #9699 (799fdf7) into master (4610fdd) will decrease coverage by 4%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #9699    +/-   ##
=======================================
- Coverage      93%     89%    -4%     
=======================================
  Files         178     178            
  Lines       15653   15671    +18     
=======================================
- Hits        14511   13920   -591     
- Misses       1142    1751   +609

CHANGELOG.md

kaushikb11 · 2021-09-30T14:22:58Z

Do we need the enable_ prefix? I think model_summary is straightforward as it is a boolean flag.

ananthsub · 2021-09-30T15:40:41Z

Do we need the enable_ prefix? I think model_summary is straightforward as it is a boolean flag.

This also came up in these places:
#9616 (comment)
#9664 (comment)
#9754 (comment)

Options so far:

Use enable_ as a prefix

Pros:
- consistent naming sceheme: users can scan through the trainer flags and see what features are enabled by default by the Trainer
- enable_ as a a prefix indicates these are booleans
Cons:
- More verbose

Directly use the name. e.g. model_summary, progress_bar, checkpointing

Pros:
- Less verbose
Cons:
- May have confusion with the callback implementation (e.g. model_summary as a flag: does it take the ModelSummary callback? or a boolean? would trainer.model_summary be the callback instance, or the boolean flag passed into the trainer?

Other options?

@PyTorchLightning/core-contributors

Poll:

Option 1 -> 👍
Option 2 -> 🚀
Other -> comment

daniellepintz · 2021-10-04T19:59:42Z

I am strongly in favor of option 1. To expand on "consistent naming scheme" you mentioned, consider these Trainer arguments:

profiler of type BaseProfiler
accelerator of type Accelerator
logger of type LightningLoggerBase

if I were reading these arguments and then saw one named model_summary I would assume it to be of type ModelSummary.

IMO boolean argument names should be prefixed with a verb, such as enable_progress_bar, reload_dataloaders_every_epoch, terminate_on_nan, move_metrics_to_cpu

daniellepintz · 2021-10-04T20:49:31Z

Question: under what circumstances would users want to disable the model summary? Is it critical we have this flag?

docs/source/common/debugging.rst

carmocca · 2021-10-05T13:14:27Z

under what circumstances would users want to disable the model summary? Is it critical we have this flag?

Maybe the summary is not correct for your model, maybe you are calling fit multiple times and you don't want it to appear repeatedly...

daniellepintz · 2021-10-06T05:00:31Z

Maybe the summary is not correct for your model, maybe you are calling fit multiple times and you don't want it to appear repeatedly...

Hmm, if the summary is not correct that sounds like a bug, and we should operate under the assumption that we have no bugs. The other case seems pretty rare, but I don't have a lot of background knowledge here so don't feel strongly

CHANGELOG.md

daniellepintz · 2021-10-07T03:56:01Z

@williamFalcon makes sense, I agree user experience is very important and didn’t mean to suggest otherwise!

pytorch_lightning/trainer/connectors/callback_connector.py

rohitgr7

LG!

pytorch_lightning/trainer/connectors/callback_connector.py

pytorch_lightning/trainer/trainer.py

benchmarks/test_basic_parity.py

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

pytorch_lightning/trainer/connectors/callback_connector.py

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

…tning-AI#9699) Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> Co-authored-by: Kaushik B <45285388+kaushikb11@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Kaushik B <kaushikbokka@gmail.com>

ananthsub added callback deprecation Includes a deprecation labels Sep 25, 2021

ananthsub requested review from awaelchli, Borda, carmocca, edenlightning, justusschock, kaushikb11, SeanNaren, tchaton and williamFalcon as code owners September 25, 2021 05:40

ananthsub changed the title ~~Add enable_model_summary flag~~ Add enable_model_summary flag and deprecate weights_summary Sep 25, 2021

ananthsub added this to the v1.5 milestone Sep 25, 2021

mergify bot added the has conflicts label Sep 25, 2021

rohitgr7 reviewed Sep 25, 2021

View reviewed changes

pytorch_lightning/trainer/connectors/callback_connector.py Show resolved Hide resolved

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved

ananthsub force-pushed the feat/add-enable-model-summary-flag branch from 3a7328b to d83ed9d Compare September 30, 2021 07:03

mergify bot removed the has conflicts label Sep 30, 2021

ananthsub force-pushed the feat/add-enable-model-summary-flag branch from 075fb46 to 8de7fd5 Compare September 30, 2021 07:30

rohitgr7 approved these changes Sep 30, 2021

View reviewed changes

docs/source/common/trainer.rst Outdated Show resolved Hide resolved

docs/source/common/trainer.rst Show resolved Hide resolved

ananthsub force-pushed the feat/add-enable-model-summary-flag branch from 877d0bf to 4e87889 Compare September 30, 2021 07:53

kaushikb11 reviewed Sep 30, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

mergify bot added the has conflicts label Oct 1, 2021

carmocca approved these changes Oct 5, 2021

View reviewed changes

docs/source/common/debugging.rst Outdated Show resolved Hide resolved

ananthsub added 3 commits October 6, 2021 13:18

Update trainer.rst

d643044

Update CHANGELOG.md

6de314d

Update debugging.rst

fe8d615

ananthsub force-pushed the feat/add-enable-model-summary-flag branch from 3a1c155 to fe8d615 Compare October 6, 2021 20:25

mergify bot added ready PRs ready to be merged and removed has conflicts labels Oct 6, 2021

four4fish reviewed Oct 6, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

687f681

mergify bot added the has conflicts label Oct 7, 2021

tchaton reviewed Oct 11, 2021

View reviewed changes

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/connectors/callback_connector.py Show resolved Hide resolved

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved

rohitgr7 approved these changes Oct 11, 2021

View reviewed changes

Borda requested review from four4fish, kaushikb11 and tchaton October 12, 2021 09:49

Borda reviewed Oct 12, 2021

View reviewed changes

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/trainer.py Show resolved Hide resolved

kaushikb11 self-assigned this Oct 12, 2021

kaushikb11 reviewed Oct 13, 2021

View reviewed changes

benchmarks/test_basic_parity.py Show resolved Hide resolved

Merge branch 'master' into feat/add-enable-model-summary-flag

6924b93

mergify bot removed the has conflicts label Oct 13, 2021

pre-commit-ci bot and others added 3 commits October 13, 2021 08:13

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

9b8b0f7

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

Update tests

6c9e173

Address reviews

729cbac

awaelchli approved these changes Oct 13, 2021

View reviewed changes

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved

pytorch_lightning/trainer/connectors/callback_connector.py Outdated Show resolved Hide resolved

Update pytorch_lightning/trainer/connectors/callback_connector.py

799fdf7

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

kaushikb11 enabled auto-merge (squash) October 13, 2021 09:37

kaushikb11 merged commit 28fc8d2 into Lightning-AI:master Oct 13, 2021

carmocca mentioned this pull request Nov 10, 2021

Update tests to avoid the deprecated weights_summary #10446

Merged

10 tasks

Borda mentioned this pull request Mar 10, 2023

Removed deprecated weights_summary argument from Trainer #13070

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `enable_model_summary` flag and deprecate `weights_summary` #9699

Add `enable_model_summary` flag and deprecate `weights_summary` #9699

ananthsub commented Sep 25, 2021 •

edited

Loading

rohitgr7 left a comment

codecov bot commented Sep 30, 2021 •

edited

Loading

kaushikb11 commented Sep 30, 2021 •

edited

Loading

ananthsub commented Sep 30, 2021 •

edited

Loading

daniellepintz commented Oct 4, 2021 •

edited

Loading

daniellepintz commented Oct 4, 2021

carmocca commented Oct 5, 2021

daniellepintz commented Oct 6, 2021

daniellepintz commented Oct 7, 2021

rohitgr7 left a comment

Add enable_model_summary flag and deprecate weights_summary #9699

Add enable_model_summary flag and deprecate weights_summary #9699

Conversation

ananthsub commented Sep 25, 2021 • edited 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?

rohitgr7 left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2021 • edited Loading

Codecov Report

kaushikb11 commented Sep 30, 2021 • edited Loading

ananthsub commented Sep 30, 2021 • edited Loading

daniellepintz commented Oct 4, 2021 • edited Loading

daniellepintz commented Oct 4, 2021

carmocca commented Oct 5, 2021

daniellepintz commented Oct 6, 2021

daniellepintz commented Oct 7, 2021

rohitgr7 left a comment

Choose a reason for hiding this comment

Add `enable_model_summary` flag and deprecate `weights_summary` #9699

Add `enable_model_summary` flag and deprecate `weights_summary` #9699

ananthsub commented Sep 25, 2021 •

edited

Loading

codecov bot commented Sep 30, 2021 •

edited

Loading

kaushikb11 commented Sep 30, 2021 •

edited

Loading

ananthsub commented Sep 30, 2021 •

edited

Loading

daniellepintz commented Oct 4, 2021 •

edited

Loading