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

Do not run process_handler when exit_codes=[]. #4380

Merged
merged 2 commits into from
Sep 23, 2020

Conversation

greschd
Copy link
Member

@greschd greschd commented Sep 21, 2020

Currently, a process_handler that gets passed an empty exit_codes list will always run. I think it would be more consistent to never run these, and always run only when exit_codes=None.

That's certainly debatable, but since the change is simple enough I decided to make a PR to decide this over instead of making an issue first.

One could also consider emitting a warning when an empty list is passed (since the handler is then essentially dead code) - but if the list of exit codes is "dynamically determined" it could lead to false warnings.

Context: I tried removing ERROR_COMPUTING_CHOLESKY from the following, and was surprised that it then always ran:

    @process_handler(priority=590, exit_codes=[
        PwCalculation.exit_codes.ERROR_COMPUTING_CHOLESKY,
    ])
    def handle_known_unrecoverable_failure(self, calculation):

Update: Just noticed that the docstring for the exit_codes parameter supports changing the behavior (or otherwise, the docstring would need changing):

    :param exit_codes: single or list of `ExitCode` instances. If defined, the handler will return `None` if the exit
        code set on the `node` does not appear in the `exit_codes`. This is useful to have a handler called only when
        the process failed with a specific exit code.

When a `process_handler` explicitly gets passed an empty
`exit_codes` list, it would previously always run. This is
now changed to _not_ run the handler instead.

The reason for this change is that it is more consistent with
the semantics of passing a lit of exit codes, where it only
triggers if the child process has any of the listed exit codes.
@codecov
Copy link

codecov bot commented Sep 21, 2020

Codecov Report

Merging #4380 into develop will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4380      +/-   ##
===========================================
- Coverage    79.34%   79.33%   -0.00%     
===========================================
  Files          470      470              
  Lines        34638    34638              
===========================================
- Hits         27480    27478       -2     
- Misses        7158     7160       +2     
Flag Coverage Δ
#django 73.19% <100.00%> (ø)
#sqlalchemy 72.40% <100.00%> (-<0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/engine/processes/workchains/utils.py 84.79% <100.00%> (ø)
aiida/engine/daemon/client.py 72.42% <0.00%> (-1.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93bde42...28e38a8. Read the comment docs.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Certainly is debatable, but don't feel like it now. You have made the case sufficiently ;) Thanks @greschd

@sphuber sphuber merged commit aa3b009 into aiidateam:develop Sep 23, 2020
@sphuber sphuber deleted the fix/empty_exit_codes_list branch September 23, 2020 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants