Remove warning for dofns that don't return anything #35521
Conversation
|
Assigning reviewers: R: @damccorm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| if lstripped_line.startswith( | ||
| "return None") or lstripped_line.rstrip() == "return": | ||
| _LOGGER.warning(return_none_warning) | ||
| if has_yield and has_return: |
There was a problem hiding this comment.
I was a bit conservative with what I removed but I actually wonder if any of the warnings in this function are useful
|
R: @liferoad since this came out of #28061 I'm somewhat ambivalent about this one. I've seen issues where this warning has or would have helped. I've also seen people get confused by the wording. Instead of removing the warning, could we clarify the text so that it is obvious that you can ignore it if it is intentional? |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
It'd be nice if there was an easy way to discriminate between whether this case might be an accident or if it's actually intended. As it stands, I'm not sure there's an obvious way to silence this warning as a DoFn author who really doesn't want to yield anything (I suppose they can just yield and empty generator but I wouldn't call that obvious) |
I would argue the good practice is to return something. |
I think it might not be a very known good practice. I for one didnt realize it was good practice and have written a decent number of side effect transforms. These warnings also don't account for dofns that yield elements in I tried turning the warning into an error to see which dofns currently don't yield anything and here's a partial list:
I think the noise-to-signal ratio might be high enough that the warnings get ignored. |
|
I think the warning is still helpful but as suggested before, maybe we can rephrase the warning to be less alarming. WDYT? |
For me it's less the phrasing and more seeing any warning at all. I typically try to investigate most warnings and these can pop up sometimes even for transforms that I didn't write. If you think it's helpful though, I can just put in Thanks for the discussion! |
|
This warning message is spamming. As mentioned above, many internal DoFns falls into the category and will raise warnings. There are not actionable either. There is also a bug in the current warning message. At least let us skip warning for internal variables. |
|
It'd be really nice to just remove altogether, not just for internal transforms. Schrodinger transforms often raise this warning and I get a lot of questions about how to investigate and whether the user should be worried. I think in a vast majority of cases, the user intent of "I don't want this DoFn to generate any outputs" is expressed by returning |
|
Read the previous discussion there might be a case the warning is valid. Opened #36703 and explained there |
Now suppressed warning for the case of a bare |
4 participants
This is often a useful pattern but this warning makes it seem like a bug.
e.g. _PassThroughThenCleanup is an example where we have a dofn that doesn't return anything