-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add inplace kwarg to torch.nn.functional.silu
#570
Conversation
for more information, see https://pre-commit.ci
thunder/torch/__init__.py
Outdated
@@ -1486,7 +1486,8 @@ def selu(a: TensorProxy, /, inplace: bool = False) -> TensorLike: | |||
|
|||
|
|||
@torchsymbol(torch.nn.functional.silu) | |||
def silu(a, /): | |||
def silu(a: TensorLike, inplace: bool = False): | |||
utils.check(not inplace, lambda: "silu only supports inplace=False", exception_type=NotImplementedError) |
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.
I might be nice to include "Thunder" somewhere in the error message so people know it's us not them...
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.
Thank you @riccardofelluga
why is ref: lightning-thunder/thunder/torch/__init__.py Line 1459 in 8309fc0
|
Probably because it's terrible to pass this without keyword, but you're right, we should ideally match the PyTorch signature. |
Before submitting
What does this PR do?
Temporarily fixes #455 till we implement inplace operators by adding the keyword arg to the operator implementation.
I changed from testing the clang to testing the ltorch symbol as it is a superset of clang in this case so we test both.