hjtran commented on PR #35224: URL: https://github.com/apache/beam/pull/35224#issuecomment-2960075065
> This change (introduced in #35160) unfortunately can break some pipelines which rely on the current behavior. Specifically, I found people running into issues with pipelines which looked like: > > ``` > class MyDofn(...): > expand(self, ...): > if self.condition: > return None > ... > > def construct_pipeline(): > foo = p | MyDofn(...) > if foo is not None: > foo | more_transforms(...) > ``` > > This probably should remain valid, I don't think its worth breaking existing pipelines for the correctness semantics. User's could update this pattern to `if not isinstance(foo, PDone):`, but I suppose we considered that and figured that this isn't worth burdening the users with doing? I can see how it might be a bit surprising that _only_ in the case of returning `None` does the result of `def expand` get cast into something else. To rephrase the goal a little bit, the change wasn't exactly to update correctness semantics, just to include a better error message for when user's make a specific kind of mistake. I think we can probably still achieve this while still returning `None`. Not sure if you have users that rely on unreleased Beam code though. If so, seems reasonable to ship this and I can try to come up with a solution given these new constraints (may also be worth adding a test to explicitly define this as expected behavior) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
