Skip to content

Fix the type check after type conversion #4787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Fix the type check after type conversion #4787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7fe484b
Fix the type check after type conversion
Flamefire Mar 4, 2025
74a09aa
Fix type check of dependencies
Flamefire Mar 4, 2025
563fe7d
Show name of type constant on error
Flamefire Mar 4, 2025
ef3b583
Adapt tests
Flamefire Mar 4, 2025
6e246b1
Add test checking type for None
Flamefire Mar 4, 2025
d06f7e4
Fix type check of patches and sanity_check_paths
Flamefire Mar 6, 2025
555eaca
Adapt tests of sanity_check_paths
Flamefire Mar 19, 2025
39d1d11
Fix definition of STRING_OR_TUPLE_DICT
Flamefire Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 64 additions & 49 deletions easybuild/framework/easyconfig/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -223,37 +223,47 @@ 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"<type '{candidates[0]}'>"
else:
return str(type_spec)


def convert_value_type(val, typ):
"""
Try to convert type of provided value to specific type.

:param val: value to convert type of
:param typ: target type
"""
res = None
# 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, type_spec_to_string(typ))
return val

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

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", type_spec_to_string(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), type_spec_to_string(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 of type %s",
val, type_spec_to_string(typ), res, type(res))

return res

Expand Down Expand Up @@ -541,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()}

Expand Down Expand Up @@ -581,6 +591,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(
{
'key_types': [str],
'elem_types': [str, TUPLE_OF_STRINGS],
}
))
STRING_OR_TUPLE_OR_DICT_LIST = (list, as_hashable({'elem_types': [str, tuple, STRING_DICT]}))

TOOLCHAIN_DICT = (dict, as_hashable({
'elem_types': {
'hidden': [bool],
Expand All @@ -602,32 +630,21 @@ 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': {
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],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually needs to be discussed: We currently allow missing dirs/files in sanity-check-paths and only fail it in CI.

We should decide where we want to handle this:

  • Make both optional when running easybuild but fail on CI for contributions
  • Make both fully optional and default them to empty lists in the code using it (sanity_check_step)
  • Make them required here (as before) and fill them with empty lists in the "conversion" function if not set, then we don't need to check if the keys exist in sanity_check_step. Optionally enforce non-empty lists on CI

'req_keys': [],
}))
# checksums is a list of checksums, one entry per file (source/patch)
# each entry can be:
Expand Down Expand Up @@ -658,12 +675,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]

# easy types, that can be verified with isinstance
EASY_TYPES = [str, bool, dict, int, list, str, tuple, type(None)]
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 = {
Expand All @@ -681,7 +697,6 @@ def ensure_iterable_license_specs(specs):
PARAMETER_TYPES[dep] = DEPENDENCIES

TYPE_CONVERSION_FUNCTIONS = {
str: str,
float: float,
int: int,
str: str,
Expand Down
29 changes: 19 additions & 10 deletions test/framework/type_checking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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:
Expand Down Expand Up @@ -156,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']]}
Expand Down Expand Up @@ -290,11 +293,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), [])

Expand Down Expand Up @@ -448,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))
Expand Down Expand Up @@ -524,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
Expand All @@ -535,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."""
Expand Down
Loading