From 1dc6cee563761d98c43f5450d4ba29f596245b9a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 6 Aug 2021 18:21:37 +0200 Subject: [PATCH 01/21] Allow setting test variables from the command line --- reframe/core/decorators.py | 26 +++++++++--------- reframe/core/fields.py | 4 +++ reframe/core/meta.py | 5 ++-- reframe/core/variables.py | 55 ++++++++++++++++++++++++++++++------- reframe/frontend/cli.py | 19 ++++++++++++- reframe/frontend/loader.py | 23 +++++++++------- unittests/test_cli.py | 10 +++++++ unittests/test_modules.py | 2 +- unittests/test_utility.py | 1 + unittests/test_variables.py | 37 +++++++++++++++++++++++++ 10 files changed, 144 insertions(+), 38 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 7f4756b05f..37dee8a446 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -27,25 +27,25 @@ from reframe.utility.versioning import VersionValidator -def _register_test(cls, args=None): +def _register_test(cls, inst_args=None, *args, **kwargs): '''Register the test. Register the test with _rfm_use_params=True. This additional argument flags this case to consume the parameter space. Otherwise, the regression test parameters would simply be initialized to None. ''' - def _instantiate(cls, args): - if isinstance(args, collections.abc.Sequence): - return cls(*args, _rfm_use_params=True) - elif isinstance(args, collections.abc.Mapping): - args['_rfm_use_params'] = True - return cls(**args) - elif args is None: - return cls(_rfm_use_params=True) - - def _instantiate_all(): + def _instantiate(cls, inst_args, *args, **kwargs): + kwargs['_rfm_use_params'] = True + if isinstance(inst_args, collections.abc.Sequence): + args += inst_args + elif isinstance(inst_args, collections.abc.Mapping): + kwargs.update(args) + + return cls(*args, **kwargs) + + def _instantiate_all(*args, **kwargs): ret = [] - for cls, args in mod.__rfm_test_registry: + for cls, inst_args in mod.__rfm_test_registry: try: if cls in mod.__rfm_skip_tests: continue @@ -53,7 +53,7 @@ def _instantiate_all(): mod.__rfm_skip_tests = set() try: - ret.append(_instantiate(cls, args)) + ret.append(_instantiate(cls, inst_args, *args, **kwargs)) except SkipTestError as e: getlogger().warning(f'skipping test {cls.__qualname__!r}: {e}') except Exception: diff --git a/reframe/core/fields.py b/reframe/core/fields.py index 4ec5addcaf..263601cb90 100644 --- a/reframe/core/fields.py +++ b/reframe/core/fields.py @@ -46,6 +46,10 @@ def __init__(self, main_type, *other_types): raise TypeError('{0} is not a sequence of types'. format(self._types)) + @property + def valid_types(self): + return self._types + def _check_type(self, value): if not any(isinstance(value, t) for t in self._types): typedescr = '|'.join(t.__name__ for t in self._types) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 76fc7bab76..bfaaa64371 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -335,13 +335,12 @@ def __call__(cls, *args, **kwargs): # Intercept constructor arguments _rfm_use_params = kwargs.pop('_rfm_use_params', False) - + _rfm_external_vals = kwargs.pop('_rfm_external_vals', {}) obj = cls.__new__(cls, *args, **kwargs) # Insert the var & param spaces - cls._rfm_var_space.inject(obj, cls) + cls._rfm_var_space.inject(obj, cls, _rfm_external_vals) cls._rfm_param_space.inject(obj, cls, _rfm_use_params) - obj.__init__(*args, **kwargs) return obj diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 2727a290c1..9936f0f602 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -12,6 +12,7 @@ import reframe.core.fields as fields import reframe.core.namespaces as namespaces +import reframe.utility as util from reframe.core.exceptions import ReframeSyntaxError @@ -34,27 +35,47 @@ class TestVar: during class instantiation. To support injecting attributes into the variable, this class implements a - separate dict `__attrs__` where these will be stored. + separate dict `_attrs` where these will be stored. :meta private: ''' __slots__ = ( - 'field', '_default_value', 'name', '__attrs__' + '_attrs', '_conv_fn', '_default_value', 'field', 'name' ) def __init__(self, *args, **kwargs): field_type = kwargs.pop('field', fields.TypedField) self._default_value = kwargs.pop('value', Undefined) - if not issubclass(field_type, fields.Field): raise TypeError( f'field {field_type!r} is not derived from ' f'{fields.Field.__qualname__}' ) + conv_fn = kwargs.pop('conv', None) self.field = field_type(*args, **kwargs) - self.__attrs__ = dict() + self._attrs = dict() + + def _default_conv(s): + errmsg = f'cannot convert {s!r} for setting variable {self.name}' + if not isinstance(self.field, fields.TypedField): + raise TypeError(errmsg) + + for typ in self.field.valid_types: + if typ is type(None) and s == 'None': + # Treat `None` specially + return None + + try: + return typ(s) + except Exception: + continue + + raise TypeError(errmsg) + + # Set the conversion function + self._conv_fn = conv_fn or _default_conv def is_defined(self): return self._default_value is not Undefined @@ -65,6 +86,9 @@ def undefine(self): def define(self, value): self._default_value = value + def define_from_string(self, value): + self.define(self._conv_fn(value)) + @property def default_value(self): # Variables must be returned by-value to prevent an instance from @@ -75,21 +99,21 @@ def default_value(self): @property def attrs(self): # Variable attributes must also be returned by-value. - return copy.deepcopy(self.__attrs__) + return copy.deepcopy(self._attrs) def __set_name__(self, owner, name): self.name = name def __setattr__(self, name, value): - '''Set any additional variable attribute into __attrs__.''' + '''Set any additional variable attribute into _attrs.''' if name in self.__slots__: super().__setattr__(name, value) else: - self.__attrs__[name] = value + self._attrs[name] = value def __getattr__(self, name): - '''Attribute lookup into __attrs__.''' - attrs = self.__getattribute__('__attrs__') + '''Attribute lookup into _attrs.''' + attrs = self.__getattribute__('_attrs') try: return attrs[name] except KeyError: @@ -524,13 +548,24 @@ def sanity(self, cls, illegal_names=None): f'{cls.__qualname__!r}' ) - def inject(self, obj, cls): + def inject(self, obj, cls, external_vals=None): '''Insert the vars in the regression test. :param obj: The test object. :param cls: The test class. + :param external_vals: A dictionary of variables and their values as + set by external sources (i.e., command-line) ''' + if external_vals: + for name, val in external_vals.items(): + try: + var = self[name] + except KeyError: + continue + else: + var.define_from_string(val) + for name, var in self.items(): setattr(cls, name, var.field) getattr(cls, name).__set_name__(obj, name) diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py index 387f95fff6..6e3285958e 100644 --- a/reframe/frontend/cli.py +++ b/reframe/frontend/cli.py @@ -329,6 +329,11 @@ def main(): '--skip-prgenv-check', action='store_true', help='Skip programming environment check' ) + run_options.add_argument( + '-S', '--setvar', action='append', metavar='VAR=VAL', + dest='vars', default=[], + help='Set test variable VAR to VAL in all selected tests' + ) run_options.add_argument( '--exec-policy', metavar='POLICY', action='store', choices=['async', 'serial'], default='async', @@ -749,8 +754,20 @@ def print_infoline(param, value): print_infoline('output directory', repr(session_info['prefix_output'])) printer.info('') try: + # Collect any variables set from the command line + external_vals = {} + for expr in options.vars: + try: + lhs, rhs = expr.split('=', maxsplit=1) + except ValueError: + printer.warning( + f'invalid test variable assignment: {expr}; skipping' + ) + else: + external_vals[lhs] = rhs + # Locate and load checks - checks_found = loader.load_all() + checks_found = loader.load_all(_rfm_external_vals=external_vals) printer.verbose(f'Loaded {len(checks_found)} test(s)') # Generate all possible test cases first; we will need them for diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 6eb24dc57d..9bfe1bc4c8 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -115,7 +115,7 @@ def prefix(self): def recurse(self): return self._recurse - def load_from_module(self, module): + def load_from_module(self, module, *args, **kwargs): '''Load user checks from module. This method tries to call the `_rfm_gettests()` method of the user @@ -133,7 +133,7 @@ def load_from_module(self, module): getlogger().debug('No tests registered') return [] - candidates = module._rfm_gettests() + candidates = module._rfm_gettests(*args, **kwargs) if not isinstance(candidates, collections.abc.Sequence): getlogger().warning( f'Tests not registered correctly in {module.__name__!r}' @@ -163,13 +163,13 @@ def load_from_module(self, module): getlogger().debug(f' > Loaded {len(ret)} test(s)') return ret - def load_from_file(self, filename, force=False): + def load_from_file(self, filename, force=False, *args, **kwargs): if not self._validate_source(filename): return [] try: return self.load_from_module( - util.import_module_from_file(filename, force) + util.import_module_from_file(filename, force), *args, **kwargs ) except Exception: exc_info = sys.exc_info() @@ -184,12 +184,14 @@ def load_from_file(self, filename, force=False): else: raise - def load_from_dir(self, dirname, recurse=False, force=False): + def load_from_dir(self, dirname, recurse=False, + force=False, *args, **kwargs): checks = [] for entry in os.scandir(dirname): if recurse and entry.is_dir(): checks.extend( - self.load_from_dir(entry.path, recurse, force) + self.load_from_dir(entry.path, recurse, + force, *args, **kwargs) ) if (entry.name.startswith('.') or @@ -197,11 +199,11 @@ def load_from_dir(self, dirname, recurse=False, force=False): not entry.is_file()): continue - checks += self.load_from_file(entry.path, force) + checks += self.load_from_file(entry.path, force, *args, **kwargs) return checks - def load_all(self, force=False): + def load_all(self, force=False, *args, **kwargs): '''Load all checks in self._load_path. If a prefix exists, it will be prepended to each path. @@ -217,8 +219,9 @@ def load_all(self, force=False): continue if os.path.isdir(d): - checks += self.load_from_dir(d, self._recurse, force) + checks += self.load_from_dir(d, self._recurse, force, + *args, **kwargs) else: - checks += self.load_from_file(d, force) + checks += self.load_from_file(d, force, *args, **kwargs) return checks diff --git a/unittests/test_cli.py b/unittests/test_cli.py index 6a1055e8b9..7020e181b1 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -777,3 +777,13 @@ def test_detect_host_topology_file(run_reframe, tmp_path): assert returncode == 0 with open(topo_file) as fp: assert json.load(fp) == cpuinfo() + + +def test_external_vars(run_reframe): + returncode, stdout, stderr = run_reframe( + checkpath=['unittests/resources/checks_unlisted/externalvars.py'], + more_options=['-S', 'foo=3'] + ) + assert 'Traceback' not in stdout + assert 'Traceback' not in stderr + assert returncode == 0 diff --git a/unittests/test_modules.py b/unittests/test_modules.py index 696eb00c31..2b85a27e94 100644 --- a/unittests/test_modules.py +++ b/unittests/test_modules.py @@ -243,7 +243,7 @@ def _emit_load_commands_tmod4(modules_system): 'module restore foo', f'module use {test_util.TEST_MODULES}' ] assert emit_cmds('foo/1.2') == ['module load foo/1.2'] - if modules_system.name is 'lmod': + if modules_system.name == 'lmod': assert emit_cmds('foo', path='/path') == ['module use /path', 'module load foo'] else: diff --git a/unittests/test_utility.py b/unittests/test_utility.py index bfc7737122..09fe8e9028 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -1544,6 +1544,7 @@ def test_jsonext_dumps(): # Classes to test JSON deserialization + class _D(jsonext.JSONSerializable): def __init__(self): self.a = 2 diff --git a/unittests/test_variables.py b/unittests/test_variables.py index fa86fdadad..70df8aa2e2 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -451,3 +451,40 @@ class A(rfm.RegressionTest): assert math.trunc(npi) == -3 assert math.floor(npi) == -4 assert math.ceil(npi) == -3 + + +@pytest.fixture +def ConvTest(): + class _X(rfm.RegressionTest): + x = variable(int) + y = variable(float, value=3.141592) + z = variable(int, type(None)) + w = variable(int, value=3, conv=lambda x: 1) + + yield _X + + +def test_define_from_string(ConvTest): + x = ConvTest( + _rfm_external_vals={'x': '3', 'y': '3.14', 'z': 'None', 'w': '2'} + ) + assert x.x == 3 + assert x.y == 3.14 + assert x.z is None + assert x.w == 1 + + +def test_define_from_string_partial(ConvTest): + x = ConvTest(_rfm_external_vals={'x': '3', 'z': 'None'}) + assert x.x == 3 + assert x.y == 3.141592 + assert x.z is None + assert x.w == 3 + + +def test_define_from_string_errors(ConvTest): + with pytest.raises(TypeError): + ConvTest(_rfm_external_vals={'x': 'None'}) + + with pytest.raises(TypeError): + ConvTest(_rfm_external_vals={'z': '3.14'}) From 078cdb54bb28338e88de09fe182417292d03952a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 10 Aug 2021 09:34:22 +0200 Subject: [PATCH 02/21] Add missing file + another unit test --- reframe/core/variables.py | 3 +-- reframe/frontend/cli.py | 2 +- .../resources/checks_unlisted/externalvars.py | 18 ++++++++++++++++++ unittests/test_cli.py | 10 ++++++++++ 4 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 unittests/resources/checks_unlisted/externalvars.py diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 9936f0f602..65671f5621 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -12,7 +12,6 @@ import reframe.core.fields as fields import reframe.core.namespaces as namespaces -import reframe.utility as util from reframe.core.exceptions import ReframeSyntaxError @@ -58,7 +57,7 @@ def __init__(self, *args, **kwargs): self._attrs = dict() def _default_conv(s): - errmsg = f'cannot convert {s!r} for setting variable {self.name}' + errmsg = f'cannot convert {s!r} for setting variable {self.name!r}' if not isinstance(self.field, fields.TypedField): raise TypeError(errmsg) diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py index 6e3285958e..35da5b6f94 100644 --- a/reframe/frontend/cli.py +++ b/reframe/frontend/cli.py @@ -761,7 +761,7 @@ def print_infoline(param, value): lhs, rhs = expr.split('=', maxsplit=1) except ValueError: printer.warning( - f'invalid test variable assignment: {expr}; skipping' + f'invalid test variable assignment: {expr!r}; skipping' ) else: external_vals[lhs] = rhs diff --git a/unittests/resources/checks_unlisted/externalvars.py b/unittests/resources/checks_unlisted/externalvars.py new file mode 100644 index 0000000000..81687ee4d1 --- /dev/null +++ b/unittests/resources/checks_unlisted/externalvars.py @@ -0,0 +1,18 @@ +import reframe as rfm +import reframe.utility.sanity as sn + + +@rfm.simple_test +class external_vars_test(rfm.RunOnlyRegressionTest): + valid_systems = ['*'] + valid_prog_environs = ['*'] + foo = variable(int, value=1) + executable = 'echo' + + @run_before('run') + def set_exec_opts(self): + self.executable_opts = [f'{self.foo}'] + + @sanity_function + def assert_foo(self): + return sn.assert_eq(sn.extractsingle(r'(\d+)', self.stdout, 1, int), 3) diff --git a/unittests/test_cli.py b/unittests/test_cli.py index 7020e181b1..89871b5947 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -787,3 +787,13 @@ def test_external_vars(run_reframe): assert 'Traceback' not in stdout assert 'Traceback' not in stderr assert returncode == 0 + + +def test_external_vars_invalid_expr(run_reframe): + returncode, stdout, stderr = run_reframe( + more_options=['-S', 'foo'] + ) + assert 'Traceback' not in stdout + assert 'Traceback' not in stderr + assert 'invalid test variable assignment' in stdout + assert returncode == 0 From e191da3069980cdacbdc9eaf8fe5b41468a5a8f4 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 10 Aug 2021 10:23:19 +0200 Subject: [PATCH 03/21] Fix PEP8 issues --- reframe/core/variables.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 65671f5621..a62b507a20 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -62,7 +62,7 @@ def _default_conv(s): raise TypeError(errmsg) for typ in self.field.valid_types: - if typ is type(None) and s == 'None': + if issubclass(typ, type(None)) and s == 'None': # Treat `None` specially return None From cbf3c4adc076c2023d1f32fa08382b29ce40f427 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 10 Aug 2021 18:58:29 +0200 Subject: [PATCH 04/21] Allow implicit type conversions for aggregate types --- reframe/utility/typecheck.py | 75 +++++++++++++++++++++++++++++------- unittests/test_typecheck.py | 49 +++++++++++++++++++++++ 2 files changed, 110 insertions(+), 14 deletions(-) diff --git a/reframe/utility/typecheck.py b/reframe/utility/typecheck.py index 20076cb64b..4bae3378a2 100644 --- a/reframe/utility/typecheck.py +++ b/reframe/utility/typecheck.py @@ -94,9 +94,12 @@ class _TypeFactory(abc.ABCMeta): - def register_subtypes(cls): - for t in cls._subtypes: - cls.register(t) + def register_wrapped_type(cls): + cls.register(cls._type) + + def _check_conv_type(cls, s): + if not isinstance(s, str): + raise TypeError('cannot convert from non-string types') # Metaclasses that implement the isinstance logic for the different aggregate @@ -110,7 +113,7 @@ def __init__(cls, name, bases, namespace): cls._elem_type = None cls._bases = bases cls._namespace = namespace - cls.register_subtypes() + cls.register_wrapped_type() def __instancecheck__(cls, inst): if not issubclass(type(inst), cls): @@ -132,10 +135,16 @@ def __getitem__(cls, elem_type): ret = _ContainerType('%s[%s]' % (cls.__name__, elem_type.__name__), cls._bases, cls._namespace) ret._elem_type = elem_type - ret.register_subtypes() + ret.register_wrapped_type() cls.register(ret) return ret + def __call__(cls, s): + cls._check_conv_type(s) + container_type = cls._type + elem_type = cls._elem_type + return container_type(elem_type(e) for e in s.split(',')) + class _TupleType(_ContainerType): '''A metaclass for tuples. @@ -174,10 +183,25 @@ def __getitem__(cls, elem_types): ) ret = _TupleType(cls_name, cls._bases, cls._namespace) ret._elem_type = elem_types - ret.register_subtypes() + ret.register_wrapped_type() cls.register(ret) return ret + def __call__(cls, s): + cls._check_conv_type(s) + container_type = cls._type + elem_types = cls._elem_type + elems = s.split(',') + if len(elem_types) == 1: + elem_t = elem_types[0] + return container_type(elem_t(e) for e in elems) + elif len(elem_types) != len(elems): + raise TypeError(f'cannot convert string {s!r} to {cls.__name__!r}') + else: + return container_type( + elem_t(e) for elem_t, e in zip(elem_types, elems) + ) + class _MappingType(_TypeFactory): '''A metaclass for type checking mapping types.''' @@ -188,7 +212,7 @@ def __init__(cls, name, bases, namespace): cls._value_type = None cls._bases = bases cls._namespace = namespace - cls.register_subtypes() + cls.register_wrapped_type() def __instancecheck__(cls, inst): if not issubclass(type(inst), cls): @@ -221,10 +245,29 @@ def __getitem__(cls, typespec): ret = _MappingType(cls_name, cls._bases, cls._namespace) ret._key_type = key_type ret._value_type = value_type - ret.register_subtypes() + ret.register_wrapped_type() cls.register(ret) return ret + def __call__(cls, s): + cls._check_conv_type(s) + mappping_type = cls._type + key_type = cls._key_type + value_type = cls._value_type + seq = [] + for key_datum in s.split(','): + try: + k, v = key_datum.split(':') + except ValueError: + # Re-raise as TypeError + raise TypeError( + f'cannot convert string {s!r} to {cls.__name__!r}' + ) from None + + seq.append((key_type(k), value_type(v))) + + return mappping_type(seq) + class _StrType(_ContainerType): '''A metaclass for type checking string types.''' @@ -247,26 +290,30 @@ def __getitem__(cls, patt): ret = _StrType("%s[r'%s']" % (cls.__name__, patt), cls._bases, cls._namespace) ret._elem_type = patt - ret.register_subtypes() + ret.register_wrapped_type() cls.register(ret) return ret + def __call__(cls, s): + cls._check_conv_type(s) + return s + class Dict(metaclass=_MappingType): - _subtypes = (dict,) + _type = dict class List(metaclass=_ContainerType): - _subtypes = (list,) + _type = list class Set(metaclass=_ContainerType): - _subtypes = (set,) + _type = set class Str(metaclass=_StrType): - _subtypes = (str,) + _type = str class Tuple(metaclass=_TupleType): - _subtypes = (tuple,) + _type = tuple diff --git a/unittests/test_typecheck.py b/unittests/test_typecheck.py index efd342eda7..5820d638dd 100644 --- a/unittests/test_typecheck.py +++ b/unittests/test_typecheck.py @@ -35,6 +35,16 @@ def test_list_type(): with pytest.raises(TypeError): types.List[int, float] + # Test type conversions + assert types.List[int]('1,2') == [1, 2] + assert types.List[int]('1') == [1] + + with pytest.raises(ValueError): + types.List[int]('foo') + + with pytest.raises(TypeError): + types.List[int](1) + def test_set_type(): s = {1, 2} @@ -54,6 +64,15 @@ def test_set_type(): with pytest.raises(TypeError): types.Set[int, float] + assert types.Set[int]('1,2') == {1, 2} + assert types.Set[int]('1') == {1} + + with pytest.raises(ValueError): + types.Set[int]('foo') + + with pytest.raises(TypeError): + types.Set[int](1) + def test_uniform_tuple_type(): t = (1, 2) @@ -74,6 +93,15 @@ def test_uniform_tuple_type(): with pytest.raises(TypeError): types.Set[3] + assert types.Tuple[int]('1,2') == (1, 2) + assert types.Tuple[int]('1') == (1,) + + with pytest.raises(ValueError): + types.Tuple[int]('foo') + + with pytest.raises(TypeError): + types.Tuple[int](1) + def test_non_uniform_tuple_type(): t = (1, 2.3, '4', ['a', 'b']) @@ -86,6 +114,14 @@ def test_non_uniform_tuple_type(): with pytest.raises(TypeError): types.Set[int, 3] + assert types.Tuple[int, str]('1,2') == (1, '2') + + with pytest.raises(TypeError): + types.Tuple[int, str]('1') + + with pytest.raises(TypeError): + types.Tuple[int, str](1) + def test_mapping_type(): d = {'one': 1, 'two': 2} @@ -106,6 +142,12 @@ def test_mapping_type(): with pytest.raises(TypeError): types.Dict[int, 3] + # Test conversions + assert types.Dict[str, int]('a:1,b:2') == {'a': 1, 'b': 2} + + with pytest.raises(TypeError): + types.Dict[str, int]('a:1,b') + def test_str_type(): s = '123' @@ -121,6 +163,13 @@ def test_str_type(): with pytest.raises(TypeError): types.Str[int] + # Test conversion + typ = types.Str[r'\d+'] + assert typ('10') == '10' + + with pytest.raises(TypeError): + types.Str[r'\d+'](1) + def test_type_names(): assert 'List' == types.List.__name__ From 85684c783f467cf7038d1f722c8d692742168935 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Thu, 12 Aug 2021 23:08:18 +0200 Subject: [PATCH 05/21] Allow cast-from-string for user types --- reframe/utility/typecheck.py | 64 +++++++++++++++++++----------------- unittests/test_typecheck.py | 31 +++++++++++++++++ 2 files changed, 65 insertions(+), 30 deletions(-) diff --git a/reframe/utility/typecheck.py b/reframe/utility/typecheck.py index 4bae3378a2..e3e25b3edc 100644 --- a/reframe/utility/typecheck.py +++ b/reframe/utility/typecheck.py @@ -93,19 +93,27 @@ import re -class _TypeFactory(abc.ABCMeta): - def register_wrapped_type(cls): - cls.register(cls._type) +class Type(abc.ABCMeta): + def __call__(cls, *args, **kwargs): + if (len(args) == 1 and + not kwargs and isinstance(args[0], str) and + hasattr(cls, '__rfm_cast__')): + return cls.__rfm_cast__(*args) + else: + obj = cls.__new__(cls, *args, **kwargs) + obj.__init__(*args, **kwargs) + return obj - def _check_conv_type(cls, s): - if not isinstance(s, str): - raise TypeError('cannot convert from non-string types') +# Metaclasses that implement the isinstance logic for the different builtin +# container types + +class _ContainerType(Type): + def register_container_type(cls): + cls.register(cls._type) -# Metaclasses that implement the isinstance logic for the different aggregate -# types -class _ContainerType(_TypeFactory): +class _SequenceType(_ContainerType): '''A metaclass for containers with uniformly typed elements.''' def __init__(cls, name, bases, namespace): @@ -113,7 +121,7 @@ def __init__(cls, name, bases, namespace): cls._elem_type = None cls._bases = bases cls._namespace = namespace - cls.register_wrapped_type() + cls.register_container_type() def __instancecheck__(cls, inst): if not issubclass(type(inst), cls): @@ -132,21 +140,20 @@ def __getitem__(cls, elem_type): raise TypeError('invalid type specification for container type: ' 'expected ContainerType[elem_type]') - ret = _ContainerType('%s[%s]' % (cls.__name__, elem_type.__name__), - cls._bases, cls._namespace) + ret = _SequenceType('%s[%s]' % (cls.__name__, elem_type.__name__), + cls._bases, cls._namespace) ret._elem_type = elem_type - ret.register_wrapped_type() + ret.register_container_type() cls.register(ret) return ret - def __call__(cls, s): - cls._check_conv_type(s) + def __rfm_cast__(cls, s): container_type = cls._type elem_type = cls._elem_type return container_type(elem_type(e) for e in s.split(',')) -class _TupleType(_ContainerType): +class _TupleType(_SequenceType): '''A metaclass for tuples. Tuples may contain uniformly-typed elements or non-uniformly typed ones. @@ -183,12 +190,11 @@ def __getitem__(cls, elem_types): ) ret = _TupleType(cls_name, cls._bases, cls._namespace) ret._elem_type = elem_types - ret.register_wrapped_type() + ret.register_container_type() cls.register(ret) return ret - def __call__(cls, s): - cls._check_conv_type(s) + def __rfm_cast__(cls, s): container_type = cls._type elem_types = cls._elem_type elems = s.split(',') @@ -203,7 +209,7 @@ def __call__(cls, s): ) -class _MappingType(_TypeFactory): +class _MappingType(_ContainerType): '''A metaclass for type checking mapping types.''' def __init__(cls, name, bases, namespace): @@ -212,7 +218,7 @@ def __init__(cls, name, bases, namespace): cls._value_type = None cls._bases = bases cls._namespace = namespace - cls.register_wrapped_type() + cls.register_container_type() def __instancecheck__(cls, inst): if not issubclass(type(inst), cls): @@ -245,12 +251,11 @@ def __getitem__(cls, typespec): ret = _MappingType(cls_name, cls._bases, cls._namespace) ret._key_type = key_type ret._value_type = value_type - ret.register_wrapped_type() + ret.register_container_type() cls.register(ret) return ret - def __call__(cls, s): - cls._check_conv_type(s) + def __rfm_cast__(cls, s): mappping_type = cls._type key_type = cls._key_type value_type = cls._value_type @@ -269,7 +274,7 @@ def __call__(cls, s): return mappping_type(seq) -class _StrType(_ContainerType): +class _StrType(_SequenceType): '''A metaclass for type checking string types.''' def __instancecheck__(cls, inst): @@ -290,12 +295,11 @@ def __getitem__(cls, patt): ret = _StrType("%s[r'%s']" % (cls.__name__, patt), cls._bases, cls._namespace) ret._elem_type = patt - ret.register_wrapped_type() + ret.register_container_type() cls.register(ret) return ret - def __call__(cls, s): - cls._check_conv_type(s) + def __rfm_cast__(cls, s): return s @@ -303,11 +307,11 @@ class Dict(metaclass=_MappingType): _type = dict -class List(metaclass=_ContainerType): +class List(metaclass=_SequenceType): _type = list -class Set(metaclass=_ContainerType): +class Set(metaclass=_SequenceType): _type = set diff --git a/unittests/test_typecheck.py b/unittests/test_typecheck.py index 5820d638dd..de86b63229 100644 --- a/unittests/test_typecheck.py +++ b/unittests/test_typecheck.py @@ -197,3 +197,34 @@ def __hash__(self): assert isinstance(d, types.Dict[int, C]) assert isinstance(cd, types.Dict[C, int]) assert isinstance(t, types.Tuple[int, C, str]) + + +def test_custom_types_conversion(): + class X(metaclass=types.Type): + def __init__(self, x): + self.x = x + + @classmethod + def __rfm_cast__(cls, s): + return X(int(s)) + + class Y: + def __init__(self, s): + self.y = int(s) + + class Z: + def __init__(self, x, y): + self.z = x + y + + assert X('3').x == 3 + assert X(3).x == 3 + assert X(x='foo').x == 'foo' + + with pytest.raises(TypeError): + X(3, 4) + + with pytest.raises(TypeError): + X(s=3) + + assert Y('1').y == 1 + assert Z(5, 3).z == 8 From 4aee5e7b911138fa5a7baf4acef3e6dd9ae8ca7b Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sat, 14 Aug 2021 00:30:27 +0200 Subject: [PATCH 06/21] Address PR comments (partially) --- reframe/core/decorators.py | 31 ++++++----- reframe/core/fields.py | 53 ++++++++++++++++-- reframe/core/meta.py | 5 +- reframe/core/variables.py | 54 ++++--------------- reframe/frontend/cli.py | 33 ++++++------ reframe/frontend/loader.py | 53 +++++++++--------- .../resources/checks_unlisted/externalvars.py | 7 ++- unittests/test_cli.py | 2 +- unittests/test_fields.py | 17 ++++++ unittests/test_variables.py | 37 ------------- 10 files changed, 146 insertions(+), 146 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 502fd1219b..849ac18c2c 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -19,6 +19,7 @@ import traceback import reframe.utility.osext as osext +import reframe.core.fields as fields import reframe.core.warnings as warn import reframe.core.hooks as hooks from reframe.core.exceptions import ReframeSyntaxError, SkipTestError, what @@ -59,13 +60,17 @@ def skip(self, test): '''Add a test to the skip set.''' self._skip_tests.add(test) - def instantiate_all(self): + def instantiate_all(self, extvars=None): '''Instantiate all the registered tests.''' ret = [] for test, variants in self._tests.items(): if test in self._skip_tests: continue + extvars = extvars or {} + for name, value in extvars.items(): + setattr(test, name, fields.make_convertible(value)) + for args, kwargs in variants: try: ret.append(test(*args, **kwargs)) @@ -109,18 +114,18 @@ def _register_parameterized_test(cls, args=None): this case to consume the parameter space. Otherwise, the regression test parameters would simply be initialized to None. ''' - def _instantiate(cls, inst_args, *args, **kwargs): - kwargs['_rfm_use_params'] = True - if isinstance(inst_args, collections.abc.Sequence): - args += inst_args - elif isinstance(inst_args, collections.abc.Mapping): - kwargs.update(args) - - return cls(*args, **kwargs) - - def _instantiate_all(*args, **kwargs): + def _instantiate(cls, args): + if isinstance(args, collections.abc.Sequence): + return cls(*args, _rfm_use_params=True) + elif isinstance(args, collections.abc.Mapping): + args['_rfm_use_params'] = True + return cls(**args) + elif args is None: + return cls(_rfm_use_params=True) + + def _instantiate_all(): ret = [] - for cls, inst_args in mod.__rfm_test_registry: + for cls, args in mod.__rfm_test_registry: try: if cls in mod.__rfm_skip_tests: continue @@ -128,7 +133,7 @@ def _instantiate_all(*args, **kwargs): mod.__rfm_skip_tests = set() try: - ret.append(_instantiate(cls, inst_args, *args, **kwargs)) + ret.append(_instantiate(cls, args)) except SkipTestError as e: getlogger().warning(f'skipping test {cls.__qualname__!r}: {e}') except Exception: diff --git a/reframe/core/fields.py b/reframe/core/fields.py index 263601cb90..35790b91a7 100644 --- a/reframe/core/fields.py +++ b/reframe/core/fields.py @@ -15,6 +15,26 @@ from reframe.utility import ScopedDict +class _Convertible: + '''Wrapper for values that allowed to be converted implicitly''' + + __slots__ = ('value') + + def __init__(self, value): + self.value = value + + +def make_convertible(value): + return _Convertible(value) + + +def remove_convertible(value): + if isinstance(value, _Convertible): + return value.value + else: + return value + + class Field: '''Base class for attribute validators.''' @@ -34,7 +54,7 @@ def __get__(self, obj, objtype): (objtype.__name__, self._name)) from None def __set__(self, obj, value): - obj.__dict__[self._name] = value + obj.__dict__[self._name] = remove_convertible(value) class TypedField(Field): @@ -58,8 +78,33 @@ def _check_type(self, value): (self._name, value, typedescr)) def __set__(self, obj, value): - self._check_type(value) - super().__set__(obj, value) + try: + self._check_type(value) + except TypeError: + raw_value = remove_convertible(value) + if raw_value is value: + # value was not convertible; reraise + raise + + # Try to convert value to any of the supported types + value = raw_value + for t in self._types: + try: + value = t(value) + except TypeError: + continue + else: + super().__set__(obj, value) + return + + # Conversion failed + raise TypeError( + f'failed to set field {self._name!r}: ' + f'could not convert to any of the supported types: ' + f'{self._types}' + ) + else: + super().__set__(obj, value) class ConstantField(Field): @@ -92,6 +137,7 @@ def __init__(self, *other_types): super().__init__(str, int, float, *other_types) def __set__(self, obj, value): + value = remove_convertible(value) self._check_type(value) if isinstance(value, str): time_match = re.match(r'^((?P\d+)d)?' @@ -123,6 +169,7 @@ def __init__(self, valuetype, *other_types): ScopedDict, *other_types) def __set__(self, obj, value): + value = remove_convertible(value) self._check_type(value) if not isinstance(value, ScopedDict): value = ScopedDict(value) if value is not None else value diff --git a/reframe/core/meta.py b/reframe/core/meta.py index bfaaa64371..76fc7bab76 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -335,12 +335,13 @@ def __call__(cls, *args, **kwargs): # Intercept constructor arguments _rfm_use_params = kwargs.pop('_rfm_use_params', False) - _rfm_external_vals = kwargs.pop('_rfm_external_vals', {}) + obj = cls.__new__(cls, *args, **kwargs) # Insert the var & param spaces - cls._rfm_var_space.inject(obj, cls, _rfm_external_vals) + cls._rfm_var_space.inject(obj, cls) cls._rfm_param_space.inject(obj, cls, _rfm_use_params) + obj.__init__(*args, **kwargs) return obj diff --git a/reframe/core/variables.py b/reframe/core/variables.py index a62b507a20..2727a290c1 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -34,47 +34,27 @@ class TestVar: during class instantiation. To support injecting attributes into the variable, this class implements a - separate dict `_attrs` where these will be stored. + separate dict `__attrs__` where these will be stored. :meta private: ''' __slots__ = ( - '_attrs', '_conv_fn', '_default_value', 'field', 'name' + 'field', '_default_value', 'name', '__attrs__' ) def __init__(self, *args, **kwargs): field_type = kwargs.pop('field', fields.TypedField) self._default_value = kwargs.pop('value', Undefined) + if not issubclass(field_type, fields.Field): raise TypeError( f'field {field_type!r} is not derived from ' f'{fields.Field.__qualname__}' ) - conv_fn = kwargs.pop('conv', None) self.field = field_type(*args, **kwargs) - self._attrs = dict() - - def _default_conv(s): - errmsg = f'cannot convert {s!r} for setting variable {self.name!r}' - if not isinstance(self.field, fields.TypedField): - raise TypeError(errmsg) - - for typ in self.field.valid_types: - if issubclass(typ, type(None)) and s == 'None': - # Treat `None` specially - return None - - try: - return typ(s) - except Exception: - continue - - raise TypeError(errmsg) - - # Set the conversion function - self._conv_fn = conv_fn or _default_conv + self.__attrs__ = dict() def is_defined(self): return self._default_value is not Undefined @@ -85,9 +65,6 @@ def undefine(self): def define(self, value): self._default_value = value - def define_from_string(self, value): - self.define(self._conv_fn(value)) - @property def default_value(self): # Variables must be returned by-value to prevent an instance from @@ -98,21 +75,21 @@ def default_value(self): @property def attrs(self): # Variable attributes must also be returned by-value. - return copy.deepcopy(self._attrs) + return copy.deepcopy(self.__attrs__) def __set_name__(self, owner, name): self.name = name def __setattr__(self, name, value): - '''Set any additional variable attribute into _attrs.''' + '''Set any additional variable attribute into __attrs__.''' if name in self.__slots__: super().__setattr__(name, value) else: - self._attrs[name] = value + self.__attrs__[name] = value def __getattr__(self, name): - '''Attribute lookup into _attrs.''' - attrs = self.__getattribute__('_attrs') + '''Attribute lookup into __attrs__.''' + attrs = self.__getattribute__('__attrs__') try: return attrs[name] except KeyError: @@ -547,24 +524,13 @@ def sanity(self, cls, illegal_names=None): f'{cls.__qualname__!r}' ) - def inject(self, obj, cls, external_vals=None): + def inject(self, obj, cls): '''Insert the vars in the regression test. :param obj: The test object. :param cls: The test class. - :param external_vals: A dictionary of variables and their values as - set by external sources (i.e., command-line) ''' - if external_vals: - for name, val in external_vals.items(): - try: - var = self[name] - except KeyError: - continue - else: - var.define_from_string(val) - for name, var in self.items(): setattr(cls, name, var.field) getattr(cls, name).__set_name__(obj, name) diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py index 35da5b6f94..0d5af40c09 100644 --- a/reframe/frontend/cli.py +++ b/reframe/frontend/cli.py @@ -716,10 +716,21 @@ def main(): ) check_search_path = site_config.get('general/0/check_search_path') - loader = RegressionCheckLoader( - load_path=check_search_path, - recurse=check_search_recursive - ) + # Collect any variables set from the command line + external_vars = {} + for expr in options.vars: + try: + lhs, rhs = expr.split('=', maxsplit=1) + except ValueError: + printer.warning( + f'invalid test variable assignment: {expr!r}; skipping' + ) + else: + external_vars[lhs] = rhs + + loader = RegressionCheckLoader(check_search_path, + check_search_recursive, + external_vars) def print_infoline(param, value): param = param + ':' @@ -754,20 +765,8 @@ def print_infoline(param, value): print_infoline('output directory', repr(session_info['prefix_output'])) printer.info('') try: - # Collect any variables set from the command line - external_vals = {} - for expr in options.vars: - try: - lhs, rhs = expr.split('=', maxsplit=1) - except ValueError: - printer.warning( - f'invalid test variable assignment: {expr!r}; skipping' - ) - else: - external_vals[lhs] = rhs - # Locate and load checks - checks_found = loader.load_all(_rfm_external_vals=external_vals) + checks_found = loader.load_all() printer.verbose(f'Loaded {len(checks_found)} test(s)') # Generate all possible test cases first; we will need them for diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index c72f02e5a8..63b723591f 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -39,7 +39,7 @@ def visit_ImportFrom(self, node): class RegressionCheckLoader: - def __init__(self, load_path, recurse=False): + def __init__(self, load_path, recurse=False, external_vars=None): # Expand any environment variables and symlinks load_path = [os.path.realpath(osext.expandvars(p)) for p in load_path] self._load_path = osext.unique_abs_paths(load_path, recurse) @@ -48,6 +48,9 @@ def __init__(self, load_path, recurse=False): # Loaded tests by name; maps test names to the file that were defined self._loaded = {} + # Variables set in the command line + self._external_vars = external_vars or {} + def _module_name(self, filename): '''Figure out a module name from filename. @@ -114,7 +117,7 @@ def prefix(self): def recurse(self): return self._recurse - def load_from_module(self, module, *args, **kwargs): + def load_from_module(self, module): '''Load user checks from module. This method tries to load the test registry from a given module and @@ -127,13 +130,6 @@ def load_from_module(self, module, *args, **kwargs): ''' from reframe.core.pipeline import RegressionTest - # Warn in case of old syntax - if hasattr(module, '_get_checks'): - getlogger().warning( - f'{module.__file__}: _get_checks() is no more supported ' - f'in test files: please use @reframe.simple_test decorator' - ) - # FIXME: Remove the legacy_registry after dropping parameterized_test registry = getattr(module, '_rfm_test_registry', None) legacy_registry = getattr(module, '_rfm_gettests', None) @@ -141,13 +137,19 @@ def load_from_module(self, module, *args, **kwargs): getlogger().debug('No tests registered') return [] - candidates = registry.instantiate_all() if registry else [] + extvars = self._external_vars + candidates = registry.instantiate_all(extvars) if registry else [] legacy_candidates = legacy_registry() if legacy_registry else [] + if extvars and legacy_candidates: + getlogger().warning( + "variables of tests using the deprecated " + "'@parameterized_test' decorator cannot be set externally; " + "please use the 'parameter' builtin in your tests" + ) # Merge registries candidates += legacy_candidates - - ret = [] + tests = [] for c in candidates: if not isinstance(c, RegressionTest): continue @@ -160,23 +162,23 @@ def load_from_module(self, module, *args, **kwargs): conflicted = self._loaded[c.name] except KeyError: self._loaded[c.name] = testfile - ret.append(c) + tests.append(c) else: raise NameConflictError( f'test {c.name!r} from {testfile!r} ' f'is already defined in {conflicted!r}' ) - getlogger().debug(f' > Loaded {len(ret)} test(s)') - return ret + getlogger().debug(f' > Loaded {len(tests)} test(s)') + return tests - def load_from_file(self, filename, force=False, *args, **kwargs): + def load_from_file(self, filename, force=False): if not self._validate_source(filename): return [] try: return self.load_from_module( - util.import_module_from_file(filename, force), *args, **kwargs + util.import_module_from_file(filename, force) ) except Exception: exc_info = sys.exc_info() @@ -191,26 +193,22 @@ def load_from_file(self, filename, force=False, *args, **kwargs): else: raise - def load_from_dir(self, dirname, recurse=False, - force=False, *args, **kwargs): + def load_from_dir(self, dirname, recurse=False, force=False): checks = [] for entry in os.scandir(dirname): if recurse and entry.is_dir(): - checks.extend( - self.load_from_dir(entry.path, recurse, - force, *args, **kwargs) - ) + checks += self.load_from_dir(entry.path, recurse, force) if (entry.name.startswith('.') or not entry.name.endswith('.py') or not entry.is_file()): continue - checks += self.load_from_file(entry.path, force, *args, **kwargs) + checks += self.load_from_file(entry.path, force) return checks - def load_all(self, force=False, *args, **kwargs): + def load_all(self, force=False): '''Load all checks in self._load_path. If a prefix exists, it will be prepended to each path. @@ -226,9 +224,8 @@ def load_all(self, force=False, *args, **kwargs): continue if os.path.isdir(d): - checks += self.load_from_dir(d, self._recurse, force, - *args, **kwargs) + checks += self.load_from_dir(d, self._recurse, force) else: - checks += self.load_from_file(d, force, *args, **kwargs) + checks += self.load_from_file(d, force) return checks diff --git a/unittests/resources/checks_unlisted/externalvars.py b/unittests/resources/checks_unlisted/externalvars.py index 81687ee4d1..7f88c15629 100644 --- a/unittests/resources/checks_unlisted/externalvars.py +++ b/unittests/resources/checks_unlisted/externalvars.py @@ -1,5 +1,6 @@ import reframe as rfm import reframe.utility.sanity as sn +import reframe.utility.typecheck as typ @rfm.simple_test @@ -7,6 +8,7 @@ class external_vars_test(rfm.RunOnlyRegressionTest): valid_systems = ['*'] valid_prog_environs = ['*'] foo = variable(int, value=1) + foolist = variable(typ.List[int]) executable = 'echo' @run_before('run') @@ -15,4 +17,7 @@ def set_exec_opts(self): @sanity_function def assert_foo(self): - return sn.assert_eq(sn.extractsingle(r'(\d+)', self.stdout, 1, int), 3) + return sn.all([ + sn.assert_eq(sn.extractsingle(r'(\d+)', self.stdout, 1, int), 3), + sn.assert_eq(self.foolist, [3, 4]) + ]) diff --git a/unittests/test_cli.py b/unittests/test_cli.py index 89871b5947..beff289bfe 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -782,7 +782,7 @@ def test_detect_host_topology_file(run_reframe, tmp_path): def test_external_vars(run_reframe): returncode, stdout, stderr = run_reframe( checkpath=['unittests/resources/checks_unlisted/externalvars.py'], - more_options=['-S', 'foo=3'] + more_options=['-S', 'foo=3', '-S', 'foolist=3,4'] ) assert 'Traceback' not in stdout assert 'Traceback' not in stderr diff --git a/unittests/test_fields.py b/unittests/test_fields.py index 6422f58c8f..b9a0333274 100644 --- a/unittests/test_fields.py +++ b/unittests/test_fields.py @@ -10,6 +10,7 @@ import reframe import reframe.core.fields as fields +import reframe.utility.typecheck as typ from reframe.core.warnings import ReframeDeprecationWarning from reframe.utility import ScopedDict @@ -71,6 +72,22 @@ def __init__(self, value): tester.field_any = 3 +def test_typed_field_convertible(): + class FieldTester: + fieldA = fields.TypedField(int, str) + fieldB = fields.TypedField(str, int) + fieldC = fields.TypedField(int) + + tester = FieldTester() + tester.fieldA = fields.make_convertible('10') + tester.fieldB = fields.make_convertible('10') + assert tester.fieldA == 10 + assert tester.fieldB == '10' + + with pytest.raises(TypeError): + tester.fieldC = fields.make_convertible(None) + + def test_timer_field(): class FieldTester: field = fields.TimerField() diff --git a/unittests/test_variables.py b/unittests/test_variables.py index 70df8aa2e2..fa86fdadad 100644 --- a/unittests/test_variables.py +++ b/unittests/test_variables.py @@ -451,40 +451,3 @@ class A(rfm.RegressionTest): assert math.trunc(npi) == -3 assert math.floor(npi) == -4 assert math.ceil(npi) == -3 - - -@pytest.fixture -def ConvTest(): - class _X(rfm.RegressionTest): - x = variable(int) - y = variable(float, value=3.141592) - z = variable(int, type(None)) - w = variable(int, value=3, conv=lambda x: 1) - - yield _X - - -def test_define_from_string(ConvTest): - x = ConvTest( - _rfm_external_vals={'x': '3', 'y': '3.14', 'z': 'None', 'w': '2'} - ) - assert x.x == 3 - assert x.y == 3.14 - assert x.z is None - assert x.w == 1 - - -def test_define_from_string_partial(ConvTest): - x = ConvTest(_rfm_external_vals={'x': '3', 'z': 'None'}) - assert x.x == 3 - assert x.y == 3.141592 - assert x.z is None - assert x.w == 3 - - -def test_define_from_string_errors(ConvTest): - with pytest.raises(TypeError): - ConvTest(_rfm_external_vals={'x': 'None'}) - - with pytest.raises(TypeError): - ConvTest(_rfm_external_vals={'z': '3.14'}) From 1f67a552128a3414b35f39542ee149a1a7d06ebc Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sat, 14 Aug 2021 23:07:02 +0200 Subject: [PATCH 07/21] Address PR comments --- reframe/utility/typecheck.py | 33 +++++++++++++++------------------ unittests/test_fields.py | 1 - unittests/test_typecheck.py | 4 ++-- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/reframe/utility/typecheck.py b/reframe/utility/typecheck.py index e3e25b3edc..84e6e0c7cf 100644 --- a/reframe/utility/typecheck.py +++ b/reframe/utility/typecheck.py @@ -93,23 +93,24 @@ import re -class Type(abc.ABCMeta): +class ConvertibleType(abc.ABCMeta): def __call__(cls, *args, **kwargs): - if (len(args) == 1 and - not kwargs and isinstance(args[0], str) and - hasattr(cls, '__rfm_cast__')): - return cls.__rfm_cast__(*args) - else: - obj = cls.__new__(cls, *args, **kwargs) - obj.__init__(*args, **kwargs) - return obj + if len(args) == 1: + cast_fn_name = f'__rfm_cast_{type(args[0]).__name__}__' + if hasattr(cls, cast_fn_name): + cast_fn = getattr(cls, cast_fn_name) + return cast_fn(args[0]) + + return super().__call__(*args, **kwargs) # Metaclasses that implement the isinstance logic for the different builtin # container types -class _ContainerType(Type): +class _ContainerType(ConvertibleType): def register_container_type(cls): + # Make sure that the class defines `_type` + assert hasattr(cls, '_type') cls.register(cls._type) @@ -143,11 +144,10 @@ def __getitem__(cls, elem_type): ret = _SequenceType('%s[%s]' % (cls.__name__, elem_type.__name__), cls._bases, cls._namespace) ret._elem_type = elem_type - ret.register_container_type() cls.register(ret) return ret - def __rfm_cast__(cls, s): + def __rfm_cast_str__(cls, s): container_type = cls._type elem_type = cls._elem_type return container_type(elem_type(e) for e in s.split(',')) @@ -190,11 +190,10 @@ def __getitem__(cls, elem_types): ) ret = _TupleType(cls_name, cls._bases, cls._namespace) ret._elem_type = elem_types - ret.register_container_type() cls.register(ret) return ret - def __rfm_cast__(cls, s): + def __rfm_cast_str__(cls, s): container_type = cls._type elem_types = cls._elem_type elems = s.split(',') @@ -251,11 +250,10 @@ def __getitem__(cls, typespec): ret = _MappingType(cls_name, cls._bases, cls._namespace) ret._key_type = key_type ret._value_type = value_type - ret.register_container_type() cls.register(ret) return ret - def __rfm_cast__(cls, s): + def __rfm_cast_str__(cls, s): mappping_type = cls._type key_type = cls._key_type value_type = cls._value_type @@ -295,11 +293,10 @@ def __getitem__(cls, patt): ret = _StrType("%s[r'%s']" % (cls.__name__, patt), cls._bases, cls._namespace) ret._elem_type = patt - ret.register_container_type() cls.register(ret) return ret - def __rfm_cast__(cls, s): + def __rfm_cast_str__(cls, s): return s diff --git a/unittests/test_fields.py b/unittests/test_fields.py index b9a0333274..9e6843b8ad 100644 --- a/unittests/test_fields.py +++ b/unittests/test_fields.py @@ -10,7 +10,6 @@ import reframe import reframe.core.fields as fields -import reframe.utility.typecheck as typ from reframe.core.warnings import ReframeDeprecationWarning from reframe.utility import ScopedDict diff --git a/unittests/test_typecheck.py b/unittests/test_typecheck.py index de86b63229..e459ec40c5 100644 --- a/unittests/test_typecheck.py +++ b/unittests/test_typecheck.py @@ -200,12 +200,12 @@ def __hash__(self): def test_custom_types_conversion(): - class X(metaclass=types.Type): + class X(metaclass=types.ConvertibleType): def __init__(self, x): self.x = x @classmethod - def __rfm_cast__(cls, s): + def __rfm_cast_str__(cls, s): return X(int(s)) class Y: From 7a52cf66e06e6b485509c747f94a5ba3d9cb20f4 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Mon, 16 Aug 2021 23:07:20 +0200 Subject: [PATCH 08/21] Address PR comments (v2) + support for setting vars per test --- reframe/core/decorators.py | 6 +----- reframe/frontend/loader.py | 18 +++++++++++++++--- .../resources/checks_unlisted/externalvars.py | 18 +++++++++++------- unittests/test_cli.py | 3 ++- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 849ac18c2c..44258919e5 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -60,17 +60,13 @@ def skip(self, test): '''Add a test to the skip set.''' self._skip_tests.add(test) - def instantiate_all(self, extvars=None): + def instantiate_all(self): '''Instantiate all the registered tests.''' ret = [] for test, variants in self._tests.items(): if test in self._skip_tests: continue - extvars = extvars or {} - for name, value in extvars.items(): - setattr(test, name, fields.make_convertible(value)) - for args, kwargs in variants: try: ret.append(test(*args, **kwargs)) diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 63b723591f..31d71a7a06 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -13,6 +13,7 @@ import sys import traceback +import reframe.core.fields as fields import reframe.utility as util import reframe.utility.osext as osext from reframe.core.exceptions import NameConflictError, is_severe, what @@ -117,6 +118,17 @@ def prefix(self): def recurse(self): return self._recurse + def _set_defaults(self, test_registry): + for test in test_registry: + for name, val in self._external_vars.items(): + if '.' in name: + testname, varname = name.split('.', maxsplit=1) + else: + testname, varname = test.__name__, name + + if testname == test.__name__: + setattr(test, varname, fields.make_convertible(val)) + def load_from_module(self, module): '''Load user checks from module. @@ -137,10 +149,10 @@ def load_from_module(self, module): getlogger().debug('No tests registered') return [] - extvars = self._external_vars - candidates = registry.instantiate_all(extvars) if registry else [] + self._set_defaults(registry) + candidates = registry.instantiate_all() if registry else [] legacy_candidates = legacy_registry() if legacy_registry else [] - if extvars and legacy_candidates: + if self._external_vars and legacy_candidates: getlogger().warning( "variables of tests using the deprecated " "'@parameterized_test' decorator cannot be set externally; " diff --git a/unittests/resources/checks_unlisted/externalvars.py b/unittests/resources/checks_unlisted/externalvars.py index 7f88c15629..2f2532a36d 100644 --- a/unittests/resources/checks_unlisted/externalvars.py +++ b/unittests/resources/checks_unlisted/externalvars.py @@ -4,20 +4,24 @@ @rfm.simple_test -class external_vars_test(rfm.RunOnlyRegressionTest): +class external_x(rfm.RunOnlyRegressionTest): valid_systems = ['*'] valid_prog_environs = ['*'] foo = variable(int, value=1) - foolist = variable(typ.List[int]) executable = 'echo' - @run_before('run') - def set_exec_opts(self): - self.executable_opts = [f'{self.foo}'] - @sanity_function def assert_foo(self): + return sn.assert_eq(self.foo, 3) + + +@rfm.simple_test +class external_y(external_x): + foolist = variable(typ.List[int]) + + @sanity_function + def assert_foolist(self): return sn.all([ - sn.assert_eq(sn.extractsingle(r'(\d+)', self.stdout, 1, int), 3), + sn.assert_eq(self.foo, 2), sn.assert_eq(self.foolist, [3, 4]) ]) diff --git a/unittests/test_cli.py b/unittests/test_cli.py index beff289bfe..64c7d5c403 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -782,7 +782,8 @@ def test_detect_host_topology_file(run_reframe, tmp_path): def test_external_vars(run_reframe): returncode, stdout, stderr = run_reframe( checkpath=['unittests/resources/checks_unlisted/externalvars.py'], - more_options=['-S', 'foo=3', '-S', 'foolist=3,4'] + more_options=['-S', 'external_x.foo=3', '-S', 'external_y.foo=2', + '-S', 'foolist=3,4'] ) assert 'Traceback' not in stdout assert 'Traceback' not in stderr From 611808dffc4caa8e29cd933c38a0707191ad3b94 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Mon, 16 Aug 2021 23:39:28 +0200 Subject: [PATCH 09/21] Support conversion from strings to None in the type system --- reframe/utility/typecheck.py | 22 ++++++++++++++++------ unittests/test_typecheck.py | 11 +++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/reframe/utility/typecheck.py b/reframe/utility/typecheck.py index 84e6e0c7cf..63b494a2c7 100644 --- a/reframe/utility/typecheck.py +++ b/reframe/utility/typecheck.py @@ -107,14 +107,14 @@ def __call__(cls, *args, **kwargs): # Metaclasses that implement the isinstance logic for the different builtin # container types -class _ContainerType(ConvertibleType): - def register_container_type(cls): +class _BuiltinType(ConvertibleType): + def __init__(cls, name, bases, namespace): # Make sure that the class defines `_type` assert hasattr(cls, '_type') cls.register(cls._type) -class _SequenceType(_ContainerType): +class _SequenceType(_BuiltinType): '''A metaclass for containers with uniformly typed elements.''' def __init__(cls, name, bases, namespace): @@ -122,7 +122,6 @@ def __init__(cls, name, bases, namespace): cls._elem_type = None cls._bases = bases cls._namespace = namespace - cls.register_container_type() def __instancecheck__(cls, inst): if not issubclass(type(inst), cls): @@ -208,7 +207,7 @@ def __rfm_cast_str__(cls, s): ) -class _MappingType(_ContainerType): +class _MappingType(_BuiltinType): '''A metaclass for type checking mapping types.''' def __init__(cls, name, bases, namespace): @@ -217,7 +216,6 @@ def __init__(cls, name, bases, namespace): cls._value_type = None cls._bases = bases cls._namespace = namespace - cls.register_container_type() def __instancecheck__(cls, inst): if not issubclass(type(inst), cls): @@ -300,6 +298,14 @@ def __rfm_cast_str__(cls, s): return s +class _NoneType(_BuiltinType): + def __rfm_cast_str__(cls, s): + if s == 'null': + return None + + raise TypeError(f"cannot convert string {s!r} to 'None'") + + class Dict(metaclass=_MappingType): _type = dict @@ -318,3 +324,7 @@ class Str(metaclass=_StrType): class Tuple(metaclass=_TupleType): _type = tuple + + +class Null(metaclass=_NoneType): + _type = type(None) diff --git a/unittests/test_typecheck.py b/unittests/test_typecheck.py index e459ec40c5..b85384d12f 100644 --- a/unittests/test_typecheck.py +++ b/unittests/test_typecheck.py @@ -9,6 +9,7 @@ def assert_type_hierarchy(builtin_type, ctype): + assert isinstance(ctype, type) assert issubclass(builtin_type, ctype) assert issubclass(ctype[int], ctype) assert issubclass(ctype[ctype[int]], ctype) @@ -228,3 +229,13 @@ def __init__(self, x, y): assert Y('1').y == 1 assert Z(5, 3).z == 8 + + +def test_none_type(): + assert isinstance(types.Null, type) + assert issubclass(type(None), types.Null) + assert isinstance(None, types.Null) + assert types.Null('null') == None + + with pytest.raises(TypeError): + types.Null('foo') From d7861e6f9fccba06a1f34ca7f6437876d58ae64b Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 17 Aug 2021 00:11:02 +0200 Subject: [PATCH 10/21] Fix PEP8 issues + remove unused imports --- reframe/core/decorators.py | 1 - unittests/test_typecheck.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 44258919e5..d31f71c967 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -19,7 +19,6 @@ import traceback import reframe.utility.osext as osext -import reframe.core.fields as fields import reframe.core.warnings as warn import reframe.core.hooks as hooks from reframe.core.exceptions import ReframeSyntaxError, SkipTestError, what diff --git a/unittests/test_typecheck.py b/unittests/test_typecheck.py index b85384d12f..7cdf22ab13 100644 --- a/unittests/test_typecheck.py +++ b/unittests/test_typecheck.py @@ -235,7 +235,7 @@ def test_none_type(): assert isinstance(types.Null, type) assert issubclass(type(None), types.Null) assert isinstance(None, types.Null) - assert types.Null('null') == None + assert types.Null('null') is None with pytest.raises(TypeError): types.Null('foo') From d4012435f647431ca65118ad17fe0a1071fabf50 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 18 Aug 2021 11:58:03 +0200 Subject: [PATCH 11/21] Address PR comments (v3) --- reframe/core/meta.py | 30 +++++++++++++++++------------- reframe/core/variables.py | 1 - reframe/frontend/loader.py | 4 +++- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 76fc7bab76..39942f9bd4 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -401,6 +401,22 @@ def __getattr__(cls, name): f'class {cls.__qualname__!r} has no attribute {name!r}' ) from None + def setvar(cls, name, value): + '''Set the value of a variable (unless the value is a descriptor).''' + + var_space = super().__getattribute__('_rfm_var_space') + if name in var_space: + if not hasattr(value, '__get__'): + var_space[name].define(value) + return + elif not var_space[name].field is value: + desc = '.'.join([cls.__qualname__, name]) + raise ReframeSyntaxError( + f'cannot override variable descriptor {desc!r}' + ) + else: + raise AttributeError(f'{cls.__name__!r} has no variable {name!r}') + def __setattr__(cls, name, value): '''Handle the special treatment required for variables and parameters. @@ -417,19 +433,8 @@ class attribute. This behavior does not apply when the assigned value is not allowed. This would break the parameter space internals. ''' - # Set the value of a variable (except when the value is a descriptor). try: - var_space = super().__getattribute__('_rfm_var_space') - if name in var_space: - if not hasattr(value, '__get__'): - var_space[name].define(value) - return - elif not var_space[name].field is value: - desc = '.'.join([cls.__qualname__, name]) - raise ReframeSyntaxError( - f'cannot override variable descriptor {desc!r}' - ) - + cls.setvar(name, value) except AttributeError: pass @@ -438,7 +443,6 @@ class attribute. This behavior does not apply when the assigned value param_space = super().__getattribute__('_rfm_param_space') if name in param_space.params: raise ReframeSyntaxError(f'cannot override parameter {name!r}') - except AttributeError: pass diff --git a/reframe/core/variables.py b/reframe/core/variables.py index 2727a290c1..c8777503ce 100644 --- a/reframe/core/variables.py +++ b/reframe/core/variables.py @@ -448,7 +448,6 @@ def join(self, other, cls): :param cls: the target class. ''' for key, var in other.items(): - # Make doubly declared vars illegal. Note that this will be # triggered when inheriting from multiple RegressionTest classes. if key in self.vars: diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 31d71a7a06..bb80e7159f 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -8,6 +8,7 @@ # import ast +import contextlib import inspect import os import sys @@ -127,7 +128,8 @@ def _set_defaults(self, test_registry): testname, varname = test.__name__, name if testname == test.__name__: - setattr(test, varname, fields.make_convertible(val)) + with contextlib.suppress(AttributeError): + test.setvar(varname, fields.make_convertible(val)) def load_from_module(self, module): '''Load user checks from module. From b2c01c4a2c88a22d7587ea9cfc813fd384b2ff3a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 18 Aug 2021 16:53:43 +0200 Subject: [PATCH 12/21] Convert `None` in loader and remove the `Null` meta-type --- reframe/frontend/loader.py | 8 +++++++- reframe/utility/typecheck.py | 16 ++++------------ .../resources/checks_unlisted/externalvars.py | 4 +++- unittests/test_cli.py | 2 +- unittests/test_typecheck.py | 10 ---------- 5 files changed, 15 insertions(+), 25 deletions(-) diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index bb80e7159f..6aca06e3c7 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -128,8 +128,14 @@ def _set_defaults(self, test_registry): testname, varname = test.__name__, name if testname == test.__name__: + # Treat special values + if val == '@none': + val = None + else: + val = fields.make_convertible(val) + with contextlib.suppress(AttributeError): - test.setvar(varname, fields.make_convertible(val)) + test.setvar(varname, val) def load_from_module(self, module): '''Load user checks from module. diff --git a/reframe/utility/typecheck.py b/reframe/utility/typecheck.py index 63b494a2c7..1b0934e59f 100644 --- a/reframe/utility/typecheck.py +++ b/reframe/utility/typecheck.py @@ -76,6 +76,10 @@ .. code-block:: none + type + | + | + | List / | / | @@ -298,14 +302,6 @@ def __rfm_cast_str__(cls, s): return s -class _NoneType(_BuiltinType): - def __rfm_cast_str__(cls, s): - if s == 'null': - return None - - raise TypeError(f"cannot convert string {s!r} to 'None'") - - class Dict(metaclass=_MappingType): _type = dict @@ -324,7 +320,3 @@ class Str(metaclass=_StrType): class Tuple(metaclass=_TupleType): _type = tuple - - -class Null(metaclass=_NoneType): - _type = type(None) diff --git a/unittests/resources/checks_unlisted/externalvars.py b/unittests/resources/checks_unlisted/externalvars.py index 2f2532a36d..dec5ec5aa9 100644 --- a/unittests/resources/checks_unlisted/externalvars.py +++ b/unittests/resources/checks_unlisted/externalvars.py @@ -18,10 +18,12 @@ def assert_foo(self): @rfm.simple_test class external_y(external_x): foolist = variable(typ.List[int]) + bar = variable(type(None), str) @sanity_function def assert_foolist(self): return sn.all([ sn.assert_eq(self.foo, 2), - sn.assert_eq(self.foolist, [3, 4]) + sn.assert_eq(self.foolist, [3, 4]), + sn.assert_eq(self.bar, None) ]) diff --git a/unittests/test_cli.py b/unittests/test_cli.py index 64c7d5c403..48abab4af3 100644 --- a/unittests/test_cli.py +++ b/unittests/test_cli.py @@ -783,7 +783,7 @@ def test_external_vars(run_reframe): returncode, stdout, stderr = run_reframe( checkpath=['unittests/resources/checks_unlisted/externalvars.py'], more_options=['-S', 'external_x.foo=3', '-S', 'external_y.foo=2', - '-S', 'foolist=3,4'] + '-S', 'foolist=3,4', '-S', 'bar=@none'] ) assert 'Traceback' not in stdout assert 'Traceback' not in stderr diff --git a/unittests/test_typecheck.py b/unittests/test_typecheck.py index 7cdf22ab13..3014267131 100644 --- a/unittests/test_typecheck.py +++ b/unittests/test_typecheck.py @@ -229,13 +229,3 @@ def __init__(self, x, y): assert Y('1').y == 1 assert Z(5, 3).z == 8 - - -def test_none_type(): - assert isinstance(types.Null, type) - assert issubclass(type(None), types.Null) - assert isinstance(None, types.Null) - assert types.Null('null') is None - - with pytest.raises(TypeError): - types.Null('foo') From 2bd226c7ab0282643b1824f2347bab940e96234c Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Wed, 18 Aug 2021 17:20:18 +0200 Subject: [PATCH 13/21] Order types in variables that support both None and strings --- reframe/core/buildsystems.py | 18 +++++++++--------- reframe/core/pipeline.py | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/reframe/core/buildsystems.py b/reframe/core/buildsystems.py index 9f2ce2e357..0813a70858 100644 --- a/reframe/core/buildsystems.py +++ b/reframe/core/buildsystems.py @@ -236,7 +236,7 @@ class Make(BuildSystem): #: #: :type: :class:`str` #: :default: :class:`None` - makefile = variable(str, type(None), value=None) + makefile = variable(type(None), str, value=None) #: The top-level directory of the code. #: @@ -245,7 +245,7 @@ class Make(BuildSystem): #: #: :type: :class:`str` #: :default: :class:`None` - srcdir = variable(str, type(None), value=None) + srcdir = variable(type(None), str, value=None) #: Limit concurrency for ``make`` jobs. #: This attribute controls the ``-j`` option passed to ``make``. @@ -349,7 +349,7 @@ class SingleSource(BuildSystem): #: :attr:`reframe.core.pipeline.RegressionTest.sourcepath` attribute. #: #: :type: :class:`str` or :class:`None` - srcfile = variable(str, type(None), value=None) + srcfile = variable(type(None), str, value=None) #: The executable file to be generated. #: @@ -357,7 +357,7 @@ class SingleSource(BuildSystem): #: :attr:`reframe.core.pipeline.RegressionTest.executable` attribute. #: #: :type: :class:`str` or :class:`None` - executable = variable(str, type(None), value=None) + executable = variable(type(None), str, value=None) #: The include path to be used for this compilation. #: @@ -381,7 +381,7 @@ class SingleSource(BuildSystem): #: - CUDA: `.cu`. #: #: :type: :class:`str` or :class:`None` - lang = variable(str, type(None), value=None) + lang = variable(type(None), str, value=None) def _auto_exec_name(self): return '%s.x' % os.path.splitext(self.srcfile)[0] @@ -475,13 +475,13 @@ class ConfigureBasedBuildSystem(BuildSystem): #: #: :type: :class:`str` #: :default: :class:`None` - srcdir = variable(str, type(None), value=None) + srcdir = variable(type(None), str, value=None) #: The CMake build directory, where all the generated files will be placed. #: #: :type: :class:`str` #: :default: :class:`None` - builddir = variable(str, type(None), value=None) + builddir = variable(type(None), str, value=None) #: Additional configuration options to be passed to the CMake invocation. #: @@ -790,7 +790,7 @@ class Spack(BuildSystem): #: .. versionchanged:: 3.7.3 #: The field is no longer required and the Spack environment will be #: automatically created if not provided. - environment = variable(typ.Str[r'\S+'], type(None), value=None) + environment = variable(type(None), typ.Str[r'\S+'], value=None) #: The directory where Spack will install the packages requested by this #: test. @@ -814,7 +814,7 @@ class Spack(BuildSystem): #: :default: :class:`None` #: #: .. versionadded:: 3.7.3 - install_tree = variable(typ.Str[r'\S+'], type(None), value=None) + install_tree = variable(type(None), typ.Str[r'\S+'], value=None) #: A list of additional specs to build and install within the given #: environment. diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 250e61d6be..a703397660 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -303,7 +303,7 @@ def pipeline_hooks(cls): #: .. versionchanged:: 3.0 #: Default value is now conditionally set to either ``'src'`` or #: :class:`None`. - sourcesdir = variable(str, type(None), value='src') + sourcesdir = variable(type(None), str, value='src') #: .. versionadded:: 2.14 #: From 4d6c76c8276fe594925420360b766f5a75c2c480 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 20 Aug 2021 00:19:10 +0200 Subject: [PATCH 14/21] Address PR comments (v4) --- reframe/core/meta.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 39942f9bd4..5116eb63f1 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -409,11 +409,15 @@ def setvar(cls, name, value): if not hasattr(value, '__get__'): var_space[name].define(value) return - elif not var_space[name].field is value: + elif var_space[name].field is not value: desc = '.'.join([cls.__qualname__, name]) raise ReframeSyntaxError( f'cannot override variable descriptor {desc!r}' ) + else: + # We are injecting the underlying descriptor of the variable, + # so set it as a normal class attribute + super(cls).__setattr__(name, value) else: raise AttributeError(f'{cls.__name__!r} has no variable {name!r}') @@ -433,10 +437,12 @@ class attribute. This behavior does not apply when the assigned value is not allowed. This would break the parameter space internals. ''' + # Try to treat `name` as variable try: cls.setvar(name, value) - except AttributeError: - pass + return + except AttributeError as e: + '''`name` is not a variable''' # Catch attempts to override a test parameter try: @@ -444,8 +450,9 @@ class attribute. This behavior does not apply when the assigned value if name in param_space.params: raise ReframeSyntaxError(f'cannot override parameter {name!r}') except AttributeError: - pass + '''Test is not parametrized''' + # Treat `name` as normal class attribute super().__setattr__(name, value) @property From 872ecf26fdd4fd526aaaf5c40a33e37538be2544 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 20 Aug 2021 11:45:55 +0200 Subject: [PATCH 15/21] Refactor setvar to return boolean --- reframe/core/meta.py | 61 +++++++++++++++++++++++--------------- reframe/frontend/loader.py | 3 +- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 5116eb63f1..0b19945475 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -402,24 +402,39 @@ def __getattr__(cls, name): ) from None def setvar(cls, name, value): - '''Set the value of a variable (unless the value is a descriptor).''' - - var_space = super().__getattribute__('_rfm_var_space') - if name in var_space: - if not hasattr(value, '__get__'): - var_space[name].define(value) - return - elif var_space[name].field is not value: - desc = '.'.join([cls.__qualname__, name]) - raise ReframeSyntaxError( - f'cannot override variable descriptor {desc!r}' - ) - else: - # We are injecting the underlying descriptor of the variable, - # so set it as a normal class attribute - super(cls).__setattr__(name, value) - else: - raise AttributeError(f'{cls.__name__!r} has no variable {name!r}') + '''Set the value of a variable. + + :param name: The name of the variable. + :param value: The value of the variable. + + :returns: :class:`True` if the variable was set. + A variable will *not* be set, if it does not exist or when an + attempt is made to set it with its underlying descriptor. + This happens during the variable injection time and it should be + delegated to the class' :func:`__setattr__` method. + + :raises ReframeSyntaxError: If an attempt is made to override a + variable with a descriptor other than its underlying one. + + ''' + + try: + var_space = super().__getattribute__('_rfm_var_space') + if name in var_space: + if not hasattr(value, '__get__'): + var_space[name].define(value) + return True + elif var_space[name].field is not value: + desc = '.'.join([cls.__qualname__, name]) + raise ReframeSyntaxError( + f'cannot override variable descriptor {desc!r}' + ) + else: + # Variable is being injected + return False + except AttributeError: + '''Catch early access attempt to the variable space.''' + return False def __setattr__(cls, name, value): '''Handle the special treatment required for variables and parameters. @@ -438,19 +453,17 @@ class attribute. This behavior does not apply when the assigned value ''' # Try to treat `name` as variable - try: - cls.setvar(name, value) + if cls.setvar(name, value): return - except AttributeError as e: - '''`name` is not a variable''' - # Catch attempts to override a test parameter + # Try to treat `name` as a parameter try: + # Catch attempts to override a test parameter param_space = super().__getattribute__('_rfm_param_space') if name in param_space.params: raise ReframeSyntaxError(f'cannot override parameter {name!r}') except AttributeError: - '''Test is not parametrized''' + '''Catch early access attempt to the parameter space.''' # Treat `name` as normal class attribute super().__setattr__(name, value) diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 6aca06e3c7..574d6ffaa1 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -134,8 +134,7 @@ def _set_defaults(self, test_registry): else: val = fields.make_convertible(val) - with contextlib.suppress(AttributeError): - test.setvar(varname, val) + test.setvar(varname, val) def load_from_module(self, module): '''Load user checks from module. From bda1c1784686d1628a4d719318f76d75a2d0299d Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 20 Aug 2021 11:51:31 +0200 Subject: [PATCH 16/21] Remove unused imports --- reframe/frontend/loader.py | 1 - 1 file changed, 1 deletion(-) diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 574d6ffaa1..3053b0c6d3 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -8,7 +8,6 @@ # import ast -import contextlib import inspect import os import sys From 76cfb17ad99c80ac12030616758a29f2351cb98d Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 20 Aug 2021 17:52:24 +0200 Subject: [PATCH 17/21] WIP: Document the -S option --- docs/manpage.rst | 47 +++++++++++++++++++++++++++++++++++++++++ reframe/frontend/cli.py | 5 +++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/docs/manpage.rst b/docs/manpage.rst index 40d0715e92..03a1b076e0 100644 --- a/docs/manpage.rst +++ b/docs/manpage.rst @@ -386,6 +386,53 @@ Options controlling ReFrame execution .. versionchanged:: 3.6.1 Multiple report files are now accepted. +.. option:: -S, --setvar=[TEST.]VAR=VAL + + Set variable ``VAR`` in all tests or optionally only in test ``TEST`` to ``VAL``. + + Multiple variables can be set at the same time by passing this option multiple times. + This option *cannot* change arbitrary test attributes, but only test variables declared with the :attr:`~reframe.core.pipeline.RegressionMixin.variable` built-in. + If an attempt is made to change an inexistent variable or a test parameter, a warning will be issued. + + ReFrame will try to convert ``VAL`` to the type of the variable. + If it does not succeed, a warning will be issued and the variable will not be set. + ``VAL`` can take the special value ``@none`` to denote that the variable must be set to :obj:`None`. + + Sequence and mapping types can also be set from the command line by using the following syntax: + + - Sequence types: ``-S seqvar=1,2,3,4`` + - Mapping types: ``-S mapvar=a:1,b:2,c:3`` + + Conversions to arbitrary objects are also supported. + See :doc:`blah-blah` for more details. + + + The optional ``TEST.`` prefix refers to the test class name (*not* the test name). + If the test name is not explicitly set through the :attr:`~reframe.core.pipeline.RegressionTest.name` attribute or if the test is not parametrised, then the class name is also the test name. + + Variable assignments passed from the command line happen *before* the test is instantiated and is the exact equivalent of assigning a new value to the variable *at the end* of the test class body. + This has a number of implications that users of this feature should be aware of: + + - In the following test, :attr:`num_tasks` will have always the value ``1`` regardless of any command-line assignment of the variable :attr:`foo`: + + .. code-block:: python + + class my_test(rfm.RegressionTest): + foo = variable(int, value=1) + num_tasks = foo + + - If the variable is set in any pipeline hook, the command line assignment will have an effect until the variable assignment in the pipeline hook is reached. + The variable will be then overwritten. + - The `test filtering <#test-filtering>`__ happens *after* a test is instantiated, so the only way to scope a variable assignment is to prefix it with the test class name. + However, this has some positive side effects: + + - Doing ``-S valid_systems='*'`` and ``-S valid_prog_environs='*'`` is the equivalent of passing the :option:`--skip-system-check` and :option:`--skip-prgenv-check` options. + - Users could alter the behavior of tests based on tag values that they pass from the command line, by changing the behavior of a test in a post-init hook based on the value of the :attr:`~reframe.core.pipeline.RegressionTest.tags` attribute. + + + .. versionadded:: 3.8.0 + + ---------------------------------- Options controlling job submission ---------------------------------- diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py index 0d5af40c09..144986a3c6 100644 --- a/reframe/frontend/cli.py +++ b/reframe/frontend/cli.py @@ -330,9 +330,10 @@ def main(): help='Skip programming environment check' ) run_options.add_argument( - '-S', '--setvar', action='append', metavar='VAR=VAL', + '-S', '--setvar', action='append', metavar='[TEST.]VAR=VAL', dest='vars', default=[], - help='Set test variable VAR to VAL in all selected tests' + help=('Set test variable VAR to VAL in all tests ' + 'or optionally in TEST only') ) run_options.add_argument( '--exec-policy', metavar='POLICY', action='store', From 882ea826e65d183ed69a1f1feffe1be44dc51ee5 Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 20 Aug 2021 23:13:03 +0200 Subject: [PATCH 18/21] Issue warning when variables cannot be set from cmdline Also: - Fix crash when test registry is empty --- reframe/frontend/loader.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 3053b0c6d3..246ffe8ea7 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -119,6 +119,10 @@ def recurse(self): return self._recurse def _set_defaults(self, test_registry): + if not test_registry: + return + + unset_vars = {} for test in test_registry: for name, val in self._external_vars.items(): if '.' in name: @@ -133,7 +137,17 @@ def _set_defaults(self, test_registry): else: val = fields.make_convertible(val) - test.setvar(varname, val) + if not test.setvar(varname, val): + unset_vars.setdefault(test.__name__, []) + unset_vars[test.__name__].append(varname) + + # Warn for all unset variables + for testname, varlist in unset_vars.items(): + varlist = ', '.join(f'{v!r}' for v in varlist) + getlogger().warning( + f'test {testname!r}: ' + f'the following variables were not set: {varlist}' + ) def load_from_module(self, module): '''Load user checks from module. From 5fafecae1db73b7a6ae95934a93bab237946272d Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Fri, 20 Aug 2021 23:39:44 +0200 Subject: [PATCH 19/21] Revert order of variables with string-or-None type --- docs/manpage.rst | 6 ++++++ reframe/core/buildsystems.py | 18 +++++++++--------- reframe/core/pipeline.py | 2 +- unittests/test_utility.py | 1 - 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/docs/manpage.rst b/docs/manpage.rst index 03a1b076e0..f625f7305a 100644 --- a/docs/manpage.rst +++ b/docs/manpage.rst @@ -428,7 +428,13 @@ Options controlling ReFrame execution - Doing ``-S valid_systems='*'`` and ``-S valid_prog_environs='*'`` is the equivalent of passing the :option:`--skip-system-check` and :option:`--skip-prgenv-check` options. - Users could alter the behavior of tests based on tag values that they pass from the command line, by changing the behavior of a test in a post-init hook based on the value of the :attr:`~reframe.core.pipeline.RegressionTest.tags` attribute. + - Users could force a test with required variables to run if they set these variables from the command line. + For example, the following test could only be run if invoked with ``-S num_tasks=``: + .. code-block:: python + + class my_test(rfm.RegressionTest): + num_tasks = required .. versionadded:: 3.8.0 diff --git a/reframe/core/buildsystems.py b/reframe/core/buildsystems.py index 0813a70858..9f2ce2e357 100644 --- a/reframe/core/buildsystems.py +++ b/reframe/core/buildsystems.py @@ -236,7 +236,7 @@ class Make(BuildSystem): #: #: :type: :class:`str` #: :default: :class:`None` - makefile = variable(type(None), str, value=None) + makefile = variable(str, type(None), value=None) #: The top-level directory of the code. #: @@ -245,7 +245,7 @@ class Make(BuildSystem): #: #: :type: :class:`str` #: :default: :class:`None` - srcdir = variable(type(None), str, value=None) + srcdir = variable(str, type(None), value=None) #: Limit concurrency for ``make`` jobs. #: This attribute controls the ``-j`` option passed to ``make``. @@ -349,7 +349,7 @@ class SingleSource(BuildSystem): #: :attr:`reframe.core.pipeline.RegressionTest.sourcepath` attribute. #: #: :type: :class:`str` or :class:`None` - srcfile = variable(type(None), str, value=None) + srcfile = variable(str, type(None), value=None) #: The executable file to be generated. #: @@ -357,7 +357,7 @@ class SingleSource(BuildSystem): #: :attr:`reframe.core.pipeline.RegressionTest.executable` attribute. #: #: :type: :class:`str` or :class:`None` - executable = variable(type(None), str, value=None) + executable = variable(str, type(None), value=None) #: The include path to be used for this compilation. #: @@ -381,7 +381,7 @@ class SingleSource(BuildSystem): #: - CUDA: `.cu`. #: #: :type: :class:`str` or :class:`None` - lang = variable(type(None), str, value=None) + lang = variable(str, type(None), value=None) def _auto_exec_name(self): return '%s.x' % os.path.splitext(self.srcfile)[0] @@ -475,13 +475,13 @@ class ConfigureBasedBuildSystem(BuildSystem): #: #: :type: :class:`str` #: :default: :class:`None` - srcdir = variable(type(None), str, value=None) + srcdir = variable(str, type(None), value=None) #: The CMake build directory, where all the generated files will be placed. #: #: :type: :class:`str` #: :default: :class:`None` - builddir = variable(type(None), str, value=None) + builddir = variable(str, type(None), value=None) #: Additional configuration options to be passed to the CMake invocation. #: @@ -790,7 +790,7 @@ class Spack(BuildSystem): #: .. versionchanged:: 3.7.3 #: The field is no longer required and the Spack environment will be #: automatically created if not provided. - environment = variable(type(None), typ.Str[r'\S+'], value=None) + environment = variable(typ.Str[r'\S+'], type(None), value=None) #: The directory where Spack will install the packages requested by this #: test. @@ -814,7 +814,7 @@ class Spack(BuildSystem): #: :default: :class:`None` #: #: .. versionadded:: 3.7.3 - install_tree = variable(type(None), typ.Str[r'\S+'], value=None) + install_tree = variable(typ.Str[r'\S+'], type(None), value=None) #: A list of additional specs to build and install within the given #: environment. diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index a703397660..250e61d6be 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -303,7 +303,7 @@ def pipeline_hooks(cls): #: .. versionchanged:: 3.0 #: Default value is now conditionally set to either ``'src'`` or #: :class:`None`. - sourcesdir = variable(type(None), str, value='src') + sourcesdir = variable(str, type(None), value='src') #: .. versionadded:: 2.14 #: diff --git a/unittests/test_utility.py b/unittests/test_utility.py index 09fe8e9028..bfc7737122 100644 --- a/unittests/test_utility.py +++ b/unittests/test_utility.py @@ -1544,7 +1544,6 @@ def test_jsonext_dumps(): # Classes to test JSON deserialization - class _D(jsonext.JSONSerializable): def __init__(self): self.a = 2 From 0c8fb2d09b1ec4b7dde05751e233dd5c4e8d574a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Sat, 21 Aug 2021 23:34:38 +0200 Subject: [PATCH 20/21] Finalize documentation of the `-S` feature --- docs/manpage.rst | 4 +++- reframe/utility/typecheck.py | 40 ++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/docs/manpage.rst b/docs/manpage.rst index f625f7305a..46fba8136c 100644 --- a/docs/manpage.rst +++ b/docs/manpage.rst @@ -404,7 +404,7 @@ Options controlling ReFrame execution - Mapping types: ``-S mapvar=a:1,b:2,c:3`` Conversions to arbitrary objects are also supported. - See :doc:`blah-blah` for more details. + See :class:`~reframe.utility.typecheck.ConvertibleType` for more details. The optional ``TEST.`` prefix refers to the test class name (*not* the test name). @@ -417,6 +417,7 @@ Options controlling ReFrame execution .. code-block:: python + @rfm.simple_test class my_test(rfm.RegressionTest): foo = variable(int, value=1) num_tasks = foo @@ -433,6 +434,7 @@ Options controlling ReFrame execution .. code-block:: python + @rfm.simple_test class my_test(rfm.RegressionTest): num_tasks = required diff --git a/reframe/utility/typecheck.py b/reframe/utility/typecheck.py index 1b0934e59f..71274dfc2c 100644 --- a/reframe/utility/typecheck.py +++ b/reframe/utility/typecheck.py @@ -98,6 +98,46 @@ class ConvertibleType(abc.ABCMeta): + '''A type that support conversions from other types. + + This is a metaclass that allows classes that use it to support arbitrary + conversions from other types using a cast-like syntax without having to + change their constructor: + + .. code-block:: python + + new_obj = convertible_type(another_type) + + For example, a class whose constructor accepts and :class:`int` may need + to support a cast-from-string conversion. This is particular useful if you + want a custom-typed test + :attr:`~reframe.core.pipeline.RegressionMixin.variable` to be able to be + set from the command line using the :option:`-S` option. + + In order to support such conversions, a class must use this metaclass and + define a class method, named as :obj:`__rfm_cast___`, for each of + the type conversion that needs to support . + + The following is an example of a class :class:`X` that its normal + constructor accepts two arguments but it also allows conversions from + string: + + .. code-block:: python + + class X(metaclass=ConvertibleType): + def __init__(self, x, y): + self.data = (x, y) + + @classmethod + def __rfm_cast_str__(cls, s): + return X(*(int(x) for x in s.split(',', maxsplit=1))) + + assert X(2, 3).data == X('2,3').data + + .. versionadded:: 3.8.0 + + ''' + def __call__(cls, *args, **kwargs): if len(args) == 1: cast_fn_name = f'__rfm_cast_{type(args[0]).__name__}__' From 403dbb4a7d6ac3b811d07be19043cca5802be2ec Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Tue, 24 Aug 2021 10:57:27 +0200 Subject: [PATCH 21/21] Address PR comments (v5) --- docs/manpage.rst | 5 ++--- reframe/core/pipeline.py | 6 ++++++ reframe/frontend/loader.py | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/manpage.rst b/docs/manpage.rst index 46fba8136c..269039f9cf 100644 --- a/docs/manpage.rst +++ b/docs/manpage.rst @@ -407,8 +407,7 @@ Options controlling ReFrame execution See :class:`~reframe.utility.typecheck.ConvertibleType` for more details. - The optional ``TEST.`` prefix refers to the test class name (*not* the test name). - If the test name is not explicitly set through the :attr:`~reframe.core.pipeline.RegressionTest.name` attribute or if the test is not parametrised, then the class name is also the test name. + The optional ``TEST.`` prefix refers to the test class name, *not* the test name. Variable assignments passed from the command line happen *before* the test is instantiated and is the exact equivalent of assigning a new value to the variable *at the end* of the test class body. This has a number of implications that users of this feature should be aware of: @@ -427,7 +426,7 @@ Options controlling ReFrame execution - The `test filtering <#test-filtering>`__ happens *after* a test is instantiated, so the only way to scope a variable assignment is to prefix it with the test class name. However, this has some positive side effects: - - Doing ``-S valid_systems='*'`` and ``-S valid_prog_environs='*'`` is the equivalent of passing the :option:`--skip-system-check` and :option:`--skip-prgenv-check` options. + - Passing ``-S valid_systems='*'`` and ``-S valid_prog_environs='*'`` is the equivalent of passing the :option:`--skip-system-check` and :option:`--skip-prgenv-check` options. - Users could alter the behavior of tests based on tag values that they pass from the command line, by changing the behavior of a test in a post-init hook based on the value of the :attr:`~reframe.core.pipeline.RegressionTest.tags` attribute. - Users could force a test with required variables to run if they set these variables from the command line. For example, the following test could only be run if invoked with ``-S num_tasks=``: diff --git a/reframe/core/pipeline.py b/reframe/core/pipeline.py index 250e61d6be..914b956f3f 100644 --- a/reframe/core/pipeline.py +++ b/reframe/core/pipeline.py @@ -212,6 +212,12 @@ def pipeline_hooks(cls): #: The name of the test. #: #: :type: string that can contain any character except ``/`` + #: :default: For non-parameterised tests, the default name is the test + #: class name. For parameterised tests, the default name is constructed + #: by concatenating the test class name and the string representations + #: of every test parameter: ``TestClassName__``. + #: Any non-alphanumeric value in a parameter's representation is + #: converted to ``_``. name = variable(typ.Str[r'[^\/]+']) #: List of programming environments supported by this test. diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 246ffe8ea7..179308a36b 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -119,7 +119,7 @@ def recurse(self): return self._recurse def _set_defaults(self, test_registry): - if not test_registry: + if test_registry is None: return unset_vars = {}