Skip to content
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

fix counting of failures in PyTorch tests #2834

Merged

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Nov 25, 2022

(created using eb --new-pr)

The RegExp handling was wrong, e.g. for an output of:

SKIPPED [2] distributions/test_constraints.py:83: `biject_to` not implemented.
FAILED distributions/test_constraints.py::test_constraint[True-constraint_fn5-False-value5]
FAILED distributions/test_constraints.py::test_constraint[True-constraint_fn7-True-value7]
============= 2 failed, 128 passed, 2 skipped, 2 warnings in 8.66s =============
distributions/test_constraints failed!

the summary_matches_pattern2 will yield "2 failed, 128 passed, 2 skipped, 2 warnings in 8.66s" which does not match r"^.*[^0-9](?Pfailures[0-9]+) failed.*$" because there is nothing in front of the number.

This PR does a bit of refactoring to avoid this: Instead of creating summary_matches_pattern2 out of a RegEx match and using it in 2 places way below where the exact RegEx might be already forgotten I combined the counting of errors/failures with the summary-message generation and moved the RegEx right on top of the loop handling it and added comments with examples.

Also the anchors and named group is not required: r"^.*[^0-9](?Pfailures[0-9]+) failed.*$" -> r"([0-9]+) failed" which is much cleaner to read.

Finally my commit to also count tests terminated by a signal was lost so I readded it and reformatted the RegEx assignment to make it easier to read

And as a drive-by fix I added a docstring to get_count_for_pattern and made it return 0 instead of None for the case where the warning is shown.

easybuild/easyblocks/p/pytorch.py Outdated Show resolved Hide resolved
@Flamefire
Copy link
Contributor Author

Flamefire commented Nov 28, 2022

Fixed yet another issue and tried to refactor it a bit to avoid more: Using finditer returns match objects which we can use to access RegEx groups by name.

Also in a PyTorch PR there was a syntax error in one of the tests but EB reported "Test command had non-zero exit code (1), but no failed tests found?!"

Similar if there were even a single test which failed then those syntax errors would have been undetected. I hence added my RegEx back which searches for failed test suites and if that doesn't match the ones we checked by the 2 custom regexes it will be reported as an error not taking the number of tests into account. I also include the failed test names in the error message for that case.

Edit: And finally fixed the minor spelling issue where it reported "0 failure" (missing "s")

easybuild/easyblocks/p/pytorch.py Show resolved Hide resolved
easybuild/easyblocks/p/pytorch.py Show resolved Hide resolved
@branfosj
Copy link
Member

Test report by @branfosj

Overview of tested easyconfigs (in order)

  • SUCCESS PyTorch-1.12.1-foss-2021a.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
bear-pg0105u36b.bear.cluster - Linux RHEL 8.5, x86_64, Intel(R) Xeon(R) Platinum 8360Y CPU @ 2.40GHz (icelake), Python 3.6.8
See https://gist.github.com/b52144157585d00a1b8d71ba060cb0a7 for a full test report.

@branfosj
Copy link
Member

Going in, thanks @Flamefire!

@branfosj branfosj merged commit dcd4148 into easybuilders:develop Nov 29, 2022
@Flamefire Flamefire deleted the 20221125132425_new_pr_pytorch branch November 30, 2022 08:19
@boegel boegel changed the title Fix counting of failures in PyTorch tests fix counting of failures in PyTorch tests Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants