-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix test_sparse_addmm_...float16 and test_sparse_matmul_...float16 test failures #73155
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
…st failures [ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
|
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 32173a2 (more details on the Dr. CI page):
🕵️ 2 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
Job | Step | Action |
---|---|---|
Unknown | 🔁 rerun |
This comment was automatically generated by Dr. CI (expand for details).
Please report bugs/suggestions to the (internal) Dr. CI Users group.
un-requesting myself as I will be out for the week |
Bumping up the relative and absolute error limits is far from satisfying to resolve issues like this. Maybe we should look into seeding our tests so that we always run on the same data, or at least pick a seed at random and record it so we can create deterministically reproducible failures. |
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
I agree these issues of flaky tests are annoying. There is @mruberry, any advice on the method of resetting the random seed for a particular set of tests or avoiding seed-related inference between unrelated tests? |
@pearu - Merging PRs has been a bit hellish. Could you try rebasing this stack please? |
….float16 test failures" Fixes #73145 Differential Revision: [D34398935](https://our.internmc.facebook.com/intern/diff/D34398935) [ghstack-poisoned]
@pearu - I agree. We should have infrastructure to that allows developers to opt-into seeded tests. But we have to be careful to not encourage seed hacking. You can try a thousand seeds and it'll eventually succeed. |
Yeah, the solution should reverse this situation: one can try a thousand seeds and it'll eventually fail. How about this, if one wants to specify the seed manually for testing then they must specify, say, at least 3 different seed values. |
@pearu - I was thinking we used a fixed seed as a fallback when a test fails and note down the seed and failing test to create an issue. That way we can investigate without blocking development. The fixed seed with very strict atol and rtol will still prevent gross mistakes, but we can subsequently investigate whether the flakiness is random or due to some underlying change. |
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@pearu - could you try rebasing this again? |
….float16 test failures" Fixes #73145 Differential Revision: [D34398935](https://our.internmc.facebook.com/intern/diff/D34398935) [ghstack-poisoned]
@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…st failures (#73155) Summary: Pull Request resolved: pytorch/pytorch#73155 Fixes #73145 Test Plan: Imported from OSS Reviewed By: mikaylagawarecki Differential Revision: D34398935 Pulled By: cpuhrsch fbshipit-source-id: b1e852f25b0888b37d9c9c1418ddf344ac8f0a04 (cherry picked from commit d63c977fb39c7dcb3f3d083edc4b25cd2d6c2ec4)
…st failures (#73155) Summary: Pull Request resolved: pytorch/pytorch#73155 Fixes #73145 Test Plan: Imported from OSS Reviewed By: mikaylagawarecki Differential Revision: D34398935 Pulled By: cpuhrsch fbshipit-source-id: b1e852f25b0888b37d9c9c1418ddf344ac8f0a04 (cherry picked from commit d63c977fb39c7dcb3f3d083edc4b25cd2d6c2ec4)
Fixes #73145
Stack from ghstack (oldest at bottom):
Differential Revision: D34398935