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

BaseRestartWorkChain: allow to override priority in handler_overrides #5546

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 29, 2022

Fixes #5545

The handler_overrides could so far be used to override the enabled
keyword of the corresponding handler. This would allow to disable or
enable a handler on a per process instance basis.

A user may want to do the same for the priority. To make this possible
the type of the handler_overrides is changed where the values should
now be dictionaries where the keys enabled and priority are
supported. These can be used to override the original values declared
in the source code of the work chain.

To provide backwards-compatibility, the old syntax is still supported
and automatically converted, with a deprecation warning being displayed.

@sphuber sphuber requested a review from espenfl May 29, 2022 21:37
@sphuber
Copy link
Contributor Author

sphuber commented May 29, 2022

Still want to add docs to this as the handler_overrides are not documented at all currently I believe.

@espenfl
Copy link
Contributor

espenfl commented May 30, 2022

@sphuber Thanks a lot. Happy to add docs. Where should it go?

espenfl
espenfl previously approved these changes May 30, 2022
instance method of the `process_class` that has been decorated with the `process_handler` decorator. The values
should be boolean.
The ``handler_overrides`` should be a dictionary where keys are strings that are the name of a process handler, i.e.
an instance method of the `process_class` that has been decorated with the `process_handler` decorator. The values
Copy link
Contributor

Choose a reason for hiding this comment

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

Should process_handler still be in single quotes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, missed that one. Fixed now.

…des`

The `handler_overrides` could so far be used to override the `enabled`
keyword of the corresponding handler. This would allow to disable or
enable a handler on a per process instance basis.

A user may want to do the same for the `priority`. To make this possible
the type of the `handler_overrides` is changed where the values should
now be dictionaries where the keys `enabled` and `priority` are
supported. These can be used to override the original values declared
in the source code of the work chain.

To provide backwards-compatibility, the old syntax is still supported
and automatically converted, with a deprecation warning being displayed.
@sphuber sphuber force-pushed the feature/5545/handler-overrides-priority branch from 0f686e2 to 8df037a Compare May 30, 2022 10:47
@sphuber
Copy link
Contributor Author

sphuber commented May 30, 2022

Thanks for the review @espenfl . I actually noticed there was a subtle bug that wasn't being tested for. The initial implementation would actually affect the handlers for multiple executions and not just the single instance. This is now fixed and explicitly tested. I have also added the docs.

@sphuber sphuber requested a review from espenfl May 30, 2022 11:03
@sphuber sphuber merged commit 7b8c61d into aiidateam:main May 30, 2022
@sphuber sphuber deleted the feature/5545/handler-overrides-priority branch May 30, 2022 11:16
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.

BaseRestartWorkChain let handler_overrides also override priority
2 participants