-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Move logger and profiler finalization to trainer's teardown #8685
Move logger and profiler finalization to trainer's teardown #8685
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8685 +/- ##
=======================================
- Coverage 93% 89% -4%
=======================================
Files 169 169
Lines 14072 14071 -1
=======================================
- Hits 13043 12464 -579
- Misses 1029 1607 +578 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be sure to update the changelog as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update the changelog with this fix as well: https://github.com/PyTorchLightning/pytorch-lightning/blob/master/CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tangbinh - could you also add a test that confirms that the logger is finalized when calling trainer.fit, trainer.validate, trainer.test, and trainer.predict?
you can see tests/trainer/logging_/test_distributed_logging.py & https://github.com/PyTorchLightning/pytorch-lightning/pull/8608/files for examples of integration tests between the trainer and loggers.
you can create a dummy logger or mock and confirm that finalize is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM :) thanks for working on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please address @awaelchli 's comment on comments as well
What does this PR do?
Previously, profiler finalization was implemented in multiple places, including the evaluation loop, prediction loop, and fit loop. Logger finalization, however, was included in the fit loop only, leading to Issue #8333.
Here, we move the logger and profiler finalization to the
_call_teardown_hook
in the trainer and call the hook from each process in case of DDP or TPU training. See #8217 for more contexts and discussions (cc @ananthsub).Does your PR introduce any breaking changes? If yes, please list them.
None
Before submitting
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:
Did you have fun?
Make sure you had fun coding 🙃