-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Coerce LazyXComAccess to list when pushed to XCom #27251
Coerce LazyXComAccess to list when pushed to XCom #27251
Conversation
5f453e0
to
e3ab730
Compare
Is it complete? Can we add some tests? |
This is functionally complete, but discussion is needed to decide if this is a good idea. I can add tests if this is considered a good idea. |
I personally think this is far better approach than #27250 - most users don't read the documentation and some of them will raise an issue if they find the problem (which is not maintainer-friendly). Implementing automated coertion is a good solution from that perspective (the best way to solve the problem is to avoid it altogether). Yes, it has some obvious performance consequences for the users, but then one thing that we could do is we could write a warning in the log that this automated coertion has potential performance problems (we can even be smart and only print it if coertion takes more than arbitrary number of seconds to not introduce too much noice. And of course documenting thos performance issues in the docs (then the log could have "See more URL_TO_THE_DOCS" pointing to the right part of documentation. For me this is the best "user-friendly" and "maintainer-friendly" approach we can come up with. |
Sounds reasonable to me. This means we will need to update both the documentation and the code, so I’ll merge the documentation PR and continue the rest here. |
The class is intended to work as a "lazy list" to avoid pulling a ton of XComs unnecessarily, but if it's pushed into XCom, the user should be aware of the performance implications, and this avoids leaking the implementation detail.
e3ab730
to
4338081
Compare
Alright, done. |
Looks cool! |
See discussion in #27209 for context.
The class is intended to work as a "lazy list" to avoid pulling a ton of XComs unnecessarily, but if it's pushed into XCom, the user should be aware of the performance implications, and this avoids leaking the implementation detail.
An alternative "solution" to the issue would be to accept this implementation detail should be exposed to the user (it already is), and simply document the caveat. This approach is explored in #27250.