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
enable dispatch stub for backend PrivateUse1 #99611
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/99611
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit bf93485: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@bdhirsh Please help me review this commit, thank you |
aten/src/ATen/native/DispatchStub.h
Outdated
@@ -170,6 +172,10 @@ struct DispatchStub<rT (*)(Args...), T> { | |||
impl.mps_dispatch_ptr = reinterpret_cast<void*>(fn_ptr); | |||
} | |||
|
|||
void set_private_use1_dispatch_ptr(FnPtr fn_ptr) { |
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.
nit: can you make it set_privateuse1_dispatch_ptr
(for greppability)
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.
done
One comment: I don't think getting DispatchStub to work with externally backends, is strictly necessary, since for any ATen kernels that are implemented with it, you can always register your own dedicated kernel to the PrivateUse1 dispatch key for that operator. Is the idea that you want to be able to re-use some kernels from ATen that are implemented in terms of dispatch stub? That sounds pretty useful, I just wanted to confirm. If so, it would be good to add a test. Do you think you can add a DispatchStub handler in |
d217ca1
to
a287040
Compare
yes,that's exactly what I want to do, "re-use some kernels" through dispatch stub。 |
} | ||
} | ||
|
||
REGISTER_PRIVATEUSE1_DISPATCH(abs_stub, &abs_kernel); |
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.
it would be nice to be able to test that we actually ran this kernel (right now the "test" just calls abs()
and checks that the outputs match).
One way to do it would be the way it's done in custom_add_Tensor
below - add a global counter for whether our custom abs kernel was called, that we increment inside of the kernel. Then expose a custom_abs_called
helper to python that you can assert in the python code.
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 for the suggestion. I have modified the unit test according to your suggestion, added custom_abs_called
and abs_counter
in test.
bde88cf
to
effa7de
Compare
Can you take a look at the failing CI? It looks like the custom cpp extension is failing to be compiled:
|
c10::IntArrayRef size, | ||
c10::optional<c10::MemoryFormat> optional_memory_format) { | ||
// Since this custom device is just for testing, not bothering to implement kernels. | ||
return self; |
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.
it looks like it's failing because custom_resize_()
is already defined further down.
You can just use the existing one instead of defining this new one.
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.
ok, I have updated the code
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.
all checks have passed, could this PR be merged?
9253ca6
to
ce72f84
Compare
@pytorchbot merge |
This PR needs to be approved by an authorized maintainer before merge. |
6099fdc
to
aba8401
Compare
This PR need to be approved by an authorized maintainer before merge. Could you approve this PR? @bdhirsh |
@bdhirsh Could you approve this PR? Thank you |
@ezyang Could you review and approve this PR? Thank you |
Sorry, y'all have already gone a review on this, but I want to put the brakes on this. The dispatch stub API is not intended for external users, and for the most part we've expunged all the cases where we registered kernels that call into stubs so that they don't CompositeExplicitAutograd anymore. Concretely, what operators do you think you need this for? |
Actually I want to extend a new hardware backend based on "PrivateUse1" key. And I want to use the dispatchstub mechanism to call my kernels. such as: https://github.com/pytorch/pytorch/blob/main/aten/src/ATen/native/UnaryOps.cpp#L446 |
Are you really using TensorIterator to implement your kernels though? Seems... unlikely. |
yes, I actually used TensorIterator.
PrivateUse1 key is just like "CPU" key, "CUDA" key. I think it is necessary to support privateuse1 in dispatchstub. |
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.
ok fine
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase and merge by leaving the following comment on this PR: Details for Dev Infra teamRaised by workflow job |
@pytorchbot merge -r |
@pytorchbot successfully started a rebase job. Check the current status here |
Successfully rebased |
aba8401
to
bf93485
Compare
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
@pytorchbot merge |
The merge job was canceled. If you believe this is a mistake,then you can re trigger it through pytorch-bot. |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
When expanding the new backend of pytorch in the form of out ot tree, Privateuse1 will be reused. So we also need to support PrivateUse1 in the dispatch stub module
cc @bdhirsh