-
Notifications
You must be signed in to change notification settings - Fork 75
Use meta device tensor to infer contiguity for expr-eval segments #5772
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
Changes from all commits
473191d
ffd6864
69a65a8
5a784d4
d828c9f
c95a584
074e947
6bcdb96
4ad3785
c0b50c3
7ffdaa3
7a5b0dc
d92e5ee
074209b
68d52fb
fb0572a
08e5b6b
784ce68
9c46183
93a4012
e5d4d67
38defa9
f50e52f
5fd7496
40782db
53d70fe
87b00e8
4afe5b1
447de33
dd41424
831c777
d246fc6
01a011c
03fa1f5
0a2878b
6e3b29e
72c66dd
b151b50
4249f8b
40b5411
b7778bd
c81f895
8466489
1500e69
7c62ef3
fb95173
f87a0ca
9665cf0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -497,6 +497,9 @@ TEST_F(AliasTest, Issue1452) { | |
| } | ||
|
|
||
| TEST_F(AliasTest, AliasOutputBeforeNonAliasOutput) { | ||
| EnableOptionsGuard opt_guard; | ||
| EnableOptionsGuard::getCurOptions().unset(EnableOption::InferContiguity); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a no-op? AliasTest doesn't seem to enable InferContiguity.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AliasTest is NVFuserTest, which enables InferContiguity.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it -- I didn't realize NVFuserTest enables the option by default. Then, why do some tests enable it again, e.g., https://github.com/NVIDIA/Fuser/pull/5772/files#diff-3675636f2228bd2f8c3f308c28fa88f1d659d8eb3d869570dcfdf013f77908aaR29? |
||
|
|
||
| auto fusion = std::make_unique<Fusion>(); | ||
| FusionGuard fg(fusion.get()); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -972,7 +972,13 @@ TEST_F(BlockQuantizationValidationTest, MergesMustBeContiguous) { | |
| class BlockQuantizationSchedulingTest | ||
| : public BlackwellBase, | ||
| public ::testing::WithParamInterface< | ||
| std::tuple<DataType, std::pair<int, int>, bool, bool>> {}; | ||
| std::tuple<DataType, std::pair<int, int>, bool, bool>> { | ||
| protected: | ||
| void SetUp() override { | ||
| BlackwellBase::SetUp(); | ||
| EnableOptionsGuard::getCurOptions().unset(EnableOption::InferContiguity); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://google.github.io/googletest/faq.html#CtorVsSetUp Prefer constructor
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for sharing. I think we should migrate our entire codebase to prefer ctor, instead of just this specific test. I don't think it is a good idea to mix ctor and SetUp. Because NVFuserTest setup InferContiguity and everything else in SetUp, we need to be consistent here, because otherwise whatever we set in ctor will be overriden by
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I actually did that for NVFuserTest. In fact, most setup code for NVFuserTest is in its constructor already except this line (https://github.com/NVIDIA/Fuser/pull/5772/files#diff-16f891fd5f846480392227c6bbf81ead352f59fdc9964e5d6e4dc6089bb622c5R61) which was added later without my notice. The only thing that should be kept in NVFuserTest::SetUp at this moment is GTEST_SKIP. |
||
| } | ||
| }; | ||
|
|
||
| TEST_P(BlockQuantizationSchedulingTest, AutoScheduleSingleOp) { | ||
| const auto data_type = std::get<0>(GetParam()); | ||
|
|
||
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.
logic: no error handling for
evaluate()failure - if evaluation fails or returns an invalid result, it will silently continueConsider validating the result or wrapping in try-catch, especially since the PR description mentions some ATen ops on meta device can hang due to GIL issues