How we include the non-builtin bool value for conditional predicate?

[unkcpz]

<class 'numpy.bool_'> is not a bool which cause the exception of "TypeError: The conditional predicate did not return a boolean" but it behave like a bool. I got this issue and it is quite subtle to debug. @sphuber How do you think we handle this condition?

I think we can add a more clear exception message with the result type printed. such like code below.

raise TypeError(f'The conditional predicate `{self._predicate.__name__}` has type {type(result)} did not return a boolean')

[sphuber]

Agree with the suggestion to include the actual type in the exception message but would propose the following phrasing:

raise TypeError(f'The conditional predicate `{self._predicate.__name__}` did not return a boolean, but `{type(result)}`')

[unkcpz]

Yep! Looks way better.

[danielhollas]

I've hit this issue in my workflow when I upgraded AiiDA last week. I was using AiiDAs Bool type in a while_ expression.

I find this new stricter behaviour rather surprising in this case, given that Bool otherwise behaves much like normal bool, e.g. Bool(True) is truthy and Bool(False) is falsy. Also, Bool is returned in logical expressions involving other AiiDA types:

$ Int(2) < 2
Bool(False)

So this change means we always need to access the underlying value in these expressions, making it somewhat less ergonomic.

Sidenote: It is strange that (in)equality operator returns normal bool

$ Int(2) == 2
True
$ Int(2) != 2
False

[danielhollas]

On a more general note: I would consider this to be a breaking change, since it broke otherwise perfectly valid workflow, but it was released in a patch version so one didn't even need to update aiida-core to be affected. So to me it would make more sense for this to be released via a minor version bump in plumpy.

[sphuber]

On a more general note: I would consider this to be a breaking change, since it broke otherwise perfectly valid workflow, but it was released in a patch version so one didn't even need to update aiida-core to be affected. So to me it would make more sense for this to be released via a minor version bump in plumpy.

We did indeed discuss this being a breaking change, but decided to release it anyway because we (incorrectly) deduced that cases that were relying on this behavior probably had incorrect logic in the workflow anyway, and so breaking it would be better than letting it continue running incorrectly. This is also how @unkcpz came across the original issue since he was returning an ExitCode in the hopes of aborting the workchain, but it happily accepted that as a truthy value. We clearly didn't think about other "bool"-like types though.

That being said, I am not sure how to fix this then. I mean, we could make the type check allow (bool, orm.Bool, numpy.bool) but then we might still bemissing things. On top of that, we would be introducing a cyclic dependency, having plumpy depend on aiida-core, which wouldn't even work. Any ideas how we can fix this? Because I still think that accepting everything which can lead to unintended behavior of the workchain is also not desirable.

Finally, releasing it as a minor version is not really possible, because we already have 0.22.0 out, but we cannot use this is in aiida-core yet, since there is an unresolved problem with the compatibility of the updated aio-pika and aiormq dependencies. So I was kind of forced to release it in 0.21 with a patch.

[danielhollas]

@sphuber how about detecting if the object has a __bool__ method implemented? Not sure if that is generally applicatble...

https://github.com/aiidateam/aiida-core/blob/0bc8ef06b57ee31a5062a225c23f04fc26d00980/aiida/orm/nodes/data/bool.py#L27

EDIT: Just checked that numpy.bool also has this dunder method. Also note that it is deprecated

Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations

Because I still think that accepting everything which can lead to unintended behavior of the workchain is also not desirable.

Yep, agree with this. :arrow_up:

[sphuber]

In b30ec7ed80da8c1eb9ed2240600303aafeae17cb I actually made an exception for None and emitted a warning instead of raising. Maybe we should just do this for any value that is not a boolean. This will give people the time to update their workchains, and then we can make it raise in the future. Would that be a better idea?

[danielhollas]

yes, that seems better, although it does not address the ergonomics concern.

What do you think of the __bool__ idea? I've just tested that replacing

if isinstance(result, bool):
    raise TypeError(f'The conditional predicate `{self._predicate.__name__}` did not return a boolean')

with

if getattr(result, '__bool__', None):
    raise TypeError(f'The conditional predicate `{self._predicate.__name__}` did not return a boolean')

works for my workflow that returns aiida.orm.Bool and should work for the numpy one as well. I checked the official docs for __bool__ and it seems like a reasonable middle ground https://docs.python.org/3/reference/datamodel.html#object.__bool__

[sphuber]

yes, that seems better, although it does not address the ergonomics concern.

What do you mean with the ergonomics concern? That there will be warnings emitted?

What do you think of the bool idea? I've just tested that replacing

It could work, but I am a bit worried that there will still be corner cases that we didn't think of and, as you remarked yourself, this could still be breaking existing workflows, which is not really nice for a minor/patch release. So I am thinking now whether we should simply err on the safe side and simply keep the original solution of just warning.

[sphuber]

Fixed in #265