Skip to content
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.

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

[BEAM-9340] Populate requirements for Python DoFn properties. #10909

Merged
merged 2 commits into from Feb 24, 2020

Conversation

robertwb
Copy link
Contributor


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
--- Build Status
Build Status
Build Status
Build Status
--- --- Build Status
XLang --- --- --- Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status
Build Status
Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

Copy link
Contributor

@je-ik je-ik left a comment

Choose a reason for hiding this comment

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

+1 Overall.
I have a minor concern, that I don't know how to solve exactly. As mentioned on ML, the set of requirements might not be static, but might depend on characteristics of input (e.g. if inputs are all bounded or not). Solution could be:
a) make the requirements a tuple (e.g. (bounded requirements, unbounded requirements), or
b) supply a logic that traverses the pipeline and creates requirements (that is, do not store requirements, but provide a logic) - I'm not sure how to implement this, or if it is even possible, or
c) ignore this nuance, because one can always declare the requirements as union of all cases, that would "only" make the pipeline fail although it could have run fine.

If we would be able (with reasonable certainty) to say that there will be no other conditions than if input is bounded or not, than I would tend to solution a). But I'm not sure about that. In the other case I would prefer solution c) (which is the current solution).

@robertwb
Copy link
Contributor Author

Thanks for the quick review.

Responded about (a) on the list for more visibility. I think (c) is the way to go, runners can become more sophisticated about conditions in which they can satisfy requirements and SDKs can become more sophisticated about conditions in which they require them based on the pipeline context.

(BTW, I did think about (b) but wanted to try to keep the requirements as local as possible, and make it possible to declare requirements that are not explicit in the pipeline proto itself.)

@robertwb
Copy link
Contributor Author

Rebased to deal with merge conflicts. Merging now.

@robertwb robertwb merged commit 58656c6 into apache:master Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants