Add note about pushing the lazy XCom proxy to XCom #27250
Conversation
| def forward_values(values): | ||
| return values # This is a lazy proxy and can't be pushed! | ||
|
|
||
| You need to explicitly call ``list(values)`` instead, and accept the performance implications. |
There was a problem hiding this comment.
This invites the question of what implications and how can you tell if it's dramatic or not.
If this is something we discourage under any circumstances I would use a stronger language
There was a problem hiding this comment.
It entirely depends on what the user pushes into the XCom and how the data is stored so there’s no way to really describe fully. All the normal XCom caveats apply, times how many upstream tasks were mapped into.
|
This looks good - but if the #27251 is solid, I have strong preference for that one. Users don't read all the details in the documentation and they will expect it to "just work" . They don't even realise (and they should not) there is a lazy-proxy used. The whole taskflow API is there precisely to hide the implementation details (Xcom.push, Xcom.pull and the like) from the user, so this is entirely expected they have no idea about this detail. And since not many users read documentation, they will open an issue instead most likely (which we want to at least try to avoid). Another option (if we cannot make #27251 solid) will be to make a very explict error message when they try to push LazyXCom and point them to this documentation directly - then they will have a chance to read it and even if they copy&paste the error message in the issue, we will just close the issue and tell them to follow the advice they got. |
|
One possibility would be to add an additiona check in |
|
I commented in #27251 -> I think automated coertion and smart warning in case of performance problem might be way better approach - both good for the users and maintainers. #27251 (comment) |
|
Either way we take, documentation on this is needed, so I’m merging this first and continuing work in #27251. |
See discussion in #27209 for context. This simply adds a note about the situation. An alternative approach is explored in #27251 that tries to "hide" the issue magically instead.