[IPU] Call accelerator hooks regardless if LM hook overridden 1/n #7826
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7826 +/- ##
======================================
- Coverage 93% 92% -0%
======================================
Files 202 202
Lines 13042 13050 +8
======================================
- Hits 12077 12047 -30
- Misses 965 1003 +38 |
| # if the PL module doesn't have the hook then call the accelerator | ||
| # used to auto-reduce things for the user with Results obj |
There was a problem hiding this comment.
one example is training_step_end for DP which is handled in the plugin. If user doesn't do the reduction we do it in the accelerator. But now with the change, if the user has it overridden you still call training_step_end on the DP plugin which will do another automatic reduction.
| @@ -101,10 +102,16 @@ def predict_step(self, *args, **kwargs): | |||
| return self.model(*args, **kwargs) | |||
|
|
|||
| def training_step_end(self, output): | |||
| return self.reduce(output) | |||
| if not is_overridden("training_step_end", self.lightning_module): | |||
There was a problem hiding this comment.
@SeanNaren this check for DP should take care of the previous rule we had: only auto reduce if user did not do it themself.
SeanNaren
requested review from
Borda,
carmocca,
justusschock,
kaushikb11,
tchaton and
williamFalcon
as code owners
SeanNaren
changed the title
SeanNaren
changed the title
SeanNaren
changed the title
SeanNaren
changed the title
Co-authored-by: Carlos Mocholí <carlossmocholi@gmail.com>
| # used to auto-reduce things for the user with Results obj | ||
| elif hasattr(self.accelerator, hook_name): | ||
| # call the accelerator hook | ||
| if hasattr(self.accelerator, hook_name): |
There was a problem hiding this comment.
I feel less comfortable adding more dependencies to call_hook. this glues together calling code across 3 distinct interfaces: callback hooks, model hooks, and accelerator hooks. what happens if one takes different arguments? or if we need to call them in different orders?
separately (unrelated to this PR) we're aggregating all of these hooks together in the profiling results. the model/callbacks/accelerator could all have different behavior. i think it's worth splitting out these into separate profiling sections to give provide more fine-grained visibility
There was a problem hiding this comment.
I agree, the accelerator hooks shouldn't be a part of call_hook.
Also agree on the profiling comment
There was a problem hiding this comment.
thanks @ananthsub!
For the IPU integration we'll be moving as is, but after I want to remove this logic for the accelerators from this call, and call them explicitly through the loop functions as this can foster some nasty bugs as you suggested. Does that sound fair?
There was a problem hiding this comment.
@SeanNaren that sounds good!
What does this PR do?
When defining a new accelerator, I noticed that some hooks were not called by the Trainer unless if the hook was overridden in the LightningModule.
We now call all hooks, and the edge case for DP where a reduction is necessary if the user doesn't do it themselves is now handled in the DP plugin. I don't think any tests were required, as all hooks are already tested.
Before submitting
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:
Did you have fun?
Make sure you had fun coding 🙃