-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add OpInfo test that tests meta functions binary ufuncs with different dtypes #113674
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
…t dtypes As per title [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/113674
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 1cb9f00 with merge base 51cbe78 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
…th different dtypes" As per title cc mruberry ZainRizvi [ghstack-poisoned]
…th different dtypes" As per title cc mruberry ZainRizvi [ghstack-poisoned]
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.
Some NITs and random thoughts, but looks good.
op = copy.copy(op) | ||
op.sample_inputs_func = sample_input | ||
|
||
self._run_dispatch_meta_test(device, dtype, op, symbolic_meta=True, inplace=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.
Would be cleaner to make the sample_inputs
generator an input to the test function, so we don't need to clone the opinfo object.
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.
The thing is that it's the function that is shared by all the tests, so I wanted to keep changes to the minimum
DecorateInfo(unittest.expectedFailure, | ||
'TestBinaryUfuncs', | ||
'test_type_promotion'), | ||
DecorateInfo(unittest.expectedFailure, 'TestMeta', 'test_binary_ufuncs_mixed_dtype'), |
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 wonder if we could factor test_type_promotion
into a helper and run it on meta devices as well? It seems to be a lot more comprehensive.
Or tbh it would be great if we could just run all the opinfo tests with a meta device...
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.
at the moment, the tests in test_meta.py
are written in a very particular way. I agree it would be better to refactor these in more OpInfo friendly.
This is a good onboarding task, I'll keep in mind.
…th different dtypes" As per title cc mruberry ZainRizvi [ghstack-poisoned]
…th different dtypes" As per title cc mruberry ZainRizvi [ghstack-poisoned]
Pull Request resolved: #113635 Approved by: https://github.com/Skylion007 ghstack dependencies: #113634, #113674
Stack from ghstack (oldest at bottom):
As per title
cc @mruberry @ZainRizvi