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.

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

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Mar 4, 2025

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.

And improve the error message (CI failure was a bit hard to debug):
Now: Converting [...] to type <type 'DEPENDENCIES'> failed similar to:
Converting [...] to type <class 'int'> failed

Fixes #4785

I had to adapt the tests:

  • [('foo', '1.2.3')] was considered a wrong type for dependencies while it is not, as the fixed code confirms

Discussion

  1. What shall we do with the result of to_*? Shall we check against the expected type as defined to ensure what we pass is always "correct", shall we do a "soft" check only checking the outer type (list, tuple, dict, str, int...) or do no checks at all and assume it is correct?
    Problem is that we might want to delegate format checking to e.g. the EasyConfig instance which may print

== FAILED: Installation ended unsuccessfully (build directory: /dev/shm/zlib/1.3.1/GCCcore-13.3.0-test-debug): build failed (first 300 chars): Invalid checksum spec '112345689': should be a string (MD5 or SHA256), 2-tuple (type, value), or tuple of alternative checksum specs. (took 0 secs)

While we'd need to do that in the type-format check code or get e.g.

ERROR: Failed to process easyconfig /apps/c3se-test-easyconfigs/zlib-1.3.1-GCCcore-13.3.0-test.eb: Converting type of [123456] (<class 'list'>) to <type 'CHECKSUMS'> failed: 'Invalid checksums: [112345689]\n\tError: Unexpected type of "<class 'int'>": 123456'
  1. Shall we not do any checking at all and assume to_* does the right thing? This would mean that we have to do stricter checks in the handling code.
  2. Shall we allow "wildcard" types? I.e. a type of None in the type-spec-definition would mean "accept any type, don't check". That would allow selectively moving checks to other places while being explicit about it. We have something similar already if we use e.g. tuple instead of TUPLE_OF_STRINGS but then we can ignore the type of e.g. specific dict entries instead of all

See discussion in #4785

@Flamefire Flamefire force-pushed the fix-type-check-after-convert branch 3 times, most recently from 5c31577 to a638adb Compare March 5, 2025 08:21
@boegel boegel added this to the 5.x milestone Mar 19, 2025
@boegel boegel changed the base branch from 5.0.x to develop March 19, 2025 10:52
@boegel
Copy link
Member

boegel commented Mar 19, 2025

@Flamefire I changed to target branch in this PR from 5.0.x to develop, you should synchronize your PR branch with current develop branch (which has received a massive update after the release of EasyBuild v5.0.0, see #4825)

@boegel boegel added the bug fix label Mar 19, 2025
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 easybuilders#4785
`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.
Add a function that gets the name of the constant out of the type.
Use that to show e.g. "Converting [...] to type <type 'DEPENDENCIES'> failed"
@Flamefire Flamefire force-pushed the fix-type-check-after-convert branch from 0dad23d to 39d1d11 Compare March 19, 2025 15:10
@Flamefire
Copy link
Contributor Author

For the sanity_check_paths something came up: Shall we require the "files" and "dirs" key in the type check or have them optional there and handle it someplace else, e.g. as currently in the CI that fails if not both are set? I think in favor of better error messages defining most things as optional is a good idea. That's what I changed here and wanted to highlight the explicit decision.

Side note: I think we should not require both keys. A missing key should be equivalent to an empty list.

@akesandgren
Copy link
Contributor

Side note: I think we should not require both keys. A missing key should be equivalent to an empty list.

I agree on that part at least.

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

@boegel boegel modified the milestones: 5.x.x, 5.x Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

convert_value_type checks has broken code
3 participants