From d38da74943e3317ae1fc6f541e2aaf837307c2ef Mon Sep 17 00:00:00 2001 From: Victor Holanda Date: Wed, 19 Feb 2020 14:01:35 +0100 Subject: [PATCH 1/3] [bug] Fix TypeError message for performance report Instead of failing when comparing reference of different types with ``` TypeError: '>=' not supported between instances of 'str' and 'float' ``` now, ReFrame fails with a much nicer message like ``` cannot compare 9.c with reference value 0 ``` --- reframe/utility/sanity.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/reframe/utility/sanity.py b/reframe/utility/sanity.py index 0584a72e41..36732cf013 100644 --- a/reframe/utility/sanity.py +++ b/reframe/utility/sanity.py @@ -547,6 +547,8 @@ def calc_bound(thres): except SanityError: error_msg = msg or '{0} is beyond reference value {1} (l={2}, u={3})' raise SanityError(_format(error_msg, val, ref, lower, upper)) + except TypeError: + raise SanityError(_format('cannot compare {0} with reference value {1}', val, ref)) else: return True From 639c55ea907f1e2aa0aef078edea6600914467b4 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Mon, 24 Feb 2020 10:44:32 +0100 Subject: [PATCH 2/3] Address PR comments --- reframe/utility/sanity.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/reframe/utility/sanity.py b/reframe/utility/sanity.py index 36732cf013..b7062511b5 100644 --- a/reframe/utility/sanity.py +++ b/reframe/utility/sanity.py @@ -58,6 +58,7 @@ computing statistical information on series of data etc. ''' + import builtins import glob as pyglob import itertools @@ -489,8 +490,13 @@ def assert_bounded(val, lower=None, upper=None, msg=None): if upper is None: upper = builtins.float('inf') - if val >= lower and val <= upper: - return True + try: + if val >= lower and val <= upper: + return True + except TypeError as e: + raise SanityError(_format( + "cannot compare '{0}' with reference value '{1}'", val, ref + )) from e error_msg = msg or 'value {0} not within bounds {1}..{2}' raise SanityError(_format(error_msg, val, lower, upper)) @@ -546,9 +552,7 @@ def calc_bound(thres): evaluate(assert_bounded(val, lower, upper)) except SanityError: error_msg = msg or '{0} is beyond reference value {1} (l={2}, u={3})' - raise SanityError(_format(error_msg, val, ref, lower, upper)) - except TypeError: - raise SanityError(_format('cannot compare {0} with reference value {1}', val, ref)) + raise SanityError(_format(error_msg, val, ref, lower, upper)) from None else: return True From baa91fa3093d75a178874fa519a96f895b5d3902 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Mon, 24 Feb 2020 15:30:08 +0100 Subject: [PATCH 3/3] Move checking of the perf var extracted variable inside the pipeline The reason for that is that we don't do any argument type checking in any other sanity function, so it would be strange to do it in either `assert_reference()` or `assert_bounded()` only. The reason for that is that type and value errors are handled specially if they are raised from user test context. In this particular case, though, the `assert_reference()` is called from the framework context as a result of user input. So it makes more sense then to do the type checking inside `check_performance()`, where we call it. We also don't type check the reference and lower/upper thresholds, because this should be addressed correctly by the updated declaration of the `reference` field as soon as https://github.com/eth-cscs/reframe/issues/1147 is fixed (see also related FIXME note for the `reference` field). Finally, this commit adds also a pipeline unit test to check for this condition. --- reframe/core/pipeline.py | 12 ++++++++++-- reframe/utility/sanity.py | 9 ++------- unittests/test_pipeline.py | 16 +++++++++++++++- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index afceb0ac60..8ad4a211c2 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -15,6 +15,7 @@ import functools import inspect import itertools +import numbers import os import shutil @@ -37,7 +38,6 @@ from reframe.core.schedulers import Job from reframe.core.schedulers.registry import getscheduler from reframe.core.systems import SystemPartition -from reframe.utility.sanity import assert_reference # Dependency kinds @@ -1321,10 +1321,18 @@ def check_performance(self): for key, values in self._perfvalues.items(): val, ref, low_thres, high_thres, *_ = values + + # Verify that val is a number + if not isinstance(val, numbers.Number): + raise SanityError( + "the value extracted for performance variable '%s' " + "is not a number: %s" % (key, val) + ) + tag = key.split(':')[-1] try: sn.evaluate( - assert_reference( + sn.assert_reference( val, ref, low_thres, high_thres, msg=('failed to meet reference: %s={0}, ' 'expected {1} (l={2}, u={3})' % tag)) diff --git a/reframe/utility/sanity.py b/reframe/utility/sanity.py index f689cafcc7..75d82444d8 100644 --- a/reframe/utility/sanity.py +++ b/reframe/utility/sanity.py @@ -495,13 +495,8 @@ def assert_bounded(val, lower=None, upper=None, msg=None): if upper is None: upper = builtins.float('inf') - try: - if val >= lower and val <= upper: - return True - except TypeError as e: - raise SanityError(_format( - "cannot compare '{0}' with reference value '{1}'", val, ref - )) from e + if val >= lower and val <= upper: + return True error_msg = msg or 'value {0} not within bounds {1}..{2}' raise SanityError(_format(error_msg, val, lower, upper)) diff --git a/unittests/test_pipeline.py b/unittests/test_pipeline.py index 75da6c222b..48550abffe 100644 --- a/unittests/test_pipeline.py +++ b/unittests/test_pipeline.py @@ -740,7 +740,6 @@ def setUp(self): 'value3': sn.extractsingle(r'performance3 = (\S+)', self.perf_file.name, 1, float) } - self.test.sanity_patterns = sn.assert_found(r'result = success', self.output_file.name) @@ -896,6 +895,21 @@ def test_tag_resolution(self): } self.test.check_performance() + def test_invalid_perf_value(self): + self.test.perf_patterns = { + 'value1': sn.extractsingle(r'performance1 = (\S+)', + self.perf_file.name, 1, float), + 'value2': sn.extractsingle(r'performance2 = (\S+)', + self.perf_file.name, 1, str), + 'value3': sn.extractsingle(r'performance3 = (\S+)', + self.perf_file.name, 1, float) + } + self.write_performance_output(performance1=1.3, + performance2='foo', + performance3=3.3) + with pytest.raises(SanityError, match='not a number'): + self.test.check_performance() + def test_perf_var_evaluation(self): # All performance values must be evaluated, despite the first one # failing To test this, we need an extract function that will have a