Skip to content

Add empty side_inputs tuple to MaybeReshuffle PTransform #36184

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.

Sign up for GitHub

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

Closed
liferoad wants to merge 1 commit into from
Closed

Add empty side_inputs tuple to MaybeReshuffle PTransform #36184

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sdks/python/apache_beam/transforms/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4081,6 +4081,8 @@ def expand(self, pbegin):
# transforms (e.g. Write).

class MaybeReshuffle(PTransform):
side_inputs = ()
Copy link
Collaborator

@claudevdm claudevdm Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is because MaybeReshuffle is defined dynamically (inside Create.expand?) which is affecting the inheritince.

In general I think it is better to avoid dynamic types unless necessary. What do you think about moving MaybeReshuffle to the top level?

Copy link
Contributor Author

@liferoad liferoad Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this for now. Not sure changing this will cause any graph compatibility issue.

Copy link
Collaborator

@claudevdm claudevdm Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think any change in this file that adds extra lines breaks update compat unfortunately since this is a nested transform, without explicit labels. See #34807

Line number gets added to the automatically generated label

Copy link
Collaborator

@shunping shunping Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered I saw this kind of compat issue before. Do we also want to add a label for this change too?

Copy link
Contributor

@damccorm damccorm Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also want to add a label for this change too?

I think we should do this and deal with the one-time update compat change. I don't think we can be in a place where we can't update this (pretty important) file

We can document this in CHANGES.md and give people a clear update path (via transform_name_mapping)

Copy link
Contributor

@damccorm damccorm Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see - we can do even better by explicitly naming it like https://github.com/apache/beam/pull/34807/files


def expand(self, pcoll):
if len(serialized_values) > 1 and reshuffle:
from apache_beam.transforms.util import Reshuffle
Expand Down
Loading