Workchains: Accept but deprecate conditional predicates returning `None`

[sphuber]

In 800bcf154c0ea0d4576636b95d2ad2285adec266, the behavior of predicates passed to _Conditional instances was changed to raise if anything but a boolean was returned. This was to catch cases where users would return non-boolean values by accident, which would anyway would most likely to broken behavior of the workchain.

However, quite a number of existing implementations used to do something like the following in a predicate

def conditional_predicate(self):
    if some_condition is True:
         return True

In the case that some_condition was not equal to True, the predicate would return None which would be evaluated as "falsy" and so would function as if False had been returned.

In order to not break all implemenations using this behavior, None is still accepted and automatically converted to False. A UserWarning is emitted to warn that the behavior is deprecated. This is used in favor of a DeprecationWarning since those are not shown by default, which means the majority of users wouldn't see the deprecation warning.

[sphuber]

@unkcpz I noticed that our previous change would start breaking a lot of workchains out there. Fortunately we haven't release it in aiida-core yet, so we can release this with plumpy==0.21.5 and then update the requirement in the aiida-core/main branch.

[codecov[bot]]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 :tada:

Comparison is base (b6cd6fd) 90.81% compared to head (ae1fc1e) 90.83%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## release/0.21.5 #261 +/- ## ================================================== + Coverage 90.81% 90.83% +0.02% ================================================== Files 21 21 Lines 2970 2974 +4 ================================================== + Hits 2697 2701 +4 Misses 273 273 ``` | [Impacted Files](https://codecov.io/gh/aiidateam/plumpy/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | Coverage Δ | | |---|---|---| | [src/plumpy/workchains.py](https://codecov.io/gh/aiidateam/plumpy/pull/261?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#diff-c3JjL3BsdW1weS93b3JrY2hhaW5zLnB5) | `94.39% <100.00%> (+0.06%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[unkcpz]

Thanks @sphuber, can you point out which workchain return None as the conditional predicate? I thought about this case during the review since that was what I used in my workchain in https://github.com/aiidateam/aiida-core/issues/5477 and I was going to use _break to stop the loop. I think if the conditional predicate is None we don't know if it should be regared as true or false, it rather indicates a bug in the workflow.

But you are correct if this breaks some workflow at the moment it is not good and the warning should be raised. I agree with this change but the phrase "please return False instead." seems not correct?

[sphuber]

Thanks @sphuber, can you point out which workchain return None as the conditional predicate?

The workchain that triggered it for me is a private internal one. But I sketched out the relevant logic in the commit message and that does seem something that users could easily have used. Since Python does not enforce typing, I think it would be easy for users to return None by accident. Especially since this works "as expected".

I thought about this case during the review since that was what I used in my workchain in aiidateam/aiida-core#5477 and I was going to use _break to stop the loop. I think if the conditional predicate is None we don't know if it should be regared as true or false, it rather indicates a bug in the workflow.

I agree that we cannot know for sure, but I think as an aiida-core developer, you were thinking in a more "advanced" manner of wanting to explicitly affect the logic with some construct. I think most users wouldn't even have thought about this option of wanting to explicitly break out of the conditional. But there is no guarantee of course.

But you are correct if this breaks some workflow at the moment it is not good and the warning should be raised. I agree with this change but the phrase "please return False instead." seems not correct?

Why not? Before the change, None would be essentially treated as False. So if they were returning None by default (but the logic was working as they intended) then they would now have to change it to False to keep the behavior but get rid of the warning, would they not?

[unkcpz]

Why not? Before the change, None would be essentially treated as False. So if they were returning None by default (but the logic was working as they intended) then they would now have to change it to False to keep the behavior but get rid of the warning, would they not?

Yes, you are right, I didn't notice None is treated as False. It makes all sense to me now.