Automatically add lightning hooks to CellariumModule as passthroughs#271
Automatically add lightning hooks to CellariumModule as passthroughs#271sjfleming wants to merge 6 commits into
Conversation
| def delegate_method(self, *args, **kwargs): | ||
| method = getattr(self.model, method_name, None) | ||
| if callable(method): | ||
| method(self.trainer) |
There was a problem hiding this comment.
Pass *args and **kwargs here as well.
There was a problem hiding this comment.
So I did try this initially:
method(self.trainer, *args, **kwargs)but I got unexpected test failures...
then I realized that the explicitly implemented ones only used the self.trainer arg.
There was a problem hiding this comment.
Should we be passing args and kwargs?
There was a problem hiding this comment.
I admit I did not understand the test failures. It seemed like something funny was happening during "predict". I gave up on args and kwargs for that reason, thinking it must be wrong. But I never understood the test failure.
| # >>> explicitly_implemented_hooks = ["on_train_epoch_start", "on_train_epoch_end"] | ||
| # >>> omit_hooks = ["on_after_batch_transfer", "on_before_batch_transfer", "on_gpu"] | ||
| # >>> passthrough_hooks = [hook for hook in all_hooks if hook not in (explicitly_implemented_hooks + omit_hooks)] | ||
| passthrough_hooks = [ |
There was a problem hiding this comment.
Would be nice this was automated like above and robust. For example automatically adding explicitly implemented hooks as well.
There was a problem hiding this comment.
I was kind of thinking that as well, but I'm not sure. On one hand, it's nice if it's automatic, but on the other hand, the code is a bit easier to read -- easier for developers to see what is actually implemented -- if it's explicit.
What do you think?
| if callable(on_train_start): | ||
| on_train_start(self.trainer) | ||
| on_train_epoch_start = getattr(self.model, "on_train_epoch_start", None) | ||
| if callable(on_train_epoch_start): |
There was a problem hiding this comment.
Would be nice to somehow call the delegate_method here so if there are any changes in it this wouldn't need any changes.
There was a problem hiding this comment.
I am not sure if I understand this point. I was thinking that we could not re-implement the explicitly implemented methods (like on_train_epoch_start) because the passthrough/delegate would end up having the same name and they would conflict (I don't know how python handles that).
2 participants
The idea here is to add a whole bunch of lightning hooks to CellariumModule so that CellariumModule can pass them all through to CellariumModel. Different CellariumModels may want to use different hooks.
Closes #226