-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[testing] support op with args/kwargs in test_unary_ufunc #52194
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
[testing] support op with args/kwargs in test_unary_ufunc #52194
Conversation
💊 CI failures summary and remediationsAs of commit 0f1846f (more details on the Dr. CI page):
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
This looks reasonable to me. This was a tricky design, and this seems like a minimally invasive and clever way to enable clamp to run the unary ufunc tests. There is one real test failure that needs investigation, however. |
Test passes locally (with gcc 9.3). Will try to reproduce it locally (failed CI looks to use gcc5.4). Thanks! |
|
Skipping it on the earlier gcc sounds OK. We have some mechanisms for getting the gcc version in Python, although I'm not sure what the best one is: There may even be another mechanism I'm not familiar with. |
|
I tried building with Ubuntu Xenial VM (wih gcc 5.4 and python 3.6) but unable to reproduce the error (logs below). Is it okay to just skip the extremal test for bfloat16 directly. VM LogBuild Info using Test Log |
Sure, see my above comment. Can we add a skip that's only active if the gcc is 5.4 or earlier, though? |
|
What I meant was that gcc 5.4 version itself doesn't look to be the culprit (as tests pass with gcc5.4 and python3.6). So adding such skip might be deceiving 🤔 |
I see, thank you for clarifying. We can skip it in this PR but let's file and link an issue so we don't forget the history and in the future when people look at the skip they'll understand it and maybe fix it. |
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.
Cool!
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Here's the challenging follow-up question to this PR: What about the existing clamp tests? pytorch/test/test_shape_ops.py Line 274 in 7b939d9
I think most of them can be removed or at least reduced now, right? |
Codecov Report
@@ Coverage Diff @@
## master #52194 +/- ##
==========================================
- Coverage 77.44% 77.24% -0.20%
==========================================
Files 1892 1892
Lines 186297 186306 +9
==========================================
- Hits 144274 143912 -362
- Misses 42023 42394 +371 |
Ah right! Will have a look and push a PR to remove or simplify redundancy. Thanks!! |
Fixes #51242