Repository navigation
Conversation
guangyey
left a comment
guangyey
left a comment
There was a problem hiding this comment.
Overall LGTM. Could you please add a test to test/regressions/
69ffe97 to
335ec76
Compare
github-actions
Bot
added
disable_e2e
335ec76 to
70030ea
Compare
|
please ensure the ut pass. |
|
@torchxpubot ut-check |
UT Result Check: PR #5241New Failures36 new failure(s) detected (not in known issues). Listed below with the PR-related ones first; see the truncation note for the remainder.
... and 16 more failure(s). See CI logs for the full list. All 16 are Unrelated: 6 more Failure Relevance AnalysisRelated (3) -- the PR's own new tests. All three fail at the same point with Unrelated (33), grouped by shared root cause:
None of these touch RReLU, activation kernels, or the files this PR changes. New Test CoverageThis PR adds/modifies 3 test(s): 0 passed, 3 failed, 0 skipped, 0 not run.
RecommendationNot safe to merge. All three newly added tests in Custom skills applied: ut-check. Generated by |
|
@cyyever The CI failure seems to be related. |
_rrelu_with_noise_xpu_train writes self.numel() distinct values into noise directly. Nothing checked that noise's shape matches self's, or that noise is contiguous; a non-contiguous noise (e.g. an expanded view) got a noise_.contiguous() temporary written into instead and discarded, so the caller's noise tensor silently kept its original values. Add the same shape check the CPU kernel has, and require noise be contiguous so the kernel can write into it directly. CUDA checks neither: its train kernel still writes into a discarded noise_.contiguous() temporary, so a non-contiguous noise silently keeps its old values there as well. Co-Authored-By: Claude Code <noreply@anthropic.com>
Exercises the checks added in the previous commit: shape-mismatched and non-contiguous noise must raise, and a contiguous noise must be written by the training kernel. No XPU device locally; verified with ruff: ``` ruff check test/regressions/test_rrelu_with_noise.py ruff format --check --line-length 120 test/regressions/test_rrelu_with_noise.py ``` Co-Authored-By: Claude Code <noreply@anthropic.com>
Materializing non-contiguous noise introduced an unchecked copy_ back into the caller's tensor. An expanded noise aliases the same storage across elements, so the copy silently wrote distinct values through overlapping memory instead of failing as intended. Test Plan: pytest test/regressions/test_rrelu_with_noise.py -v
a198456 to
0cb8f03
Compare
|
@guangyey Fixed |
|
@torchxpubot review |
|
@torchxpubot ut-check |
Confirmed the key fact from upstream. Here is the report. UT Result Check: PR #5241New Failures28 new failure(s) detected (not in known issues): 3 Related, 25 Unrelated.
... and 8 more failure(s), all in Failure Relevance AnalysisRelated (3) — the PR's own new tests, all failing with the same error:
This is a test-side API error, not a kernel defect. Verified against upstream The practical consequence: the three tests fail at attribute lookup before touching the device, so the kernel change in Unrelated (25) — environment and upstream-test issues, three clusters:
None of these touch rrelu, activation kernels, or the regression-test harness. Data note: the New Test CoverageThis PR adds/modifies 3 test(s): 0 passed, 3 failed, 0 skipped, 0 not run.
RecommendationNot safe to merge. Every test this PR adds fails, so the Generated by |
|
op_regression,third_party.torch-xpu-ops.test.regressions.test_rrelu_with_noise.TestRreluWithNoise,test_noise_non_contiguous |
4 participants
_rrelu_with_noise_xpu_train writes self.numel() distinct values into noise directly. Nothing checked that noise's shape matches self's, and for a non-contiguous noise (e.g. an expanded view) the kernel's
.contiguous()temporary was written into instead and discarded, so the caller's noise tensor silently kept its original values with no error.Adds the same shape check the CPU kernel already has, and writes noise through a contiguous temporary that is copied back into the caller's noise when it isn't contiguous -- mirroring the output handling already in this kernel and the CPU fix in pytorch/pytorch#196087.
Test Plan: no XPU device available where this was written; offered for CI.
This PR was authored with the assistance of an AI coding assistant.