From 7fe484ba623f01e2c296553f41c2e8c71af76a70 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Mar 2025 10:58:13 +0100 Subject: [PATCH 1/8] Fix the type check after type conversion We cannot use `isinstance` on non-trivial types, i.e. our tuple-specifications. Incidently it works if the result is the type in the first element of the tuple, otherwise it fails with a `TypeError` because the remainder is not a type. The correct way is to use our `is_value_of_type`. 2 small corrections: 1. We don't need `EASY_TYPES` which had some types missing, e.g. `float`. Instead we can check if a type was passed and hence can be used with `isinstance`. 2. `convert_value_type` shouldn't be called when the type is already correct. We ensure that in all usages in framework code. So issue a warning for that case. Fixes #4785 --- easybuild/framework/easyconfig/types.py | 41 ++++++++++--------------- test/framework/type_checking.py | 4 +-- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index 78f021768d..7452861df4 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -143,7 +143,7 @@ def is_value_of_type(value, expected_type): """ type_ok = False - if expected_type in EASY_TYPES: + if isinstance(expected_type, type): # easy types can be checked using isinstance type_ok = isinstance(value, expected_type) @@ -230,30 +230,25 @@ def convert_value_type(val, typ): :param val: value to convert type of :param typ: target type """ - res = None - - if typ in EASY_TYPES and isinstance(val, typ): - _log.debug("Value %s is already of specified target type %s, no conversion needed", val, typ) - res = val - - elif typ in CHECKABLE_TYPES and is_value_of_type(val, typ): - _log.debug("Value %s is already of specified non-trivial target type %s, no conversion needed", val, typ) - res = val + # Shouldn't be called if the type is already correct + if is_value_of_type(val, typ): + _log.warning("Value %s is already of specified target type %s, no conversion needed", val, typ) + return val - elif typ in TYPE_CONVERSION_FUNCTIONS: + try: func = TYPE_CONVERSION_FUNCTIONS[typ] - _log.debug("Trying to convert value %s (type: %s) to %s using %s", val, type(val), typ, func) - try: - res = func(val) - _log.debug("Type conversion seems to have worked, new type: %s", type(res)) - except Exception as err: - raise EasyBuildError("Converting type of %s (%s) to %s using %s failed: %s", val, type(val), typ, func, err) + except KeyError: + raise EasyBuildError("No conversion function available (yet) for target type %s", typ) - if not isinstance(res, typ): - raise EasyBuildError("Converting value %s to type %s didn't work as expected: got %s", val, typ, type(res)) + _log.debug("Trying to convert value %s (type: %s) to %s using %s", val, type(val), typ, func) + try: + res = func(val) + _log.debug("Type conversion seems to have worked, new type: %s", type(res)) + except Exception as err: + raise EasyBuildError("Converting type of %s (%s) to %s using %s failed: %s", val, type(val), typ, func, err) - else: - raise EasyBuildError("No conversion function available (yet) for target type %s", typ) + if not is_value_of_type(res, typ): + raise EasyBuildError("Converting value %s to type %s didn't work as expected: got %s", val, typ, type(res)) return res @@ -662,9 +657,6 @@ def ensure_iterable_license_specs(specs): SANITY_CHECK_PATHS_DICT, SANITY_CHECK_PATHS_ENTRY, STRING_DICT, STRING_OR_TUPLE_LIST, STRING_OR_TUPLE_DICT, STRING_OR_TUPLE_OR_DICT_LIST, TOOLCHAIN_DICT, TUPLE_OF_STRINGS] -# easy types, that can be verified with isinstance -EASY_TYPES = [str, bool, dict, int, list, str, tuple, type(None)] - # type checking is skipped for easyconfig parameters names not listed in PARAMETER_TYPES PARAMETER_TYPES = { 'checksums': CHECKSUMS, @@ -681,7 +673,6 @@ def ensure_iterable_license_specs(specs): PARAMETER_TYPES[dep] = DEPENDENCIES TYPE_CONVERSION_FUNCTIONS = { - str: str, float: float, int: int, str: str, diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py index a9ce5df15b..45c9bb7e17 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -290,11 +290,11 @@ def test_convert_value_type(self): self.assertEqual(convert_value_type(('foo', 'bar'), LIST_OF_STRINGS), ['foo', 'bar']) self.assertEqual(convert_value_type((), LIST_OF_STRINGS), []) - # idempotency - self.assertEqual(convert_value_type('foo', str), 'foo') + # idempotency, although convert_value_type shouldn't be used if the type is already correct self.assertEqual(convert_value_type('foo', str), 'foo') self.assertEqual(convert_value_type(100, int), 100) self.assertEqual(convert_value_type(1.6, float), 1.6) + self.assertIs(convert_value_type(True, bool), True) self.assertEqual(convert_value_type(['foo', 'bar'], LIST_OF_STRINGS), ['foo', 'bar']) self.assertEqual(convert_value_type([], LIST_OF_STRINGS), []) From 74a09aa6354eff60a6a30f02a9dd31bd41999550 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Mar 2025 12:10:25 +0100 Subject: [PATCH 2/8] Fix type check of dependencies `dependencies` in easyconfigs are a list of strings or of tuples of strings (e.g. for toolchain specs with name-version) Add support for such tuples and allow them for dependencies to make the result of `to_dependencies` pass the check. --- easybuild/framework/easyconfig/types.py | 51 +++++++++++++++---------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index 7452861df4..bb2c3db247 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -34,7 +34,7 @@ from easybuild.base import fancylogger from easybuild.framework.easyconfig.format.format import DEPENDENCY_PARAMETERS -from easybuild.framework.easyconfig.format.format import SANITY_CHECK_PATHS_DIRS, SANITY_CHECK_PATHS_FILES +from easybuild.framework.easyconfig.format.format import SANITY_CHECK_PATHS_DIRS, SANITY_CHECK_PATHS_FILES, Dependency from easybuild.tools.build_log import EasyBuildError _log = fancylogger.getLogger('easyconfig.types', fname=False) @@ -576,6 +576,24 @@ def ensure_iterable_license_specs(specs): # these constants use functions defined in this module, so they needs to be at the bottom of the module # specific type: dict with only name/version as keys with string values, and optionally a hidden key with bool value # additional type requirements are specified as tuple of tuples rather than a dict, since this needs to be hashable +TUPLE_OF_STRINGS = (tuple, as_hashable({'elem_types': [str]})) +LIST_OF_STRINGS = (list, as_hashable({'elem_types': [str]})) +STRING_OR_TUPLE_LIST = (list, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS]})) +STRING_OR_TUPLE_TUPLE = (tuple, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS]})) +STRING_DICT = (dict, as_hashable( + { + 'elem_types': [str], + 'key_types': [str], + } +)) +STRING_OR_TUPLE_DICT = (dict, as_hashable( + { + 'elem_types': [str], + 'key_types': [str, TUPLE_OF_STRINGS], + } +)) +STRING_OR_TUPLE_OR_DICT_LIST = (list, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS, STRING_DICT]})) + TOOLCHAIN_DICT = (dict, as_hashable({ 'elem_types': { 'hidden': [bool], @@ -597,24 +615,13 @@ def ensure_iterable_license_specs(specs): 'opt_keys': ['full_mod_name', 'short_mod_name', 'toolchain', 'versionsuffix'], 'req_keys': ['name', 'version'], })) -DEPENDENCIES = (list, as_hashable({'elem_types': [DEPENDENCY_DICT]})) +DEPENDENCY_TUPLE = (tuple, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS, dict, bool]})) + +DEPENDENCY_TYPES = [DEPENDENCY_DICT, Dependency, DEPENDENCY_TUPLE] +# For iterated dependencies +DEPENDENCY_LIST = (list, as_hashable({'elem_types': DEPENDENCY_TYPES})) +DEPENDENCIES = (list, as_hashable({'elem_types': DEPENDENCY_TYPES + [DEPENDENCY_LIST]})) -TUPLE_OF_STRINGS = (tuple, as_hashable({'elem_types': [str]})) -LIST_OF_STRINGS = (list, as_hashable({'elem_types': [str]})) -STRING_OR_TUPLE_LIST = (list, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS]})) -STRING_DICT = (dict, as_hashable( - { - 'elem_types': [str], - 'key_types': [str], - } -)) -STRING_OR_TUPLE_DICT = (dict, as_hashable( - { - 'elem_types': [str], - 'key_types': [str, TUPLE_OF_STRINGS], - } -)) -STRING_OR_TUPLE_OR_DICT_LIST = (list, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS, STRING_DICT]})) SANITY_CHECK_PATHS_ENTRY = (list, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS, STRING_OR_TUPLE_DICT]})) SANITY_CHECK_PATHS_DICT = (dict, as_hashable({ 'elem_types': { @@ -653,9 +660,11 @@ def ensure_iterable_license_specs(specs): CHECKABLE_TYPES = [CHECKSUM_AND_TYPE, CHECKSUM_LIST, CHECKSUM_TUPLE, CHECKSUM_LIST_W_DICT, CHECKSUM_TUPLE_W_DICT, CHECKSUM_DICT, CHECKSUMS, - DEPENDENCIES, DEPENDENCY_DICT, LIST_OF_STRINGS, - SANITY_CHECK_PATHS_DICT, SANITY_CHECK_PATHS_ENTRY, STRING_DICT, STRING_OR_TUPLE_LIST, - STRING_OR_TUPLE_DICT, STRING_OR_TUPLE_OR_DICT_LIST, TOOLCHAIN_DICT, TUPLE_OF_STRINGS] + DEPENDENCIES, DEPENDENCY_DICT, DEPENDENCY_LIST, LIST_OF_STRINGS, + SANITY_CHECK_PATHS_DICT, SANITY_CHECK_PATHS_ENTRY, + STRING_DICT, STRING_OR_TUPLE_DICT, STRING_OR_TUPLE_LIST, STRING_OR_TUPLE_TUPLE, + STRING_OR_TUPLE_OR_DICT_LIST, DEPENDENCY_TUPLE, + TOOLCHAIN_DICT, TUPLE_OF_STRINGS] # type checking is skipped for easyconfig parameters names not listed in PARAMETER_TYPES PARAMETER_TYPES = { From 563fe7dabe50604f4c7eb81d67d5e576048e5536 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Mar 2025 12:25:20 +0100 Subject: [PATCH 3/8] Show name of type constant on error Add a function that gets the name of the constant out of the type. Use that to show e.g. "Converting [...] to type failed" --- easybuild/framework/easyconfig/types.py | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index bb2c3db247..f60ee1b99e 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -223,6 +223,18 @@ def check_type_of_param_value(key, val, auto_convert=False): return type_ok, newval +def type_spec_to_string(type_spec): + """ + Convert the given type to the name of a constant defined in this file if any. + If no (unique) constant is found the string representation of the type is returned. + """ + candidates = [name for name, value in globals().items() if name.isupper() and type_spec is value] + if len(candidates) == 1: + return f"" + else: + return str(type_spec) + + def convert_value_type(val, typ): """ Try to convert type of provided value to specific type. @@ -232,23 +244,26 @@ def convert_value_type(val, typ): """ # Shouldn't be called if the type is already correct if is_value_of_type(val, typ): - _log.warning("Value %s is already of specified target type %s, no conversion needed", val, typ) + _log.warning("Value %s is already of specified target type %s, no conversion needed", + val, type_spec_to_string(typ)) return val try: func = TYPE_CONVERSION_FUNCTIONS[typ] except KeyError: - raise EasyBuildError("No conversion function available (yet) for target type %s", typ) + raise EasyBuildError("No conversion function available (yet) for target type %s", type_spec_to_string(typ)) _log.debug("Trying to convert value %s (type: %s) to %s using %s", val, type(val), typ, func) try: res = func(val) _log.debug("Type conversion seems to have worked, new type: %s", type(res)) except Exception as err: - raise EasyBuildError("Converting type of %s (%s) to %s using %s failed: %s", val, type(val), typ, func, err) + raise EasyBuildError("Converting type of %s (%s) to %s using %s failed: %s", + val, type(val), type_spec_to_string(typ), func, err) if not is_value_of_type(res, typ): - raise EasyBuildError("Converting value %s to type %s didn't work as expected: got %s", val, typ, type(res)) + raise EasyBuildError("Converting value %s to type %s didn't work as expected: got %s of type %s", + val, type_spec_to_string(typ), res, type(res)) return res @@ -536,7 +551,7 @@ def _to_checksum(checksum, list_level=0, allow_dict=True): # When we already are in a tuple no further recursion is allowed -> set list_level very high return tuple(_to_checksum(x, list_level=99, allow_dict=allow_dict) for x in checksum) else: - return list(_to_checksum(x, list_level=list_level+1, allow_dict=allow_dict) for x in checksum) + return [_to_checksum(x, list_level=list_level+1, allow_dict=allow_dict) for x in checksum] elif isinstance(checksum, dict) and allow_dict: return {key: _to_checksum(value, allow_dict=False) for key, value in checksum.items()} From ef3b58339c0642b65421f66fc56d5813d1314c89 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Mar 2025 13:31:29 +0100 Subject: [PATCH 4/8] Adapt tests --- test/framework/type_checking.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py index 45c9bb7e17..c5611f0127 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -101,6 +101,9 @@ def test_check_type_of_param_value_deps(self): [{'name': 'foo', 'version': '1.2.3', 'versionsuffix': '', 'toolchain': {'name': 'GCC', 'version': '4.7'}}], [{'name': 'foo', 'version': '1.2.3', 'toolchain': {'name': 'GCC', 'version': '4.7'}}], [{'name': 'foo', 'version': '1.2.3'}, {'name': 'bar', 'version': '3.4.5'}], + [('foo', '1.2.3')], + [('foo', '1.2.3'), ('bar', '2')], + [[('foo', '1.2.3')], [('foo', '3.2.1')]], # Iterated dependencies ] for inp in inputs: self.assertEqual(check_type_of_param_value('dependencies', inp), (True, inp)) @@ -110,8 +113,8 @@ def test_check_type_of_param_value_deps(self): [{'name': 'foo'}], ['foo,1.2.3'], [{'foo': '1.2.3'}], - [('foo', '1.2.3')], - [{'name': 'foo', 'version': '1.2.3'}, ('bar', '3.4.5')], + [['foo', '1.2.3']], + [{'name': 'foo', 'version': '1.2.3'}, ['bar', '3.4.5']], [{'name': 'foo', 'version': '1.2.3', 'somekey': 'wrong'}], ] for inp in inputs: From 6e246b11672a791c4dd6eaea80236980c85a4e6e Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Tue, 4 Mar 2025 15:21:38 +0100 Subject: [PATCH 5/8] Add test checking type for None --- test/framework/type_checking.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py index c5611f0127..940311833f 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -451,6 +451,12 @@ def test_is_value_of_type(self): self.assertFalse(is_value_of_type(1, str)) self.assertFalse(is_value_of_type("foo", int)) + # checking for None + self.assertFalse(is_value_of_type(1, type(None))) + self.assertFalse(is_value_of_type('a', type(None))) + self.assertTrue(is_value_of_type(None, type(None))) + self.assertFalse(is_value_of_type(('a', 'b'), type(None))) + # list of strings check self.assertTrue(is_value_of_type([], LIST_OF_STRINGS)) self.assertTrue(is_value_of_type(['foo', 'bar'], LIST_OF_STRINGS)) From d06f7e4664080a8635babb1726341b923ea626d0 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 6 Mar 2025 10:56:10 +0100 Subject: [PATCH 6/8] Fix type check of patches and sanity_check_paths --- easybuild/framework/easyconfig/types.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index f60ee1b99e..7f0fd474c6 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -607,7 +607,7 @@ def ensure_iterable_license_specs(specs): 'key_types': [str, TUPLE_OF_STRINGS], } )) -STRING_OR_TUPLE_OR_DICT_LIST = (list, as_hashable({'elem_types': [str, TUPLE_OF_STRINGS, STRING_DICT]})) +STRING_OR_TUPLE_OR_DICT_LIST = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT]})) TOOLCHAIN_DICT = (dict, as_hashable({ 'elem_types': { @@ -643,8 +643,8 @@ def ensure_iterable_license_specs(specs): SANITY_CHECK_PATHS_FILES: [SANITY_CHECK_PATHS_ENTRY], SANITY_CHECK_PATHS_DIRS: [SANITY_CHECK_PATHS_ENTRY], }, - 'opt_keys': [], - 'req_keys': [SANITY_CHECK_PATHS_FILES, SANITY_CHECK_PATHS_DIRS], + 'opt_keys': [SANITY_CHECK_PATHS_FILES, SANITY_CHECK_PATHS_DIRS], + 'req_keys': [], })) # checksums is a list of checksums, one entry per file (source/patch) # each entry can be: From 555eacac18033295addca25445411a5458e15522 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 19 Mar 2025 13:25:21 +0100 Subject: [PATCH 7/8] Adapt tests of sanity_check_paths --- test/framework/type_checking.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/framework/type_checking.py b/test/framework/type_checking.py index 940311833f..c3a8a1022d 100644 --- a/test/framework/type_checking.py +++ b/test/framework/type_checking.py @@ -159,15 +159,15 @@ def test_check_type_of_param_value_sanity_check_paths(self): self.assertEqual(check_type_of_param_value('sanity_check_paths', inp), (True, inp)) inputs = [ - {}, - {'files': []}, + {'files2': []}, {'files': [], 'dirs': [], 'somethingelse': []}, {'files': [['bin/foo']], 'dirs': []}, {'files': [], 'dirs': [1]}, {'files': ['foo'], 'dirs': [(1, 2)]}, ] for inp in inputs: - self.assertEqual(check_type_of_param_value('sanity_check_paths', inp), (False, None)) + self.assertEqual(check_type_of_param_value('sanity_check_paths', inp), (False, None), + msg=f'Should be invalid: {inp}') # sanity_check_paths (auto-convert) inp = {'files': ['bin/foo', ['bin/bar', 'bin/baz']], 'dirs': [['lib', 'lib64', 'lib32']]} @@ -533,8 +533,8 @@ def test_is_value_of_type(self): # list element for 'files', should be string or tuple self.assertFalse(is_value_of_type({'files': ['f1', ['f2a', 'f2b']], 'dirs': []}, SANITY_CHECK_PATHS_DICT)) - # missing 'dirs' key - self.assertFalse(is_value_of_type({'files': ['f1', 'f2']}, SANITY_CHECK_PATHS_DICT)) + # missing (optional) 'dirs' key + self.assertTrue(is_value_of_type({'files': ['f1', 'f2']}, SANITY_CHECK_PATHS_DICT)) # tuple rather than list self.assertFalse(is_value_of_type({'files': (1, 2), 'dirs': []}, SANITY_CHECK_PATHS_DICT)) # int elements rather than strings/tuples-of-strings @@ -544,7 +544,7 @@ def test_is_value_of_type(self): # extra key is not allowed self.assertFalse(is_value_of_type({'files': [], 'dirs': [], 'foo': []}, SANITY_CHECK_PATHS_DICT)) # no keys at all - self.assertFalse(is_value_of_type({}, SANITY_CHECK_PATHS_DICT)) + self.assertTrue(is_value_of_type({}, SANITY_CHECK_PATHS_DICT)) def test_as_hashable(self): """Test as_hashable function.""" From 39d1d117036cca5e1cdcf692f8c16977b08eeb1b Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 19 Mar 2025 16:07:41 +0100 Subject: [PATCH 8/8] Fix definition of STRING_OR_TUPLE_DICT --- easybuild/framework/easyconfig/types.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/easybuild/framework/easyconfig/types.py b/easybuild/framework/easyconfig/types.py index 7f0fd474c6..0f33ccbd9f 100644 --- a/easybuild/framework/easyconfig/types.py +++ b/easybuild/framework/easyconfig/types.py @@ -603,8 +603,8 @@ def ensure_iterable_license_specs(specs): )) STRING_OR_TUPLE_DICT = (dict, as_hashable( { - 'elem_types': [str], - 'key_types': [str, TUPLE_OF_STRINGS], + 'key_types': [str], + 'elem_types': [str, TUPLE_OF_STRINGS], } )) STRING_OR_TUPLE_OR_DICT_LIST = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT]}))