Add hook and exit_code keyword for more nuanced cache validation. #3637

greschd · 2019-12-10T17:27:20Z

Adds a method is_valid_cache to the Process class, which is called with a ProcessNode instance as an argument. This method is called by ProcessNode.is_valid_cache. It allows implementing
custom behavior in Process sub-classes, such as the CalcJob classes implemented in plugins.

As a default, the Process.is_valid_cache checks whether the exit code returned from the process has a - newly introduced - invalidates_cache attribute set to True. This is a simple way of marking the as cache invalid for a specific exit code only.

Question: Should all the documentation go into the "Caching: implementation details" section, or do we want to mention the spec.exit_code option in the "main" caching section?

greschd · 2019-12-10T17:28:57Z

Mentioning also @sphuber, who helped me with the design.

ltalirz · 2019-12-10T17:58:58Z

Should all the documentation go into the "Caching: implementation details" section, or do we want to mention the spec.exit_code option in the "main" caching section?

Let's put it this way - since the documentation is going to be restructured into tutorials/howtos/topics/reference, this information is certainly not going to go into the "how to enable caching".

So, I would keep it in the implementation section for the moment. We'll decide what to do with this section later.

greschd · 2019-12-10T20:50:38Z

Ready for review now.

sphuber

Thanks @greschd some minor changes and we need to discuss if the change in ExitCode which is technically breaking is acceptable

aiida/engine/processes/exit_code.py

aiida/engine/processes/process.py

sphuber · 2019-12-11T13:17:50Z

aiida/orm/nodes/process/process.py

+        # Catch errors when getting the process_class, or when it does not have
+        # a 'is_valid_cache' method.
+        try:
+            is_valid_cache_func = self.process_class.is_valid_cache


Can't this also throw other errors, specifically something like EntryPointError if the class cannot be loaded?

Hmm yes, indeed it can. It's not quite clear to me what the correct response to that is, though. If the process class can not be loaded due to an entry point error, essentially we can no longer perform the checks that might be implemented in that class. Should that fall back to being a valid cache, or invalid one?

Should we perhaps err on the safe side and consider it to be invalid. If the user really want them to be cached they should simply install the relevant plugin. I am just wondering if in this case we should emit a warning otherwise these calculations may be silently skipped for caching and might be difficult to track down

So we keep catching ValueError and return True (because that also happens in calcfunctions, I think), and for the rest log a warning and return False?

Side note: This seems like a pretty weird edge case -- you're just launching a new process, but then its process class somehow isn't available. How does this even happen? Not saying we shouldn't handle it, though.

Updated the code now.

Seems I have misunderstood this before: The ValueError happens whenever the process_class can not be loaded.

This clashes with the existing calcfunctions tests not because calcfunctions are inherently problematic, but because those tests define new calcfunctions inside the class. Since these can not be imported, they are not allowed to cache. That could be fixed by moving all calcfunctions to the module level.

Since calcfunctions should anyway be importable for the daemon to find them, I think it is a safe choice to disable caching when they are not importable. It is definitely a change in behavior though, so if you want to be strict on backwards-compatibility, it should be enabled instead.

docs/source/developer_guide/core/caching.rst

aiida/orm/nodes/process/process.py

Adds a method `is_valid_cache` to the `Process` class, which is called with a `ProcessNode` instance as an argument. This method is called by `ProcessNode.is_valid_cache`. It allows implementing custom behavior in `Process` sub-classes, such as the `CalcJob` classes implemented in plugins. As a default, the `Process.is_valid_cache` checks whether the exit code returned from the process has a -- newly introduced -- `invalidates_cache` attribute set to `True`. This is a simple way of marking the as cache invalid for a specific exit code only. Note that this is a backwards-incompatible change, which can break code that uses unpacking of ExitCode, for example status, message = ExitCode(...) The new ways of controlling caching are documented in the developer's documentation.

aiida/orm/nodes/process/process.py

This makes the calcfuntions importable, which is now required for them to be used in caching. Also catching only ValueError when getting the process_class, instead of a broad except.

greschd · 2019-12-12T15:38:57Z

I've implemented the changes in the calcfunctions tests - for me this is good to go.

Let me know if you'd rather have the version where inaccessible process_class returns True, and I'll revert the test changes and implement that instead.

sphuber · 2019-12-12T16:02:17Z

Thanks a lot @greschd

greschd requested a review from giovannipizzi December 10, 2019 17:27

greschd force-pushed the add_valid_cache_hook_at_calcjob branch from 67ebf2b to 2a206ff Compare December 10, 2019 17:47

greschd changed the title ~~Add hook and exit_code keyword for more nuanced cache validation.~~ [WIP] Add hook and exit_code keyword for more nuanced cache validation. Dec 10, 2019

greschd force-pushed the add_valid_cache_hook_at_calcjob branch 2 times, most recently from d70b40d to 4495486 Compare December 10, 2019 19:06

greschd changed the title ~~[WIP] Add hook and exit_code keyword for more nuanced cache validation.~~ Add hook and exit_code keyword for more nuanced cache validation. Dec 10, 2019

sphuber requested changes Dec 11, 2019

View reviewed changes

greschd requested a review from sphuber December 11, 2019 17:09

greschd force-pushed the add_valid_cache_hook_at_calcjob branch from b327b80 to b75a649 Compare December 12, 2019 08:34

greschd force-pushed the add_valid_cache_hook_at_calcjob branch from b75a649 to 91e85c5 Compare December 12, 2019 10:16

sphuber reviewed Dec 12, 2019

View reviewed changes

aiida/orm/nodes/process/process.py Outdated Show resolved Hide resolved

Define calcfunctions at module level in tests.

e02ee97

This makes the calcfuntions importable, which is now required for them to be used in caching. Also catching only ValueError when getting the process_class, instead of a broad except.

greschd requested a review from sphuber December 12, 2019 13:51

sphuber approved these changes Dec 12, 2019

View reviewed changes

sphuber merged commit f68b304 into aiidateam:develop Dec 12, 2019

greschd mentioned this pull request Aug 13, 2020

Define exit codes that should invalidate the cache aiidateam/aiida-wannier90#102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hook and exit_code keyword for more nuanced cache validation. #3637

Add hook and exit_code keyword for more nuanced cache validation. #3637

greschd commented Dec 10, 2019 •

edited

Loading

greschd commented Dec 10, 2019

ltalirz commented Dec 10, 2019

greschd commented Dec 10, 2019

sphuber left a comment

sphuber Dec 11, 2019

greschd Dec 11, 2019

sphuber Dec 12, 2019

greschd Dec 12, 2019

greschd Dec 12, 2019

greschd Dec 12, 2019

greschd commented Dec 12, 2019

sphuber commented Dec 12, 2019

Add hook and exit_code keyword for more nuanced cache validation. #3637

Add hook and exit_code keyword for more nuanced cache validation. #3637

Conversation

greschd commented Dec 10, 2019 • edited Loading

greschd commented Dec 10, 2019

ltalirz commented Dec 10, 2019

greschd commented Dec 10, 2019

sphuber left a comment

Choose a reason for hiding this comment

sphuber Dec 11, 2019

Choose a reason for hiding this comment

greschd Dec 11, 2019

Choose a reason for hiding this comment

sphuber Dec 12, 2019

Choose a reason for hiding this comment

greschd Dec 12, 2019

Choose a reason for hiding this comment

greschd Dec 12, 2019

Choose a reason for hiding this comment

greschd Dec 12, 2019

Choose a reason for hiding this comment

greschd commented Dec 12, 2019

sphuber commented Dec 12, 2019

greschd commented Dec 10, 2019 •

edited

Loading