Skip to content

Ensure WorkChain does not exit unless stepper returns non-zero value#1945

Merged
sphuber merged 1 commit into
aiidateam:developfrom
sphuber:fix_1944_workchain_exits_when_it_should_not
Sep 6, 2018
Merged

Ensure WorkChain does not exit unless stepper returns non-zero value#1945
sphuber merged 1 commit into
aiidateam:developfrom
sphuber:fix_1944_workchain_exits_when_it_should_not

Conversation

@sphuber
Copy link
Copy Markdown
Contributor

@sphuber sphuber commented Sep 5, 2018

Fixes #1944

The engine exposes the possibility to the user of aborting a running
workchain from within its outline step functions, by simply returning
a non-zero integer or ExitCode instance with a non-zero status.
Any other return value should be ignored in terms of aborting the
running of the WorkChain. Due to a logical flaw in the _do_step
implementation of the WorkChain this heuristic was not being
respected and returning an ExitCode instance with zero status, would
cause the WorkChain to exit the control flow.

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #1945 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1945   +/-   ##
========================================
  Coverage    67.55%   67.55%           
========================================
  Files          321      321           
  Lines        33275    33275           
========================================
  Hits         22479    22479           
  Misses       10796    10796

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 a6ca7b2...941e238. Read the comment docs.

Copy link
Copy Markdown
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Apart for my comment, it's ok

Comment thread aiida/work/workchain.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this the right way to check also in py2/py3? (e.g. long in py2, ...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No clue to be honest. Surely @dev-zero would be able to tell us? :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you suspect that stepper_result could be a long in Python 2, the future-proof ways to check are:

import six

if isinstance(stepper_result, six.integer_types) and stepper_result != 0:

or

import numbers  # Python built-in since 2.6

if isinstance(stepper_result, numbers.Integral) and stepper_result != 0:

but for stepper_result to be a long in Python 2, you either have to set it explicitly using long(123), the ...L literal suffix or that it grows bigger than sys.maxint (which is in Python 2: 9223372036854775807) by itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since it can be a value that is returned by a user (return value from a WorkChain step function) and the spec is that it should allow positive integers greater than zero (which therefore technically should include longs), I will go with the six variant. Thanks @dev-zero

The engine exposes the possibility to the user of aborting a running
workchain from within its outline step functions, by simply returning
a non-zero integer or `ExitCode` instance with a non-zero status.
Any other return value should be ignored in terms of aborting the
running of the `WorkChain`. Due to a logical flaw in the `_do_step`
implementation of the `WorkChain` this heuristic was not being
respected and returning an `ExitCode` instance with zero status, would
cause the `WorkChain` to exit the control flow.
@sphuber sphuber force-pushed the fix_1944_workchain_exits_when_it_should_not branch from 941e238 to bb3b4ba Compare September 6, 2018 07:03
@sphuber sphuber merged commit a94294f into aiidateam:develop Sep 6, 2018
@sphuber sphuber deleted the fix_1944_workchain_exits_when_it_should_not branch September 6, 2018 07:30
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.

4 participants