You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "hjtran (via GitHub)" <gi...@apache.org> on 2025/08/14 21:36:52 UTC
[PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
hjtran opened a new pull request, #35874:
URL: https://github.com/apache/beam/pull/35874
User ran into an error from accidentally passing a `[pcoll, DoOutputsTuple]` to `Flatten` and Flatten never throws an error so the downstream issue was very difficult to debug.
Added some validation to `Flatten`. New error looks like:
```
ValueError("Input to Flatten must be an iterable of PCollections. Item 1 of the iterable is type <class 'DoOutputsTuple'>")
```
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192252124
> [!WARNING]
> Gemini encountered an error creating the review. You can try again by commenting `/gemini review`.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #35874:
URL: https://github.com/apache/beam/pull/35874#discussion_r2279564351
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -3849,6 +3849,15 @@ def _extract_input_pvalues(self, pvalueish):
raise ValueError(
'Input to Flatten must be an iterable. '
'Got a value of type %s instead.' % type(pvalueish))
+
+ # Spot check to see if any of the items are iterables of PCollections
+ # and raise an error if so. This is always a user-error
+ for idx, item in enumerate(pvalueish):
+ if (isinstance(item, typing.Sequence) and len(item) and
+ isinstance(item[0], pvalue.PCollection)):
+ raise TypeError(
+ 'Inputs to Flatten cannot include an iterable of PCollections. '
+ f'(input at index {idx}: "{item}")')
Review Comment:
I think we should check the entire sequence. I doubt we have millions of them.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192035259
Assigning reviewers:
R: @claudevdm for label python.
Note: If you would like to opt out of this review, comment `assign to next reviewer`.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192254950
/gemini review
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192232889
This might break some internal Google tests.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "hjtran (via GitHub)" <gi...@apache.org>.
hjtran commented on code in PR #35874:
URL: https://github.com/apache/beam/pull/35874#discussion_r2279560154
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -3849,6 +3849,15 @@ def _extract_input_pvalues(self, pvalueish):
raise ValueError(
'Input to Flatten must be an iterable. '
'Got a value of type %s instead.' % type(pvalueish))
+
+ # Spot check to see if any of the items are iterables of PCollections
+ # and raise an error if so. This is always a user-error
+ for idx, item in enumerate(pvalueish):
+ if (isinstance(item, typing.Sequence) and len(item) and
+ isinstance(item[0], pvalue.PCollection)):
+ raise TypeError(
+ 'Inputs to Flatten cannot include an iterable of PCollections. '
+ f'(input at index {idx}: "{item}")')
Review Comment:
I intentionally only chose to sample the first element to save time in case there were any especially large sequences and I figure most actual scenarios where this happens it'll be a homogenous list.
@liferoad what do you think, is it worth checking the entire sequence?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192999596
cc @ahmedabu98
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "hjtran (via GitHub)" <gi...@apache.org>.
hjtran commented on code in PR #35874:
URL: https://github.com/apache/beam/pull/35874#discussion_r2279580801
##########
sdks/python/apache_beam/typehints/typecheck_test.py:
##########
@@ -179,6 +179,15 @@ def process(self, element, *args, **kwargs):
(self.p | beam.Create(['1', '1']) | beam.ParDo(ToInt()))
self.p.run().wait_until_finish()
+ def test_bad_flatten_input(self):
+ with self.assertRaisesRegex(
+ TypeError,
+ "Inputs to Flatten cannot include an iterable of PCollections. "):
+ with beam.Pipeline() as p:
+ pc = p | beam.Create([1, 1])
+ flatten_inputs = [pc, (pc, )]
+ flatten_inputs | beam.Flatten()
Review Comment:
I don't think this is necessary
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192243021
> > This might break some internal Google tests.
>
> We'd be surfacing real bugs though, right? If anyone specifies a pcollection in an iterable, the pcollection reference gets pickled which isn't what the user is intending I don't think
Yes, this is just a note for Googlers to be aware.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "hjtran (via GitHub)" <gi...@apache.org>.
hjtran commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192240061
> This might break some internal Google tests.
We'd be surfacing real bugs though, right? If anyone specifies a pcollection in an iterable, the pcollection reference gets pickled which isn't what the user is intending I don't think
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "hjtran (via GitHub)" <gi...@apache.org>.
hjtran commented on code in PR #35874:
URL: https://github.com/apache/beam/pull/35874#discussion_r2279569126
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -3849,6 +3849,15 @@ def _extract_input_pvalues(self, pvalueish):
raise ValueError(
'Input to Flatten must be an iterable. '
'Got a value of type %s instead.' % type(pvalueish))
+
+ # Spot check to see if any of the items are iterables of PCollections
+ # and raise an error if so. This is always a user-error
+ for idx, item in enumerate(pvalueish):
+ if (isinstance(item, typing.Sequence) and len(item) and
+ isinstance(item[0], pvalue.PCollection)):
+ raise TypeError(
+ 'Inputs to Flatten cannot include an iterable of PCollections. '
+ f'(input at index {idx}: "{item}")')
Review Comment:
SGTM
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad merged PR #35874:
URL: https://github.com/apache/beam/pull/35874
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on code in PR #35874:
URL: https://github.com/apache/beam/pull/35874#discussion_r2279550491
##########
sdks/python/apache_beam/typehints/typecheck_test.py:
##########
@@ -179,6 +179,15 @@ def process(self, element, *args, **kwargs):
(self.p | beam.Create(['1', '1']) | beam.ParDo(ToInt()))
self.p.run().wait_until_finish()
+ def test_bad_flatten_input(self):
+ with self.assertRaisesRegex(
+ TypeError,
+ "Inputs to Flatten cannot include an iterable of PCollections. "):
+ with beam.Pipeline() as p:
+ pc = p | beam.Create([1, 1])
+ flatten_inputs = [pc, (pc, )]
+ flatten_inputs | beam.Flatten()
Review Comment:
This is a good test case. To make the validation more comprehensive, consider adding another test case for an iterable that contains a mix of types, including a `PCollection`, but where the `PCollection` is not the first element. The current implementation would miss this, but an improved validation logic (as suggested in my other comment) would catch it.
For example:
```python
def test_bad_flatten_input_mixed_iterable(self):
with self.assertRaisesRegex(
TypeError,
"Inputs to Flatten cannot include an iterable of PCollections. "):
with beam.Pipeline() as p:
pc = p | beam.Create([1, 1])
flatten_inputs = [pc, (1, pc)]
flatten_inputs | beam.Flatten()
```
This would ensure the validation is robust against more subtle user errors.
##########
sdks/python/apache_beam/transforms/core.py:
##########
@@ -3849,6 +3849,15 @@ def _extract_input_pvalues(self, pvalueish):
raise ValueError(
'Input to Flatten must be an iterable. '
'Got a value of type %s instead.' % type(pvalueish))
+
+ # Spot check to see if any of the items are iterables of PCollections
+ # and raise an error if so. This is always a user-error
+ for idx, item in enumerate(pvalueish):
+ if (isinstance(item, typing.Sequence) and len(item) and
+ isinstance(item[0], pvalue.PCollection)):
+ raise TypeError(
+ 'Inputs to Flatten cannot include an iterable of PCollections. '
+ f'(input at index {idx}: "{item}")')
Review Comment:
The current check for nested PCollections is a good start, but it's not fully robust. It only checks if the first element of a sequence is a `PCollection`, which can miss cases like `[1, pcoll]`. Also, `typing.Sequence` is a broad ABC that can include types like `str`, which is probably not intended.
To make this validation more robust and specific, I suggest checking for `list` or `tuple` instances and then verifying if *any* of their elements are `PCollection`s. This will correctly handle more edge cases.
```suggestion
if isinstance(item, (list, tuple)) and any(
isinstance(sub_item, pvalue.PCollection) for sub_item in item):
raise TypeError(
'Inputs to Flatten cannot include an iterable of PCollections. '
f'(input at index {idx}: "{item}")')
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add type checking to confirm that the flatten inputs are actually pco… [beam]
Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on PR #35874:
URL: https://github.com/apache/beam/pull/35874#issuecomment-3192232476
/gemini review
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org