From c778314ec1984c99fcb947e65fca53bda8b02a1a Mon Sep 17 00:00:00 2001 From: Theofilos Manitaras Date: Wed, 5 Jun 2019 10:13:42 +0200 Subject: [PATCH 1/4] Fix bug in performance report output --- reframe/frontend/statistics.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/reframe/frontend/statistics.py b/reframe/frontend/statistics.py index b4a3294dce..5031dba62a 100644 --- a/reframe/frontend/statistics.py +++ b/reframe/frontend/statistics.py @@ -129,9 +129,9 @@ def performance_report(self): for key, ref in t.check.perfvalues.items(): var = key.split(':')[-1] - val = ref[0] + val = ref[1] try: - unit = ref[4] + unit = ref[5] except IndexError: unit = '(no unit specified)' From 0a30a02df10f561fac8c758f9474a9d734001290 Mon Sep 17 00:00:00 2001 From: Theofilos Manitaras Date: Wed, 5 Jun 2019 11:41:07 +0200 Subject: [PATCH 2/4] Address PR comments --- unittests/resources/checks/frontend_checks.py | 16 ++++++++++++++++ unittests/test_cli.py | 12 ++++++++++++ unittests/test_loader.py | 4 ++-- unittests/test_policies.py | 12 ++++++------ 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/unittests/resources/checks/frontend_checks.py b/unittests/resources/checks/frontend_checks.py index fae813d109..46286993c7 100644 --- a/unittests/resources/checks/frontend_checks.py +++ b/unittests/resources/checks/frontend_checks.py @@ -80,6 +80,22 @@ def __init__(self): } +@rfm.simple_test +class PerformanceSuccessCheck(BaseFrontendCheck): + def __init__(self): + super().__init__() + self.valid_systems = ['*'] + self.valid_prog_environs = ['*'] + self.perf_patterns = { + 'perf': sn.extractsingle(r'perf: (\d+)', self.stdout, 1, int) + } + self.reference = { + '*': { + 'perf': (10, -0.1, 0.1, 'Gflop/s') + } + } + + @rfm.simple_test class CustomPerformanceFailureCheck(BaseFrontendCheck): """Simulate a performance check that ignores completely logging""" diff --git a/unittests/test_cli.py b/unittests/test_cli.py index c89e89acfc..8aab08b82d 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -221,6 +221,18 @@ def test_performance_check_failure(self): ['login'], self.environs)) self.assertTrue(self._perflog_exists('PerformanceFailureCheck')) + def test_performance_report(self): + self.checkpath = ['unittests/resources/checks/frontend_checks.py'] + self.more_options = ['-t', 'PerformanceSuccessCheck', + '--performance-report'] + returncode, stdout, stderr = self._run_reframe() + + self.assertIn('PASSED', stdout) + self.assertEqual(0, returncode) + self.assertTrue(self._perflog_exists('PerformanceSuccessCheck')) + self.assertIn(r'PERFORMANCE REPORT', stdout) + self.assertIn(r'perf: 10 Gflop/s', stdout) + def test_skip_system_check_option(self): self.checkpath = ['unittests/resources/checks/frontend_checks.py'] self.more_options = ['--skip-system-check', '-t', 'NoSystemCheck'] diff --git a/unittests/test_loader.py b/unittests/test_loader.py index d4656e2216..04c07525c4 100644 --- a/unittests/test_loader.py +++ b/unittests/test_loader.py @@ -32,11 +32,11 @@ def test_load_file_absolute(self): def test_load_recursive(self): checks = self.loader.load_from_dir('unittests/resources/checks', recurse=True) - self.assertEqual(11, len(checks)) + self.assertEqual(12, len(checks)) def test_load_all(self): checks = self.loader_with_path.load_all() - self.assertEqual(10, len(checks)) + self.assertEqual(11, len(checks)) def test_load_all_with_prefix(self): checks = self.loader_with_prefix.load_all() diff --git a/unittests/test_policies.py b/unittests/test_policies.py index 903a606cdd..b04fec14c9 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -66,7 +66,7 @@ def test_runall(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(7, stats.num_cases()) + self.assertEqual(8, stats.num_cases()) self.assertEqual(4, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -76,7 +76,7 @@ def test_runall_skip_system_check(self): self.runall(self.checks, skip_system_check=True) stats = self.runner.stats - self.assertEqual(8, stats.num_cases()) + self.assertEqual(9, stats.num_cases()) self.assertEqual(4, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -86,7 +86,7 @@ def test_runall_skip_prgenv_check(self): self.runall(self.checks, skip_environ_check=True) stats = self.runner.stats - self.assertEqual(8, stats.num_cases()) + self.assertEqual(9, stats.num_cases()) self.assertEqual(4, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -97,7 +97,7 @@ def test_runall_skip_sanity_check(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(7, stats.num_cases()) + self.assertEqual(8, stats.num_cases()) self.assertEqual(3, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(0, self._num_failures_stage('sanity')) @@ -108,7 +108,7 @@ def test_runall_skip_performance_check(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(7, stats.num_cases()) + self.assertEqual(8, stats.num_cases()) self.assertEqual(3, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -119,7 +119,7 @@ def test_strict_performance_check(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(7, stats.num_cases()) + self.assertEqual(8, stats.num_cases()) self.assertEqual(5, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) From e16d8709b0aed46331d881af7e85493131ab9707 Mon Sep 17 00:00:00 2001 From: Theofilos Manitaras Date: Thu, 6 Jun 2019 09:18:13 +0200 Subject: [PATCH 3/4] Address PR comments (version 2) --- reframe/core/pipeline.py | 10 +++++----- reframe/frontend/statistics.py | 4 ++-- unittests/resources/checks/frontend_checks.py | 16 ---------------- unittests/test_cli.py | 5 +---- unittests/test_loader.py | 4 ++-- unittests/test_policies.py | 12 ++++++------ 6 files changed, 16 insertions(+), 35 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 298ee257b8..6fbcb15706 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -1134,19 +1134,19 @@ def check_performance(self): "tag `%s' not resolved in references for `%s'" % (tag, self._current_partition.fullname)) - self._perfvalues[key] = (tag, value, *self.reference[key]) + self._perfvalues[key] = (value, *self.reference[key]) self._perf_logger.log_performance(logging.INFO, tag, value, *self.reference[key]) - for values in self._perfvalues.values(): - tag, val, ref, low_thres, high_thres, *_ = values + for key, values in self._perfvalues.items(): + val, ref, low_thres, high_thres, *_ = values try: evaluate( assert_reference( val, ref, low_thres, high_thres, msg=('failed to meet reference: %s={0}, ' - 'expected {1} (l={2}, u={3})' % tag), - ) + 'expected {1} (l={2}, u={3})' % + key.split(':')[-1])) ) except SanityError as e: raise PerformanceError(e) diff --git a/reframe/frontend/statistics.py b/reframe/frontend/statistics.py index 5031dba62a..b4a3294dce 100644 --- a/reframe/frontend/statistics.py +++ b/reframe/frontend/statistics.py @@ -129,9 +129,9 @@ def performance_report(self): for key, ref in t.check.perfvalues.items(): var = key.split(':')[-1] - val = ref[1] + val = ref[0] try: - unit = ref[5] + unit = ref[4] except IndexError: unit = '(no unit specified)' diff --git a/unittests/resources/checks/frontend_checks.py b/unittests/resources/checks/frontend_checks.py index 46286993c7..fae813d109 100644 --- a/unittests/resources/checks/frontend_checks.py +++ b/unittests/resources/checks/frontend_checks.py @@ -80,22 +80,6 @@ def __init__(self): } -@rfm.simple_test -class PerformanceSuccessCheck(BaseFrontendCheck): - def __init__(self): - super().__init__() - self.valid_systems = ['*'] - self.valid_prog_environs = ['*'] - self.perf_patterns = { - 'perf': sn.extractsingle(r'perf: (\d+)', self.stdout, 1, int) - } - self.reference = { - '*': { - 'perf': (10, -0.1, 0.1, 'Gflop/s') - } - } - - @rfm.simple_test class CustomPerformanceFailureCheck(BaseFrontendCheck): """Simulate a performance check that ignores completely logging""" diff --git a/unittests/test_cli.py b/unittests/test_cli.py index 8aab08b82d..69ffb3c3f1 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -223,13 +223,10 @@ def test_performance_check_failure(self): def test_performance_report(self): self.checkpath = ['unittests/resources/checks/frontend_checks.py'] - self.more_options = ['-t', 'PerformanceSuccessCheck', + self.more_options = ['-t', 'PerformanceFailureCheck', '--performance-report'] returncode, stdout, stderr = self._run_reframe() - self.assertIn('PASSED', stdout) - self.assertEqual(0, returncode) - self.assertTrue(self._perflog_exists('PerformanceSuccessCheck')) self.assertIn(r'PERFORMANCE REPORT', stdout) self.assertIn(r'perf: 10 Gflop/s', stdout) diff --git a/unittests/test_loader.py b/unittests/test_loader.py index 04c07525c4..d4656e2216 100644 --- a/unittests/test_loader.py +++ b/unittests/test_loader.py @@ -32,11 +32,11 @@ def test_load_file_absolute(self): def test_load_recursive(self): checks = self.loader.load_from_dir('unittests/resources/checks', recurse=True) - self.assertEqual(12, len(checks)) + self.assertEqual(11, len(checks)) def test_load_all(self): checks = self.loader_with_path.load_all() - self.assertEqual(11, len(checks)) + self.assertEqual(10, len(checks)) def test_load_all_with_prefix(self): checks = self.loader_with_prefix.load_all() diff --git a/unittests/test_policies.py b/unittests/test_policies.py index b04fec14c9..903a606cdd 100644 --- a/unittests/test_policies.py +++ b/unittests/test_policies.py @@ -66,7 +66,7 @@ def test_runall(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(8, stats.num_cases()) + self.assertEqual(7, stats.num_cases()) self.assertEqual(4, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -76,7 +76,7 @@ def test_runall_skip_system_check(self): self.runall(self.checks, skip_system_check=True) stats = self.runner.stats - self.assertEqual(9, stats.num_cases()) + self.assertEqual(8, stats.num_cases()) self.assertEqual(4, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -86,7 +86,7 @@ def test_runall_skip_prgenv_check(self): self.runall(self.checks, skip_environ_check=True) stats = self.runner.stats - self.assertEqual(9, stats.num_cases()) + self.assertEqual(8, stats.num_cases()) self.assertEqual(4, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -97,7 +97,7 @@ def test_runall_skip_sanity_check(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(8, stats.num_cases()) + self.assertEqual(7, stats.num_cases()) self.assertEqual(3, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(0, self._num_failures_stage('sanity')) @@ -108,7 +108,7 @@ def test_runall_skip_performance_check(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(8, stats.num_cases()) + self.assertEqual(7, stats.num_cases()) self.assertEqual(3, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) @@ -119,7 +119,7 @@ def test_strict_performance_check(self): self.runall(self.checks) stats = self.runner.stats - self.assertEqual(8, stats.num_cases()) + self.assertEqual(7, stats.num_cases()) self.assertEqual(5, len(stats.failures())) self.assertEqual(2, self._num_failures_stage('setup')) self.assertEqual(1, self._num_failures_stage('sanity')) From 275ebd9291c40fd42dd98bcdbf0cae954f1fa60a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 6 Jun 2019 22:30:22 +0200 Subject: [PATCH 4/4] Minor style fixes --- reframe/core/pipeline.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 6fbcb15706..6e1faa9c8e 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -1140,13 +1140,13 @@ def check_performance(self): for key, values in self._perfvalues.items(): val, ref, low_thres, high_thres, *_ = values + tag = key.split(':')[-1] try: evaluate( assert_reference( val, ref, low_thres, high_thres, msg=('failed to meet reference: %s={0}, ' - 'expected {1} (l={2}, u={3})' % - key.split(':')[-1])) + 'expected {1} (l={2}, u={3})' % tag)) ) except SanityError as e: raise PerformanceError(e)