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
Delete stub forward() method in LightningModule #19423
base: master
Are you sure you want to change the base?
Conversation
The current stub breaks TorchScript/`to_torchscript` with: ``` otSupportedError: Compiled functions can't take variable number of arguments or use keyword-only arguments with defaults: File "/opt/conda/envs/image-gen/lib/python3.11/site-packages/pytorch_lightning/core/module.py", line 657 def forward(self, *args: Any, **kwargs: Any) -> Any: ~~~~~~~ <--- HERE r"""Same as :meth:`torch.nn.Module.forward`. ``` This becomes an issue when you use a module as container for other modules with bespoke methods and do not rely on the default forward call.
Hey @BlackHC |
Well, to be fair, the stub is not the problem by itself. The problem is that you are using the wrong signature for the stub. The correct default signature as defined is I think |
for more information, see https://pre-commit.ci
This stub is only defined so it appears in the docs. Removing kwargs will mean this now raises class MyModel(LightningModule):
def forward(self, *inputs, **kwargs):
return super().forward(*inputs, **kwargs)
m = MyModel()
y = m("foo", bar="bar") TypeError: LightningModule.forward() got an unexpected keyword argument 'bar' Which before would've raised TypeError: _forward_unimplemented() got an unexpected keyword argument 'bar' This isn't a big deal since this is meant to always be overridden and the error type is the same. However, I am worried about your statement:
All computations are intended to run inside |
To be precise, actually we only use a module as a container module and then access its submodules. (The advantage over ModuleDict is that you get nicer code completion etc.) However, looking at the tests, torchscript does not like *inputs either :| That makes me wonder how it runs on a regular Torch Module in the first place (but that works as the fix showed). The alternative is, I guess, to copy the _forward_unimplemented approach. I'll try that next. |
The current stub breaks TorchScript/
to_torchscript
with:This becomes an issue when you use a module as container for other modules with bespoke methods and do not rely on the default forward call.
What does this PR do?
Fixes #<issue_number>
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Reviewer checklist
📚 Documentation preview 📚: https://pytorch-lightning--19423.org.readthedocs.build/en/19423/