From d4afafd40ab24f25aac944ec367e6a62cafdf00e Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 13 Mar 2024 11:04:24 +0100 Subject: [PATCH 01/12] Explicitely mention that the PyTorch easyblock needs updating when failing for this reason --- easybuild/easyblocks/p/pytorch.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index 164d3b9376e..467f74669d4 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -497,9 +497,16 @@ def test_step(self): if failed_test_suites != all_failed_test_suites: failure_report = ['Failed tests (suites/files):'] + failure_report # Test suites where we didn't match a specific regexp and hence likely didn't count the failures - failure_report.extend('+ %s' % t for t in sorted(all_failed_test_suites - failed_test_suites)) + uncounted_test_suites = all_failed_test_suites - failed_test_suites + if uncounted_test_suites: + failure_report.append('Could not count failed tests for the following test suites/files:') + failure_report.extend(sorted(uncounted_test_suites)) # Test suites not included in the catch-all regexp but counted. Should be empty. - failure_report.extend('? %s' % t for t in sorted(failed_test_suites - all_failed_test_suites)) + unexpected_test_suites = failed_test_suites - all_failed_test_suites + if unexpected_test_suites: + failure_report.append('Counted failures of tests from the following test suites/files that are not ' + 'contained in the summary output of PyTorch:') + failure_report.extend(sorted(unexpected_test_suites)) failure_report = '\n'.join(failure_report) @@ -519,7 +526,9 @@ def test_step(self): msg += failure_report # If no tests are supposed to fail or some failed for which we were not able to count errors fail now - if max_failed_tests == 0 or failed_test_suites != all_failed_test_suites: + if failed_test_suites != all_failed_test_suites: + raise EasyBuildError('Failing because the PyTorch EasyBlock needs updating!\n' + msg) + elif max_failed_tests == 0: raise EasyBuildError(msg) else: msg += '\n\n' + ' '.join([ From d44f74b8842dfbb7da0fdb5c59937082400d5bae Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 14 May 2024 14:46:42 +0200 Subject: [PATCH 02/12] Move the test-parsing from logfiles into own function --- easybuild/easyblocks/p/pytorch.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index 467f74669d4..69dec130079 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -583,11 +583,11 @@ def make_module_req_guess(self): return guesses -if __name__ == '__main__': - arg = sys.argv[1] - if not os.path.isfile(arg): - raise RuntimeError('Expected a test result file to parse, got: ' + arg) - with open(arg, 'r') as f: +def parse_logfile(file): + """Parse the EB logfile and print the failed tests""" + if not os.path.isfile(file): + raise RuntimeError('Expected a test result file to parse, got: ' + file) + with open(file, 'r') as f: content = f.read() m = re.search(r'cmd .*python[^ ]* run_test\.py .* exited with exit code.*output', content) if m: @@ -599,3 +599,7 @@ def make_module_req_guess(self): print("Failed test names: ", find_failed_test_names(content)) print("Test result: ", parse_test_log(content)) + + +if __name__ == '__main__': + parse_logfile(sys.argv[1]) From 6cfef993094e1c3296c2db407e17d49415d7af36 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 14 May 2024 16:45:44 +0200 Subject: [PATCH 03/12] Refactor: Make distinction between `failure_report` and its base list --- easybuild/easyblocks/p/pytorch.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index 69dec130079..f491782a44f 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -486,7 +486,8 @@ def test_step(self): # Create clear summary report test_result = parse_test_log(tests_out) - failure_report = ['%s (%s)' % (suite.name, suite.summary) for suite in test_result.failed_suites] + # Use a list of messages we can later join together + failure_msgs = ['%s (%s)' % (suite.name, suite.summary) for suite in test_result.failed_suites] failed_test_suites = set(suite.name for suite in test_result.failed_suites) # Gather all failed tests suites in case we missed any (e.g. when it exited due to syntax errors) # Also unique to be able to compare the lists below @@ -495,20 +496,20 @@ def test_step(self): ) # If we missed any test suites prepend a list of all failed test suites if failed_test_suites != all_failed_test_suites: - failure_report = ['Failed tests (suites/files):'] + failure_report + failure_msgs = ['Failed tests (suites/files):'] + failure_msgs # Test suites where we didn't match a specific regexp and hence likely didn't count the failures uncounted_test_suites = all_failed_test_suites - failed_test_suites if uncounted_test_suites: - failure_report.append('Could not count failed tests for the following test suites/files:') - failure_report.extend(sorted(uncounted_test_suites)) + failure_msgs.append('Could not count failed tests for the following test suites/files:') + failure_msgs.extend(sorted(uncounted_test_suites)) # Test suites not included in the catch-all regexp but counted. Should be empty. unexpected_test_suites = failed_test_suites - all_failed_test_suites if unexpected_test_suites: - failure_report.append('Counted failures of tests from the following test suites/files that are not ' - 'contained in the summary output of PyTorch:') - failure_report.extend(sorted(unexpected_test_suites)) + failure_msgs.append('Counted failures of tests from the following test suites/files that are not ' + 'contained in the summary output of PyTorch:') + failure_msgs.extend(sorted(unexpected_test_suites)) - failure_report = '\n'.join(failure_report) + failure_report = '\n'.join(failure_msgs) # Calculate total number of unsuccesful and total tests failed_test_cnt = test_result.failure_cnt + test_result.error_cnt From 20b60c353eff5041e64727f8355ed76669c7c881 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 14 May 2024 16:53:02 +0200 Subject: [PATCH 04/12] Always fail on uncounted test suites and improve reason Differentiate between "outdated" EasyBlock and tests terminated by a signal --- easybuild/easyblocks/p/pytorch.py | 105 +++++++++++++++++++----------- 1 file changed, 67 insertions(+), 38 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index f491782a44f..af68b49f6b4 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -45,17 +45,31 @@ if sys.version_info >= (3, 9): from typing import NamedTuple + # Lists of tests that were marked as ERROR and FAIL respectively FailedTestNames = NamedTuple('FailedTestNames', [('error', list[str]), ('fail', list[str])]) + # Name of test suite and summary of the tests run/failed/... TestSuiteResult = NamedTuple('TestSuiteResult', [('name', str), ('summary', str)]) + # Test suite that was terminated with a signal + TerminatedTestSuite = NamedTuple('TerminatedTestSuite', [('name', str), ('signal', str)]) TestResult = NamedTuple('TestResult', [('test_cnt', int), ('error_cnt', int), ('failure_cnt', int), - ('failed_suites', list[TestSuiteResult])]) + ('failed_suites', list[TestSuiteResult]), + ('terminated_suites', list[TerminatedTestSuite]), + ('all_failed_suites', set[str]), + ]) else: from collections import namedtuple FailedTestNames = namedtuple('FailedTestNames', ('error', 'fail')) TestSuiteResult = namedtuple('TestSuiteResult', ('name', 'summary')) - TestResult = namedtuple('TestResult', ('test_cnt', 'error_cnt', 'failure_cnt', 'failed_suites')) + TerminatedTestSuite = namedtuple('TerminatedTestSuite', ('name', 'signal')) + TestResult = namedtuple('TestResult', ('test_cnt', + 'error_cnt', + 'failure_cnt', + 'failed_suites', + 'terminated_suites', + 'all_failed_suites' + )) def find_failed_test_names(tests_out): @@ -198,7 +212,17 @@ def get_count_for_pattern(regex, text): for m in re.finditer(regex, tests_out, re.M): test_cnt += sum(get_count_for_pattern(p, m.group("summary")) for p in count_patterns) - return TestResult(test_cnt=test_cnt, error_cnt=error_cnt, failure_cnt=failure_cnt, failed_suites=failed_suites) + # Gather all failed tests suites in case we missed any, + # e.g. when it exited due to syntax errors or with a signal such as SIGSEGV + failed_suites_and_signal = set( + re.findall(r"^(?P.*) failed!( Received signal: (\w+))?\s*$", tests_out, re.M) + ) + + return TestResult(test_cnt=test_cnt, error_cnt=error_cnt, failure_cnt=failure_cnt, + failed_suites=failed_suites, + terminated_suites=[TerminatedTestSuite(name, signal) + for name, _, signal in failed_suites_and_signal if signal], + all_failed_suites={i[0] for i in failed_suites_and_signal}) class EB_PyTorch(PythonPackage): @@ -489,11 +513,7 @@ def test_step(self): # Use a list of messages we can later join together failure_msgs = ['%s (%s)' % (suite.name, suite.summary) for suite in test_result.failed_suites] failed_test_suites = set(suite.name for suite in test_result.failed_suites) - # Gather all failed tests suites in case we missed any (e.g. when it exited due to syntax errors) - # Also unique to be able to compare the lists below - all_failed_test_suites = set( - re.findall(r"^(?P.*) failed!(?: Received signal: \w+)?\s*$", tests_out, re.M) - ) + all_failed_test_suites = test_result.all_failed_suites # If we missed any test suites prepend a list of all failed test suites if failed_test_suites != all_failed_test_suites: failure_msgs = ['Failed tests (suites/files):'] + failure_msgs @@ -509,46 +529,55 @@ def test_step(self): 'contained in the summary output of PyTorch:') failure_msgs.extend(sorted(unexpected_test_suites)) + # Assemble final report failure_report = '\n'.join(failure_msgs) - # Calculate total number of unsuccesful and total tests failed_test_cnt = test_result.failure_cnt + test_result.error_cnt - + # Only add count if we detected any failed tests if failed_test_cnt > 0: - max_failed_tests = self.cfg['max_failed_tests'] - failure_or_failures = 'failure' if test_result.failure_cnt == 1 else 'failures' error_or_errors = 'error' if test_result.error_cnt == 1 else 'errors' - msg = "%d test %s, %d test %s (out of %d):\n" % ( + failure_report = "%d test %s, %d test %s (out of %d):\n" % ( test_result.failure_cnt, failure_or_failures, test_result.error_cnt, error_or_errors, test_result.test_cnt - ) - msg += failure_report - - # If no tests are supposed to fail or some failed for which we were not able to count errors fail now - if failed_test_suites != all_failed_test_suites: - raise EasyBuildError('Failing because the PyTorch EasyBlock needs updating!\n' + msg) - elif max_failed_tests == 0: - raise EasyBuildError(msg) + ) + failure_report + + if failed_test_suites != all_failed_test_suites: + # Fail because we can't be sure how many tests failed + # so comparing to max_failed_tests cannot reasonably be done + terminated_suite_names = set(name for name, _ in test_result.terminated_suites) + if failed_test_suites | terminated_suite_names == all_failed_test_suites: + suites = ", ".join("%s(%s)" % name_signal for name_signal in test_result.terminated_suites) + msg = ('Failing because these test suites were terminated which makes it impossible' + 'to accurately count the failed tests: ' + suites + '!') else: - msg += '\n\n' + ' '.join([ - "The PyTorch test suite is known to include some flaky tests,", - "which may fail depending on the specifics of the system or the context in which they are run.", - "For this PyTorch installation, EasyBuild allows up to %d tests to fail." % max_failed_tests, - "We recommend to double check that the failing tests listed above ", - "are known to be flaky, or do not affect your intended usage of PyTorch.", - "In case of doubt, reach out to the EasyBuild community (via GitHub, Slack, or mailing list).", - ]) - # Print to console, the user should really be aware that we are accepting failing tests here... - print_warning(msg) - - # Also log this warning in the file log - self.log.warning(msg) - - if failed_test_cnt > max_failed_tests: - raise EasyBuildError("Too many failed tests (%d), maximum allowed is %d", - failed_test_cnt, max_failed_tests) + msg = 'Failing because the test accounting in the PyTorch EasyBlock needs updating!' + raise EasyBuildError(msg + '\n' + + 'You can check the test failures (in the log) manually and if they are harmless, ' + 'use --ignore-test-failures to make the test step pass.\n' + failure_report) + + if failed_test_cnt > 0: + max_failed_tests = self.cfg['max_failed_tests'] + + # If no tests are supposed to fail don't print the explanation, just fail + if max_failed_tests == 0: + raise EasyBuildError(failure_report) + msg = failure_report + '\n\n' + ' '.join([ + "The PyTorch test suite is known to include some flaky tests,", + "which may fail depending on the specifics of the system or the context in which they are run.", + "For this PyTorch installation, EasyBuild allows up to %d tests to fail." % max_failed_tests, + "We recommend to double check that the failing tests listed above ", + "are known to be flaky, or do not affect your intended usage of PyTorch.", + "In case of doubt, reach out to the EasyBuild community (via GitHub, Slack, or mailing list).", + ]) + # Print to console in addition to file, + # the user should really be aware that we are accepting failing tests here... + print_warning(msg, log=self.log) + + if failed_test_cnt > max_failed_tests: + raise EasyBuildError("Too many failed tests (%d), maximum allowed is %d", + failed_test_cnt, max_failed_tests) elif failure_report: raise EasyBuildError("Test ended with failures! Exit code: %s\n%s", tests_ec, failure_report) elif tests_ec: From e5b0dc7f1b26412a4a8941f88e07b04e8d4070cf Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 20 Feb 2025 16:57:33 +0100 Subject: [PATCH 05/12] Use dataclass instead of NamedTuple --- easybuild/easyblocks/p/pytorch.py | 43 +++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index cc1f357bd7c..cb03512c49c 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -44,20 +44,35 @@ if sys.version_info >= (3, 9): - from typing import NamedTuple - # Lists of tests that were marked as ERROR and FAIL respectively - FailedTestNames = NamedTuple('FailedTestNames', [('error', list[str]), ('fail', list[str])]) - # Name of test suite and summary of the tests run/failed/... - TestSuiteResult = NamedTuple('TestSuiteResult', [('name', str), ('summary', str)]) - # Test suite that was terminated with a signal - TerminatedTestSuite = NamedTuple('TerminatedTestSuite', [('name', str), ('signal', str)]) - TestResult = NamedTuple('TestResult', [('test_cnt', int), - ('error_cnt', int), - ('failure_cnt', int), - ('failed_suites', list[TestSuiteResult]), - ('terminated_suites', list[TerminatedTestSuite]), - ('all_failed_suites', set[str]), - ]) + from dataclasses import dataclass + + @dataclass + class FailedTestNames: + """Hold list of tests names that failed with error or failure""" + error: list[str] + fail: list[str] + + @dataclass + class TestSuiteResult: + """Hold the name of a test suite and a summary of the failures""" + name: str + summary: str + + @dataclass + class TerminatedTestSuite: + """Test suite name and the signal it terminated with""" + name: str + signal: str + + @dataclass + class TestResult: + """Status report and results of a test run""" + test_cnt: int + error_cnt: int + failure_cnt: int + failed_suites: list[TestSuiteResult] + terminated_suites: list[TerminatedTestSuite] + all_failed_suites: set[str] # Names of all failed suites else: from collections import namedtuple FailedTestNames = namedtuple('FailedTestNames', ('error', 'fail')) From b66420a7df18ffe6f4f33a142d50442935f413e5 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 20 Feb 2025 17:01:39 +0100 Subject: [PATCH 06/12] Small simplification of regex --- easybuild/easyblocks/p/pytorch.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index cb03512c49c..fc06f256054 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -230,13 +230,13 @@ def get_count_for_pattern(regex, text): # Gather all failed tests suites in case we missed any, # e.g. when it exited due to syntax errors or with a signal such as SIGSEGV failed_suites_and_signal = set( - re.findall(r"^(?P.*) failed!( Received signal: (\w+))?\s*$", tests_out, re.M) + re.findall(r"^(?P.*) failed!(?: Received signal: (\w+))?\s*$", tests_out, re.M) ) return TestResult(test_cnt=test_cnt, error_cnt=error_cnt, failure_cnt=failure_cnt, failed_suites=failed_suites, terminated_suites=[TerminatedTestSuite(name, signal) - for name, _, signal in failed_suites_and_signal if signal], + for name, signal in failed_suites_and_signal if signal], all_failed_suites={i[0] for i in failed_suites_and_signal}) From aeba74bf4e34674f85aee848446273839ab97594 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 20 Feb 2025 17:04:05 +0100 Subject: [PATCH 07/12] Rename test_result to parsed_test_result --- easybuild/easyblocks/p/pytorch.py | 32 +++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index fc06f256054..f846cc993c0 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -501,15 +501,15 @@ def test_step(self): 'excluded_tests': ' '.join(excluded_tests) }) - test_result = super(EB_PyTorch, self).test_step(return_output_ec=True) - if test_result is None: + parsed_test_result = super(EB_PyTorch, self).test_step(return_output_ec=True) + if parsed_test_result is None: if self.cfg['runtest'] is False: msg = "Do not set 'runtest' to False, use --skip-test-step instead." else: msg = "Tests did not run. Make sure 'runtest' is set to a command." raise EasyBuildError(msg) - tests_out, tests_ec = test_result + tests_out, tests_ec = parsed_test_result # Show failed subtests to aid in debugging failures failed_test_names = find_failed_test_names(tests_out) @@ -524,11 +524,11 @@ def test_step(self): self.log.warning("\n".join(msg)) # Create clear summary report - test_result = parse_test_log(tests_out) + parsed_test_result = parse_test_log(tests_out) # Use a list of messages we can later join together - failure_msgs = ['%s (%s)' % (suite.name, suite.summary) for suite in test_result.failed_suites] - failed_test_suites = set(suite.name for suite in test_result.failed_suites) - all_failed_test_suites = test_result.all_failed_suites + failure_msgs = ['%s (%s)' % (suite.name, suite.summary) for suite in parsed_test_result.failed_suites] + failed_test_suites = set(suite.name for suite in parsed_test_result.failed_suites) + all_failed_test_suites = parsed_test_result.all_failed_suites # If we missed any test suites prepend a list of all failed test suites if failed_test_suites != all_failed_test_suites: failure_msgs = ['Failed tests (suites/files):'] + failure_msgs @@ -547,23 +547,23 @@ def test_step(self): # Assemble final report failure_report = '\n'.join(failure_msgs) # Calculate total number of unsuccesful and total tests - failed_test_cnt = test_result.failure_cnt + test_result.error_cnt - # Only add count if we detected any failed tests + failed_test_cnt = parsed_test_result.failure_cnt + parsed_test_result.error_cnt + # Only add count message if we detected any failed tests if failed_test_cnt > 0: - failure_or_failures = 'failure' if test_result.failure_cnt == 1 else 'failures' - error_or_errors = 'error' if test_result.error_cnt == 1 else 'errors' + failure_or_failures = 'failure' if parsed_test_result.failure_cnt == 1 else 'failures' + error_or_errors = 'error' if parsed_test_result.error_cnt == 1 else 'errors' failure_report = "%d test %s, %d test %s (out of %d):\n" % ( - test_result.failure_cnt, failure_or_failures, - test_result.error_cnt, error_or_errors, - test_result.test_cnt + parsed_test_result.failure_cnt, failure_or_failures, + parsed_test_result.error_cnt, error_or_errors, + parsed_test_result.test_cnt ) + failure_report if failed_test_suites != all_failed_test_suites: # Fail because we can't be sure how many tests failed # so comparing to max_failed_tests cannot reasonably be done - terminated_suite_names = set(name for name, _ in test_result.terminated_suites) + terminated_suite_names = set(name for name, _ in parsed_test_result.terminated_suites) if failed_test_suites | terminated_suite_names == all_failed_test_suites: - suites = ", ".join("%s(%s)" % name_signal for name_signal in test_result.terminated_suites) + suites = ", ".join("%s(%s)" % name_signal for name_signal in parsed_test_result.terminated_suites) msg = ('Failing because these test suites were terminated which makes it impossible' 'to accurately count the failed tests: ' + suites + '!') else: From 09c525ca35921770fcc6a7ab9711a21f09c67f1b Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 21 Feb 2025 10:09:51 +0100 Subject: [PATCH 08/12] Use dictionary for terminated_suites Easier to search for test by name --- easybuild/easyblocks/p/pytorch.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index f846cc993c0..1eddec1fa77 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -58,12 +58,6 @@ class TestSuiteResult: name: str summary: str - @dataclass - class TerminatedTestSuite: - """Test suite name and the signal it terminated with""" - name: str - signal: str - @dataclass class TestResult: """Status report and results of a test run""" @@ -71,7 +65,7 @@ class TestResult: error_cnt: int failure_cnt: int failed_suites: list[TestSuiteResult] - terminated_suites: list[TerminatedTestSuite] + terminated_suites: dict[str, str] # Name and signal of terminated suites all_failed_suites: set[str] # Names of all failed suites else: from collections import namedtuple @@ -235,8 +229,8 @@ def get_count_for_pattern(regex, text): return TestResult(test_cnt=test_cnt, error_cnt=error_cnt, failure_cnt=failure_cnt, failed_suites=failed_suites, - terminated_suites=[TerminatedTestSuite(name, signal) - for name, signal in failed_suites_and_signal if signal], + # Assumes that the suite name is unique + terminated_suites={name: signal for name, signal in failed_suites_and_signal if signal}, all_failed_suites={i[0] for i in failed_suites_and_signal}) @@ -561,9 +555,10 @@ def test_step(self): if failed_test_suites != all_failed_test_suites: # Fail because we can't be sure how many tests failed # so comparing to max_failed_tests cannot reasonably be done - terminated_suite_names = set(name for name, _ in parsed_test_result.terminated_suites) + terminated_suite_names = set(parsed_test_result.terminated_suites) if failed_test_suites | terminated_suite_names == all_failed_test_suites: - suites = ", ".join("%s(%s)" % name_signal for name_signal in parsed_test_result.terminated_suites) + suites = ", ".join("%s(%s)" % name_signal + for name_signal in parsed_test_result.terminated_suites.items()) msg = ('Failing because these test suites were terminated which makes it impossible' 'to accurately count the failed tests: ' + suites + '!') else: From 8bfb14eb5aac01b69dda77a8284496d34ef5cc03 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 21 Feb 2025 10:12:18 +0100 Subject: [PATCH 09/12] Assemble final report once --- easybuild/easyblocks/p/pytorch.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index 1eddec1fa77..6d8c3b01ba5 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -538,19 +538,20 @@ def test_step(self): 'contained in the summary output of PyTorch:') failure_msgs.extend(sorted(unexpected_test_suites)) - # Assemble final report - failure_report = '\n'.join(failure_msgs) # Calculate total number of unsuccesful and total tests failed_test_cnt = parsed_test_result.failure_cnt + parsed_test_result.error_cnt # Only add count message if we detected any failed tests if failed_test_cnt > 0: failure_or_failures = 'failure' if parsed_test_result.failure_cnt == 1 else 'failures' error_or_errors = 'error' if parsed_test_result.error_cnt == 1 else 'errors' - failure_report = "%d test %s, %d test %s (out of %d):\n" % ( + failure_msgs.insert(0, "%d test %s, %d test %s (out of %d):" % ( parsed_test_result.failure_cnt, failure_or_failures, parsed_test_result.error_cnt, error_or_errors, parsed_test_result.test_cnt - ) + failure_report + )) + + # Assemble final report + failure_report = '\n'.join(failure_msgs) if failed_test_suites != all_failed_test_suites: # Fail because we can't be sure how many tests failed From 1c2a4d1e8eface16661d8747aabced2f79051df9 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 21 Feb 2025 10:12:51 +0100 Subject: [PATCH 10/12] Fix message when all unaccounted suites terminated We have 3 error cases: 1. Some suites were terminated and hence don't have proper test output we can use 2. Unexpected output format not parsed correctly or missed. Maybe some terminated suites but the major point is that we need an EasyBlock update for the former. 3. We parsed more suites than the PyTorch summary output showed. Likely a bug in the EasyBlock being to greedy. Diffrentiate those cases to not show a wrong message. --- easybuild/easyblocks/p/pytorch.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index 6d8c3b01ba5..1e165dea313 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -556,14 +556,19 @@ def test_step(self): if failed_test_suites != all_failed_test_suites: # Fail because we can't be sure how many tests failed # so comparing to max_failed_tests cannot reasonably be done - terminated_suite_names = set(parsed_test_result.terminated_suites) - if failed_test_suites | terminated_suite_names == all_failed_test_suites: - suites = ", ".join("%s(%s)" % name_signal - for name_signal in parsed_test_result.terminated_suites.items()) + if failed_test_suites | set(parsed_test_result.terminated_suites) == all_failed_test_suites: + # All failed test suites are either counted or terminated with a signal msg = ('Failing because these test suites were terminated which makes it impossible' - 'to accurately count the failed tests: ' + suites + '!') + 'to accurately count the failed tests: ') + msg += ", ".join("%s(%s)" % name_signal + for name_signal in sorted(parsed_test_result.terminated_suites.items())) + elif len(failed_test_suites) < len(all_failed_test_suites): + msg = ('Failing because not all failed tests could be determined. ' + 'The test accounting in the PyTorch EasyBlock needs updating!\n' + 'Missing: ' + ', '.join(sorted(all_failed_test_suites - failed_test_suites))) else: - msg = 'Failing because the test accounting in the PyTorch EasyBlock needs updating!' + msg = ('Failing because there were unexpected failures detected: ' + + ', '.join(sorted(failed_test_suites - all_failed_test_suites))) raise EasyBuildError(msg + '\n' + 'You can check the test failures (in the log) manually and if they are harmless, ' 'use --ignore-test-failures to make the test step pass.\n' + failure_report) From f73302cfa9bca33993d930c74c476dfc461910e0 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 21 Feb 2025 10:21:49 +0100 Subject: [PATCH 11/12] Improve failure message for uncounted suites --- easybuild/easyblocks/p/pytorch.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index 1e165dea313..5fdd15c1d0c 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -505,7 +505,7 @@ def test_step(self): tests_out, tests_ec = parsed_test_result - # Show failed subtests to aid in debugging failures + # Show failed subtests, if any, to aid in debugging failures failed_test_names = find_failed_test_names(tests_out) if failed_test_names.error or failed_test_names.fail: msg = [] @@ -520,17 +520,27 @@ def test_step(self): # Create clear summary report parsed_test_result = parse_test_log(tests_out) # Use a list of messages we can later join together - failure_msgs = ['%s (%s)' % (suite.name, suite.summary) for suite in parsed_test_result.failed_suites] + failure_msgs = ['\t%s (%s)' % (suite.name, suite.summary) for suite in parsed_test_result.failed_suites] + # These were accounted for failed_test_suites = set(suite.name for suite in parsed_test_result.failed_suites) + # Those are all that failed according to the summary output all_failed_test_suites = parsed_test_result.all_failed_suites - # If we missed any test suites prepend a list of all failed test suites + # We should have determined all failed test suites and only those. + # Otherwise show the mismatch and terminate later if failed_test_suites != all_failed_test_suites: - failure_msgs = ['Failed tests (suites/files):'] + failure_msgs + failure_msgs.insert(0, 'Failed tests (suites/files):') # Test suites where we didn't match a specific regexp and hence likely didn't count the failures uncounted_test_suites = all_failed_test_suites - failed_test_suites if uncounted_test_suites: failure_msgs.append('Could not count failed tests for the following test suites/files:') - failure_msgs.extend(sorted(uncounted_test_suites)) + for suite_name in sorted(uncounted_test_suites): + try: + signal = parsed_test_result.terminated_suites[suite_name] + reason = f'Terminated with {signal}' + except KeyError: + # Not ended with signal, might have failed due to e.g. syntax errors + reason = 'Did not run properly' + failure_msgs.append(f'\t{suite_name} ({reason})') # Test suites not included in the catch-all regexp but counted. Should be empty. unexpected_test_suites = failed_test_suites - all_failed_test_suites if unexpected_test_suites: From ad19427e4200df91311012839fefe5c82805ef64 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 21 Feb 2025 11:10:05 +0100 Subject: [PATCH 12/12] Be more careful with failed message It is not given that the test output parsing code is the issue. There is no reasonable output if the test failed to start at all, e.g. due to syntax errors. --- easybuild/easyblocks/p/pytorch.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/easybuild/easyblocks/p/pytorch.py b/easybuild/easyblocks/p/pytorch.py index 5fdd15c1d0c..7898be4d62f 100644 --- a/easybuild/easyblocks/p/pytorch.py +++ b/easybuild/easyblocks/p/pytorch.py @@ -539,7 +539,7 @@ def test_step(self): reason = f'Terminated with {signal}' except KeyError: # Not ended with signal, might have failed due to e.g. syntax errors - reason = 'Did not run properly' + reason = 'Undetected or did not run properly' failure_msgs.append(f'\t{suite_name} ({reason})') # Test suites not included in the catch-all regexp but counted. Should be empty. unexpected_test_suites = failed_test_suites - all_failed_test_suites @@ -574,7 +574,7 @@ def test_step(self): for name_signal in sorted(parsed_test_result.terminated_suites.items())) elif len(failed_test_suites) < len(all_failed_test_suites): msg = ('Failing because not all failed tests could be determined. ' - 'The test accounting in the PyTorch EasyBlock needs updating!\n' + 'Tests failed to start or the test accounting in the PyTorch EasyBlock needs updating!\n' 'Missing: ' + ', '.join(sorted(all_failed_test_suites - failed_test_suites))) else: msg = ('Failing because there were unexpected failures detected: ' +