-
Notifications
You must be signed in to change notification settings - Fork 285
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
Correctly count the number of failing tests (not failing test suites) in PyTorch builds #2794
Correctly count the number of failing tests (not failing test suites) in PyTorch builds #2794
Conversation
… Improve error reporting to provide more insight into which test suites fail
…sybuild-easyblocks#2794 we'll actually start counting failing tests, instead of failing test suites. Thus, much higher numbers can be expected, since many test suites have multiple failing tests
… read overview of failing tests etc than what is logged in the logfile by default.
Two test report for this PR can be found in this easyconfig PR here and here |
@casparvl We will likely need to increase |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) edit:
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) edit:
|
@casparvl Test failures are still very low for That does raise questions about the much higher failing test count in easybuilders/easybuild-easyconfigs#15924 though? |
Test report by @casparvl Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) |
@boegel Two reasons for that:
Note that there are other errors that occur quite a large number of times. You're right in that we should probably investigate those a bit further. One of them being the |
Thanks for clarifying @casparvl! Since this enhanced PyTorch easyblock has been tested both here (with PyTorch 1.11) and in #15924 for Python 1.12, I don't see a reason to hold back this PR further... |
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.
lgtm
(created using
eb --new-pr
)Previously, the PyTorch EasyBlock would count the number of test that were run (around 86k) and the number of test suites that failed (typically 10 or 15 or so). It presented the latter as if this was the number of failing tests, which would make it seem absolutely negligible. That is not a fair comparison however, as each test suite contains many tests, and if a test suite failed, it usually contains multiple failing tests.
This PR specifies more specific regex patterns to grab for in the output, to get the actual amount of individual tests that fail. With e.g. this run that now amounts to around 200 failures/errors. Compared to 86k total tests, that's still acceptable, and doesn't point to any fundamental issues with the PyTorch build - it merely reflects broken tests.
The printed warning is now also more clear, to help take the end-user with the decision if this amount of errors is acceptable or not. E.g. 200 test failures, distributed evenly over 15 or so test suites might be fine. But 200 test failures in a single test suite might point to one particular functionality of PyTorch being completely broken.
To make this concrete, with this PR, the standard output will look as follows: