-
Notifications
You must be signed in to change notification settings - Fork 100
Add ops for HF transformers #2217
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
base: main
Are you sure you want to change the base?
Conversation
Hi @kshitij12345 , @IvanYashchuk could you take a look of the PR |
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, thanks @kiya00.
Hi @IvanYashchuk @mruberry , could you take a look at the PR |
Co-authored-by: Masaki Kozuki <[email protected]>
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.
@t-vi, could you please merge this PR?
@@ -1177,7 +1179,7 @@ def movedim(a: TensorLike, /, source: int | Sequence[int], destination: int | Se | |||
return clang.movedim(a, source, destination) | |||
|
|||
|
|||
@torchsymbol(torch.nn.functional.pad) | |||
@torchsymbol(torch.nn.functional.pad, torch._C._nn.pad, id="torch.nn.functional.pad", is_method=False) |
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.
Why are we calling torch._C._nn.pad directly?
Isn't is_method=False
the default?
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 is for ThunderFx path, it appears in the dynamo graph as torch._C._nn.pad
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 can remove the is_method=False, keep it here explicit just because I saw a lot of other ops do the same
As identified in the split reasons investigated in #2169 (https://docs.google.com/spreadsheets/d/1BKK-1T4UqZHaE8x39gIRFpoCuDmMFC3Z3rn_mBWUStA/edit?usp=sharing), this PR addresses several of them to help reduce the number of segments:
Adds support for
torch._C._nn.pad
, which is functionally equivalent totorch.nn.functional.pad
.(Reference: PyTorch source),
Registers some operators that are aliases of already-registered ones.
Introduces checks for einops operators in the splitter.
(These operators are registered here)
Fixes #2169.