-
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
Always use trainer.call_hook
#8498
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8498 +/- ##
=======================================
- Coverage 93% 89% -4%
=======================================
Files 172 172
Lines 14093 14092 -1
=======================================
- Hits 13095 12514 -581
- Misses 998 1578 +580 |
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.
this will disable usage tracing in IDE, right?
Yes, this was already the case. Most hooks are already using If you have ideas about how to improve that, you can comment in #8506 |
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 ! Great work !
What does this PR do?
Always try to use
trainer.call_hook
to call hooks as we need it to set the_current_fx_name
attribute in theLightningModule
which is required for logging.Since
call_hook
also calls any trainer and accelerator methods with the same name, we need to skip them forsetup
andteardown
.A test is added which tries to
self.log()
in allLightningModule
andCallback
hooks to assert thatMisconfigurationException
is raised.Part of #7898
Follow-ups
call_hook
. The current pattern of always calling the accelerator and trainer hooks of the same name is dangerous. Also, we want the users to interact with it after loop customization. [RFC] Re-designcall_hook
interface #8506Does your PR introduce any breaking changes ? If yes, please list them.
None intended
Before submitting
PR review