-
Notifications
You must be signed in to change notification settings - Fork 54
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
Return KernelArgumentHolder instead of std::vector<at::Tensor> #3946
base: main
Are you sure you want to change the base?
Conversation
Return KernelArgumentHolder instead of std::vector<at::Tensor>
…ic_outs_step_8_tmp
3d3db12
to
d88fd8e
Compare
Review updated until commit ebe727b Description
Changes walkthrough 📝
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
!test |
!test |
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.
Looks good to me but I'll let others look before stamping. Should we next rename KernelArgumentHolder
since it is no longer only arguments but also kernel outputs? I don't know of a good term off hand: maybe something like KernelIOContainer
?
@@ -64,7 +64,7 @@ def get_python_tests(python_test_dir): | |||
|
|||
def get_test_timeout(test_name): | |||
"""Return timeout in seconds for a given test""" | |||
if test_name in ["test_nvfuser", "test_matmul", "test_ops"]: |
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 does test_name
include extension only for that one test?
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 don't follow you question.
test_nvfuser, test_matmul, and test_ops.py are the only three tests that take a very long time (well over 10 minutes), so we're just adjusting their timeouts accordingly.
No description provided.