-
Notifications
You must be signed in to change notification settings - Fork 748
Use default overloads when calling custom ops #7836
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7836
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 Cancelled Jobs, 1 Unrelated FailureAs of commit b9a2862 with merge base e78ed83 ( CANCELLED JOBS - The following jobs were cancelled. Please retry:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
CI Failure unrelated. |
digantdesai
left a comment
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.
IIUC
executorch/exir/verification/verifier.py
Line 68 in 31f35d9
| return super().allowed_op_types() + (torch._ops.OpOverloadPacket,) |
If yes, this is OK.
I think executorch/exir/verification/verifier.py Line 239 in 31f35d9
|
b9a2862 to
3eea1f1
Compare
If a node is created without specifying an overload, A OpOverloadPacket is created, rather than an OpOverload. This works in a GraphModule, but the OpOverloadPacket is not a valid operator type in the _EXIREdgeDialectVerifier, which means that Edge ExportedPrograms can't contain a GraphModule with such ops.
In short, specifying using the default overload seems to be the more correct way of calling a custom
operator.
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218