Repository navigation
Add proper dtype checks for histc_out. - #5554
AKloniecki wants to merge 3 commits into
Conversation
Signed-off-by: Artur Kłoniecki <arturx.kloniecki@intel.com>
BBBela
left a comment
BBBela
left a comment
There was a problem hiding this comment.
Could you please add any description to this PR to have some context?
Does this change fix some failing test case or is it enhancement? If there is a failing test case that is fixed by this PR, then it would be good to somehow connect them, if not, then maybe a regression test should be added?
github-actions
Bot
added
disable_e2e
BBBela
left a comment
BBBela
left a comment
There was a problem hiding this comment.
Sorry, but for me the PR description is too minimalistic.
There is no information what test is failing, what is the plan. I think it should be in the description that this PR would make this test end with Unexpected Success, so there should be an issue with skipped label that will be resolved once the upstream PR removing expectedFailureXPU is merged.
BBBela
left a comment
BBBela
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
Although there is a problem caused by torch-xpu-ops and PyTorch dependencies. Merging this PR will make the mentioned test to fail (unexpected success) until the upstream one (pytorch/pytorch#199126) is merged. In torch-xpu-ops it is not a problem as there is already issue with skipped label, so it will not throw in CI. The problem would appear when this PR is merged and torch-xpu-ops commit pin is updated in PyTorch before pytorch/pytorch#199126 is merged.
I suppose, once this PR is merged into torch-xpu-ops we should try to merge pytorch/pytorch#199126 along with updating the torch-xpu-ops commit pin to avoid CI failures in PyTorch XPU CI.
@guangyey, @CuiYifeng - what do you think - can we do it that way? So once this PR is merged, we bump the torch-xpu-ops commit pin along with removing expectedFailureXPU in this PR: pytorch/pytorch#199126?
dvrogozh
left a comment
dvrogozh
left a comment
There was a problem hiding this comment.
@AKloniecki , that's important to check what work was done already in the upstream and if anyone has attempted to fix what you are about to do for the XPU. Running git blame for the upstream ./aten/src/ATen/native/cuda/SummaryOps.cu you will see that this was done for CPU and CUDA in the following PR:
Which is linked to the issue:
Reviewing above link, there is the following XPU side issue and PR opened by @N0AHZACH:
#5380 has a preference as it was opened earlier. Plus it has changes to the tests which are missing in your PR. @AKloniecki , please, help to review #5380 and provide your comments.
My expectation is that PR to update torch-xpu-ops will take care of the above, i.e. either 199126 itself should contain commit pin update or other PR updating the pin should contain a fix from 199126. |
4 participants
histc_outcontract expects proper validation of out tensor dtype.Currently there are
histc_outtests upstream in pytorch/test/test_reductions.py which are xfailed due to lack of proper dtype handling on our side.PR removing xfails upstream: pytorch/pytorch#199126
This PR, correcting histc_out behaviour, causes upstream xfails to be reported as unexpected success, which is why #5570 has been oppened and should be closed when both this and then upstream PRs are merged.
Fixes: pytorch/pytorch#196008
Fixes: #5237
Relates-to: pytorch/pytorch#196100