add DETAIL logs for batch use cases #11008

edward-io · 2021-12-09T06:29:01Z

What does this PR do?

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

None

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/checkpoint_connector.py

tchaton

Looks great !

pytorch_lightning/__init__.py

pytorch_lightning/plugins/training_type/ddp.py

pytorch_lightning/__init__.py

pytorch_lightning/trainer/trainer.py

pytorch_lightning/trainer/callback_hook.py

carmocca

Requesting changes for a final review when it's all ready :)

pytorch_lightning/__init__.py

pytorch_lightning/loops/fit_loop.py

pytorch_lightning/strategies/ddp.py

pytorch_lightning/trainer/trainer.py

tests/utilities/test_warnings.py

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

awaelchli · 2022-01-16T14:54:44Z

pytorch_lightning/trainer/trainer.py

+        log.detail(f"{self.__class__.__name__}: restoring training state")
        self.checkpoint_connector.restore_training_state()


I disagree with most changes in this PR. Instead of moving the log calls into the actual methods, this is now littering the trainer with more code that is irrelevant to the reader. Our goal is to make the Trainer more readable and easier to trace steps for contributors and researchers who are curious. This PR is going in the wrong direction IMO.
Please reconsider this, I feel very strongly about this.

cc @PyTorchLightning/core-lightning

The choice of logging in the caller vs the callee is as old as logging has existed and it is a tradeoff between flexibility vs convenience.

I don't think these log calls make the Trainer unreadable, as they are just glorified prints. You could see their value as a replacement for comments.

Please reconsider this, I feel very strongly about this.

However, I don't feel very strongly about it, so if you do, I'm okay with changing these.

edward-io requested review from awaelchli, Borda, carmocca, justusschock, kaushikb11, rohitgr7, SeanNaren, tchaton and williamFalcon as code owners December 9, 2021 06:29

ananthsub reviewed Dec 9, 2021

View reviewed changes

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

edward-io force-pushed the add-detail-logs branch from af1d345 to a1d794b Compare December 9, 2021 07:10

mergify bot added the has conflicts label Dec 9, 2021

edward-io force-pushed the add-detail-logs branch from a1d794b to 4e0ece5 Compare December 9, 2021 07:25

mergify bot removed the has conflicts label Dec 9, 2021

tchaton approved these changes Dec 9, 2021

View reviewed changes

ananthsub approved these changes Dec 9, 2021

View reviewed changes

pytorch_lightning/__init__.py Outdated Show resolved Hide resolved

mergify bot added the ready PRs ready to be merged label Dec 9, 2021

ananthsub changed the title ~~add VERBOSE logs for production use cases~~ add VERBOSE logs for batch use cases Dec 9, 2021

ananthsub reviewed Dec 10, 2021

View reviewed changes

pytorch_lightning/plugins/training_type/ddp.py Outdated Show resolved Hide resolved

pytorch_lightning/plugins/training_type/ddp.py Outdated Show resolved Hide resolved

pytorch_lightning/__init__.py Outdated Show resolved Hide resolved

carmocca reviewed Dec 14, 2021

View reviewed changes

pytorch_lightning/__init__.py Outdated Show resolved Hide resolved

Borda requested a review from carmocca December 17, 2021 18:08

mergify bot added the has conflicts label Dec 22, 2021

edward-io force-pushed the add-detail-logs branch from 1bc4396 to bb72db5 Compare December 23, 2021 00:53

mergify bot removed the has conflicts label Dec 23, 2021

edward-io changed the title ~~add VERBOSE logs for batch use cases~~ add DETAIL logs for batch use cases Dec 23, 2021

ananthsub reviewed Dec 25, 2021

View reviewed changes

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

pytorch_lightning/trainer/callback_hook.py Outdated Show resolved Hide resolved

mergify bot added the has conflicts label Dec 28, 2021

carmocca requested changes Dec 31, 2021

View reviewed changes

pytorch_lightning/__init__.py Outdated Show resolved Hide resolved

mergify bot removed the ready PRs ready to be merged label Dec 31, 2021

edward-io force-pushed the add-detail-logs branch from 936a97a to 80c19d0 Compare January 6, 2022 00:26

mergify bot removed the has conflicts label Jan 6, 2022

edward-io requested a review from carmocca January 6, 2022 00:30

ananthsub reviewed Jan 6, 2022

View reviewed changes

pytorch_lightning/__init__.py Outdated Show resolved Hide resolved

pytorch_lightning/__init__.py Outdated Show resolved Hide resolved

carmocca reviewed Jan 6, 2022

View reviewed changes

edward-io added 6 commits January 10, 2022 20:53

add VERBOSE logging level

19cc683

rename verbose -> detail and extra logging

e9b0f5d

update trainer.py

f655e5b

remove callback_hook.py changes

bf93f74

add log test

c6c94f6

add classname to logs, rename DETAIL -> _DETAIL

44d46e3

edward-io force-pushed the add-detail-logs branch from 010315c to 44d46e3 Compare January 11, 2022 04:53

fix pep8

f0a97ba

edward-io force-pushed the add-detail-logs branch from d63b7fb to f0a97ba Compare January 11, 2022 21:01

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

e5a2e13

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

edward-io requested a review from carmocca January 11, 2022 23:13

carmocca approved these changes Jan 12, 2022

View reviewed changes

carmocca added this to the 1.6 milestone Jan 12, 2022

mergify bot added the ready PRs ready to be merged label Jan 12, 2022

carmocca added feature Is an improvement or enhancement and removed ready PRs ready to be merged labels Jan 12, 2022

mergify bot added the ready PRs ready to be merged label Jan 12, 2022

carmocca merged commit 6107ce8 into Lightning-AI:master Jan 12, 2022

awaelchli reviewed Jan 16, 2022

View reviewed changes

carmocca mentioned this pull request Feb 9, 2023

Remove custom log events #16707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add DETAIL logs for batch use cases #11008

add DETAIL logs for batch use cases #11008

edward-io commented Dec 9, 2021

tchaton left a comment

carmocca left a comment

awaelchli Jan 16, 2022 •

edited

awaelchli Jan 16, 2022

carmocca Jan 18, 2022

		log.detail(f"{self.__class__.__name__}: restoring training state")
		self.checkpoint_connector.restore_training_state()

add DETAIL logs for batch use cases #11008

add DETAIL logs for batch use cases #11008

Conversation

edward-io commented Dec 9, 2021

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?

tchaton left a comment

Choose a reason for hiding this comment

carmocca left a comment

Choose a reason for hiding this comment

awaelchli Jan 16, 2022 • edited

Choose a reason for hiding this comment

awaelchli Jan 16, 2022

Choose a reason for hiding this comment

carmocca Jan 18, 2022

Choose a reason for hiding this comment

awaelchli Jan 16, 2022 •

edited