WorkChain: guarantee to maintain order of appended awaitables
#4156
Conversation
sphuber
added
the
pr/blocked
| # awaitable as a placeholder, in the `resolve_awaitable`, it can be found and replaced by the resolved value. | ||
| if awaitable.action == AwaitableAction.ASSIGN: | ||
| self.ctx[awaitable.key] = awaitable | ||
| elif awaitable.action == AwaitableAction.APPEND: |
There was a problem hiding this comment.
I have a question here, it seems the functionality assign_ can be replaced by the append_, is there any special user scenario for assign_?
There was a problem hiding this comment.
What is the attribute 'key' of awaitable ? I can't find where it is initialized.
There was a problem hiding this comment.
The key is set in construct_awaitables:
Basically it is the key that is used in
to_context or ToContext and so is the key that should be used to store the resolved value in the ctx
There was a problem hiding this comment.
I have a question here, it seems the functionality assign_ can be replaced by the append_, is there any special user scenario for
assign_?
Why do you say so? If you use assign, it will be assigned to that key in the ctx overriding any value that may have been there. If you use append it will create a list or simply append the value if the key already contained a list.
There was a problem hiding this comment.
yes, assign_ can overriding the values set to the context, but when do we need to reset the context, I can't image under what circumstances would it be necessary to override the value that been in ctx?
| @@ -62,6 +61,7 @@ def construct_awaitable(target): | |||
| 'action': AwaitableAction.ASSIGN, | |||
| 'target': awaitable_target, | |||
| 'outputs': False, | |||
| 'resolved': False, | |||
There was a problem hiding this comment.
Not fully sure why resolved used here, I guess it is used to prevent repeatedly resolve the awaitable, since there are two mechanism now. Could you add an entry in docstring for explanation?
There was a problem hiding this comment.
I based this PR on another branch that is used for PR #4154 . Since that one touched quite a bit of the same code, I didn't want to have to deal with the merge conflicts. So I just based this PR of off that branch. That is why there are 3 commits. Only the last commit is relevant to solve this issue. The other commits reinstate the second mechanism of checking awaitables, as you mentioned. That solves one problem, but added another problem with the awaitables that now, sometimes, it can be "actioned" twice, putting the same node in the context twice, which is unwanted. So I needed some kind of "guard" to make sure it is done only once. The resolved attribute is my solution. It has its own problems and possibilities for failure but was the best solution I could come up with for now.
Thanks for the review @unkcpz always useful to have more eyes on the code. Anyway this PR will have to wait to be merged before PR #4154 is merged |
sphuber
force-pushed
the
fix/2961/awaitable-append-order
branch
from
823b13c
to
2f0c0a6
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4156 +/- ##
===========================================
- Coverage 78.91% 78.89% -0.01%
===========================================
Files 467 467
Lines 34489 34500 +11
===========================================
+ Hits 27212 27215 +3
- Misses 7277 7285 +8
Continue to review full report at Codecov.
|
| if awaitable.action == AwaitableAction.ASSIGN: | ||
| self.ctx[awaitable.key] = awaitable | ||
| elif awaitable.action == AwaitableAction.APPEND: | ||
| self.ctx.setdefault(awaitable.key, []).append(awaitable) |
There was a problem hiding this comment.
Might be worth having a quick check in here just to make sure it hasn't been appended before. Just something like
assert awaitable.pk not in (await.pk for await in awaitables)
There was a problem hiding this comment.
Why? I don't see the use case, but if a user wants to add the same subprocess twice, why not let them? When asserting, we are mostly doing this for internal consistency, no? Here we would be basically checking whatever the user returned from a step, because the insert_awaitable is only called when to_context or ToContext is used with a child process
There was a problem hiding this comment.
Yup, agreed they might want to be able to do it but this PR doesn't support it. Unless I've misunderstood, when resolving it looks like you interate the placeholders container from start to end and so will find the frist matching entry which will be the corresponding node the second time around and just be overwritten leaving the second occurrence unresolved.
Agreed re: assert, this should really be some kind of timetime error, just didn't want to push an additional line of code change on you.
There was a problem hiding this comment.
ah damn you're right. Well that is just a straight up bug! When checking the placeholders I should also check that it actually is still an awaitable and not a node, in addition to checking the pk, right?
There was a problem hiding this comment.
Yeah, I was thinking of a type check as well...
Intuitively, one would expect the order of awaitables in the context once they have been resolved, to maintain the same order with which they had been inserted using the `append_` construct in the `to_context` call. However, this was not the case, as the resolved awaitables were just appended to the target container in the order that they resolved. To correct this behavior, once the awaitable is inserted, it is already inserted in the context, where its resolved value should be placed in the future. This allows the `resolve_awaitable` method to find the correct place and simply swap out the awaitable, that has been serving as a placeholder, with the resolved value.
sphuber
force-pushed
the
fix/2961/awaitable-append-order
branch
from
2f0c0a6
to
8f8f10c
Compare
Fixes #2961
Intuitively, one would expect the order of awaitables in the context
once they have been resolved, to maintain the same order with which they
had been inserted using the
append_construct in theto_contextcall. However, this was not the case, as the resolved awaitables were
just appended to the target container in the order that they resolved.
To correct this behavior, once the awaitable is inserted, it is already
inserted in the context, where its resolved value should be placed in
the future. This allows the
resolve_awaitablemethod to find thecorrect place and simply swap out the awaitable, that has been serving
as a placeholder, with the resolved value.