Better errors when inputs are omitted. #28289
Conversation
It's not always possible to know if a transform consumes inputs, or can act as a root transform (and in fact some may be able to do both depending on their configuration), but when a transform expecting inputs doesn't get them the error can be quite obscure. This adds best-effort checking and a better error in that case. We also allow explicitly setting empty imputs to work around this error (which is where most of the complexity of this change lies). Importantly, sources (no matter their name) are not required to have inputs.
|
R: @byronellis |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Codecov Report
@@ Coverage Diff @@
## master #28289 +/- ##
==========================================
- Coverage 72.26% 72.25% -0.02%
==========================================
Files 684 684
Lines 100842 100937 +95
==========================================
+ Hits 72878 72931 +53
- Misses 26384 26426 +42
Partials 1580 1580
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| This is best-effort, primarily for better and earlier error messages. | ||
| """ | ||
| return not typ.startswith('Read') |
There was a problem hiding this comment.
Presuming this is to provide a reasonable default? Seems like it might lead to surprising false positives though?
There was a problem hiding this comment.
Yep, just a reasonable default. Most transforms that do not start with Read require an input by, the main exception being SQL (where we can't tell and the "typical" use of selecting on PCollection would be good to error on). One can explicitly state inputs: [] to suppress this error.
|
|
||
| def requires_inputs(self, typ, args): | ||
| if self._urns[type] in self.schema_transforms(): | ||
| return bool(self.schema_transforms()[self._urns[type]].inputs) |
There was a problem hiding this comment.
Unfortunately relying on schema transform's inputs/outputs is not very reliable (and maybe completely unreliable). They do not depend on the configuration passed to the transforms making them basically useless for determining anything other than inputs/outputs that are present in all possible configurations.
There was a problem hiding this comment.
Yeah, it'd be nice to be able to query about the requirements of a configured transform before invoking it, but that's a larger change... Most (though not all) transforms have fixed inputs at least, so this'll cover those.
It's not always possible to know if a transform consumes inputs, or can act as a root transform (and in fact some may be able to do both depending on their configuration), but when a transform expecting inputs doesn't get them the error can be quite obscure. This adds best-effort checking and a better error in that case.
We also allow explicitly setting empty imputs to work around this error (which is where most of the complexity of this change lies). Importantly, sources (no matter their name) are not required to have inputs.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.