Skip to content
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

Check the caching setting with python path need to be valid #6087

Merged

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Jul 18, 2023

fixes #6064

The identifier is checked when calling verdi config set -a caching.enabled_for, it is not raised in the first place because the validator only checks the format of the identifier but does not actually validate if it is a loadable module/class.
Look at other tests which are using identifier = "some_ident", those are now all failed.
I think it is worth to use importlib to check the idendifier string more strictly, what do you think? @sphuber

@sphuber
Copy link
Contributor

sphuber commented Jul 24, 2023

Thanks @unkcpz . This obviously changes (breaks) current behavior as it will now break existing caching configurations that might include rules that do not match importable resources. Also, it is a bit asymmetric, as the entry point based rules are not checked whether they can be loaded.

So here is a suggestions for two changes:

  1. Add the same check for entry points, trying to load it and raise if it cannot be loaded
  2. Make the import and entry point load-check optional by adding a boolean flag strict which is False by default

For the latter, we can decide whether we want to make it True by default in some future release that will allow backwards-incompatibility.

Raise a `ValueError` if the identifier cannot be imported, which will
help prevent accidental typos from appearing that the caching
configuration is being ignored.
@sphuber sphuber force-pushed the fix/6064/caching-rule-raise-for-unknown-module branch 2 times, most recently from 2d036ea to 3042b97 Compare September 1, 2023 11:32
@sphuber sphuber self-requested a review September 1, 2023 11:47
@sphuber
Copy link
Contributor

sphuber commented Sep 1, 2023

@unkcpz I implemented the changes I suggested. Please have a look. If you think the changes are good, I think we can merge this.

@sphuber
Copy link
Contributor

sphuber commented Sep 1, 2023

I might just need to update the docs a bit.

@unkcpz
Copy link
Member Author

unkcpz commented Sep 1, 2023

@sphuber Really sorry that I didn't manage to update and finish this, and thanks a lot for taking care of it.
Yes, the changes look good to me. Cheers!

So far, the caching configuration validation only considered whether the
defined identifiers were valid syntactically. This made it possible for
a user to specify a valid identifier but that didn't actually match a
class that can be imported or an entry point that cannot be loaded. If
this is due to a typo, the user may be confused why the caching config
seems to be ignored.

The caching control functionality adds the `strict` argument, which when
set to `True`, besides checking the syntax validity of an identifier,
will also try to import/load it and raise a `ValueError` if it fails. By
default it is set to `False` to maintain backwards compatibility.
@sphuber sphuber force-pushed the fix/6064/caching-rule-raise-for-unknown-module branch from 3042b97 to b9144bb Compare September 4, 2023 09:25
@sphuber sphuber merged commit f272e19 into aiidateam:main Sep 4, 2023
11 checks passed
@unkcpz unkcpz deleted the fix/6064/caching-rule-raise-for-unknown-module branch September 4, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider raising an error or issuing warning when caching rule does not match known class
2 participants