You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/06/25 07:08:17 UTC
[GitHub] [beam] je-ik opened a new pull request #15082: [BEAM-12538] Allow PortablePipelineOptions to be specified on command line
je-ik opened a new pull request #15082:
URL: https://github.com/apache/beam/pull/15082
Add option to specify PortablePipelineOptions to be able to define default environment for the expansion.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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](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://beam.apache.org/contribute/#make-reviewers-job-easier).
`ValidatesRunner` compliance status (on master branch)
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/badge/icon?subject=V1+Streaming">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon?subject=V1+Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/badge/icon?subject=V2+Streaming">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon?subject=Java+8">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon?subject=Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon?subject=Portable+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/badge/icon?subject=Portable">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon?subject=Structured+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon?subject=ValCont">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
</tbody>
</table>
Examples testing status on various runners
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/badge/icon?subject=V1+Java11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
Post-Commit SDK/Transform Integration Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>Go</th>
<th>Java</th>
<th>Python</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon?subject=3.6">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon?subject=3.7">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon?subject=3.8">
</a>
</td>
</tr>
</tbody>
</table>
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>---</th>
<th>Java</th>
<th>Python</th>
<th>Go</th>
<th>Website</th>
<th>Whitespace</th>
<th>Typescript</th>
</tr>
</thead>
<tbody>
<tr>
<td>Non-portable</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon?subject=Tests">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon?subject=Lint">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon?subject=Docker">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon?subject=Docs">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Portable</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
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)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r667227991
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -462,6 +488,9 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
throw new RuntimeException(exn);
}
}));
+
+ SplittableParDo.convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary(pipeline);
Review comment:
So IMO the default path should be SDF.
All portable runners are expected to support SDF.
Multi-language only works for portable runners.
Flink supporting non-SDF sources is a special case. But if we need to support that it should be behind the experiment "use_deprecated_read". Default path (SDF) should not require an experiment .
--
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
[GitHub] [beam] chamikaramj commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-873143714
Run XVR_Direct PostCommit
--
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
[GitHub] [beam] chamikaramj commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662117312
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
How about just adding both "beam-sdks-java-io-expansion-service" and beam-runners-direct-java" to the classpath when starting up the expansion service ? No need to update dependencies of Beam. Following seems to work for me.
export CLASSPATH=beam-sdks-java-io-expansion-service-2.32.0-SNAPSHOT.jar:beam-runners-direct-java-2.32.0-SNAPSHOT.jar
java org.apache.beam.sdk.expansion.service.ExpansionService 12345
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872511163
> Why would annotations be safer?
That might be only opinion-based. But creating new `ExternalServiceOptions` means you have two places to add an option when you want it to be respected by both runners and external services - you will not add an option to ExternalServiceOptions _only_. That is only a projection, not a complete domain. That is why I think, that selection is better done with annotations.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r667242321
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -462,6 +488,9 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
throw new RuntimeException(exn);
}
}));
+
+ SplittableParDo.convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary(pipeline);
Review comment:
Sure, agree. The default is SDF. Everything else is triggered by `use_deprecated_read` or similar.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662123112
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
Hmm, maybe just deleting the `@Validation.Required` from `runner` in `PipelineOptions` might be enough? What is the reasoning for having both `@Validation.Required` and `@Default.InstanceFactory` on the same field? That means that the field has default value - though will be present - and if it is left to the default, it will throw the exception as soon as the Pipeline will try to use it, right?
--
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
[GitHub] [beam] chamikaramj commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r664797025
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -462,6 +488,9 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
throw new RuntimeException(exn);
}
}));
+
+ SplittableParDo.convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary(pipeline);
Review comment:
Please move this to "else" block for "use_deprecated_read" above.
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -524,4 +554,15 @@ public static void main(String[] args) throws Exception {
server.start();
server.awaitTermination();
}
+
+ private static class NoOpRunner extends PipelineRunner<PipelineResult> {
+ public static NoOpRunner fromOptions(PipelineOptions opts) {
+ return new NoOpRunner();
+ }
+
Review comment:
I mean some basic tests to make sure that a Pipeline can be created and expanded with a "NoOpRunner" (not to check for any functionality of "NoOpRunner").
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -362,6 +368,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
}
private @MonotonicNonNull Map<String, TransformProvider> registeredTransforms;
+ private final PipelineOptions pipelineOptions;
+
+ public ExpansionService() {
+ this(new String[] {});
+ }
+
+ public ExpansionService(String[] args) {
+ this(PipelineOptionsFactory.fromArgs(args).create());
+ }
+
+ public ExpansionService(PipelineOptions opts) {
Review comment:
Till we have proper validation in place, can we just explicitly create a second PipelineOptions with only the set of options that we wish to support ? (to make sure that arbitrary options that may or may not be supported do not get passed to the external transform expansion).
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
It's redundant if DirectRunner works for expansion, right ? I would also add a comment regarding why this was needed and a TODO with JIRA to remove this when DirectRunner does not conflict with Flink.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872476861
> We should make a new PipelineOptions subclass `ExternalServiceOptions` so it is clear which options are actually respected.
I think that the safer way would be the annotations. We clearly have two option spaces - runner dependent and runner independent (but majority of runner independent are needed by the runner as well). In the classical runners, there were only runner dependent options (which is why runner is `@Validation.Required`), but cross-language changed that, I'd say.
--
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
[GitHub] [beam] je-ik edited a comment on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik edited a comment on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872511163
> Why would annotations be safer?
That might be only opinion-based. But creating new `ExternalServiceOptions` means you have two places to add an option when you want it to be respected by both runners and external services - you will not add an option to ExternalServiceOptions _only_. That is only a projection, not a complete domain. That is why I think, that selection is better done with annotations.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662101299
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
Exactly `PipelineOptions.java:301`
--
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
[GitHub] [beam] chamikaramj commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662098491
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
How or where does this fail if you just do "java -jar beam-sdks-java-io-expansion-service-<version>-SNAPSHOT.jar <port> --defaultEnvironmentType=PROCESS" without specifying a Runner here ?
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r667506931
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +515,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ PipelineOptions effectiveOpts = PipelineOptionsFactory.create();
Review comment:
:+1:
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -462,6 +488,9 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
throw new RuntimeException(exn);
}
}));
+
+ SplittableParDo.convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary(pipeline);
Review comment:
Oops. My bad. I thought the `if` statement was (!use_sdf_read && (use_deprecated_read || beam_fn_api_use_deprecated_read)) - which actually would make more sense to me, because that is consistent with the fact, that we made SDF the default. This way I think the `beam_fn_api_use_deprecated_read` will not work, but I actually don't know where it is set. Will move the call to the inner block.
##########
File path: sdks/java/expansion-service/src/test/java/org/apache/beam/sdk/expansion/service/ExpansionServerTest.java
##########
@@ -45,4 +47,20 @@ public void testHostPortAvailableAfterClose() throws Exception {
assertThat(expansionServer.getHost(), is("localhost"));
assertThat(expansionServer.getPort(), greaterThan(0));
}
+
+ @Test
Review comment:
Hm, I thought there was one. :)
If there is no such test, I think it would be good to have one. I'll create a Jira for that.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662120548
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
The problem is that `:sdks:java:expansion-service` would (in fact) depend on `:runners:direct-java`. It would require this at least for `testRuntimeOnly`, but actually all uses of this module would need to add that (or any other runner and specify --runner, but that will get ignored). I think that the biggest problem is that runner is marked `@Validation.Required`, athough it is not required (apparently, expansion service works without runner). That is the root issue and everything else seems like a (smaller or bigger) hack. Implementing a `NoOpRunner` seems like the smallest issue, as that does not leak or break users anyhow.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-873399757
Run XVR_Spark PostCommit
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-874530848
Run Java PreCommit
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r665530782
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
Hm, it depends on what you mean by 'works'. Yes, it will expand, but the expansion service really is not supposed to call the `run` method. That would definitely result in wrong behavior, even with DirectRunner.
--
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
[GitHub] [beam] ibzib commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872581895
> > I don't see how this would be different with annotations. We don't need to move any options, just capture a subset. (To be clear, I was imagining something like `ExpansionServiceOptions extends PortableEnvironmentOptions, ExperimentalOptions`.)
>
> Yes, inheritance works the same, if we can easily do that, then it would be OK in this sense. Still, it would require that all the options (`PortableEnvironmentOptions`) be respected by `ExpansionServiceOptions`, which is not the case.
That's why I propose separating out PortableEnvironmentOptions from PortablePipelineOptions. It'd be like this:
PortableEnvironmentOptions
PortablePipelineOptions extends PortableEnvironmentOptions, ...
ExpansionServiceOptions extends PortableEnvironmentOptions, ExperimentalOptions
Ideally though we wouldn't need ExperimentalOptions, as we're discussing on the mailing list.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PortablePipelineOptions to be specified on command line
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-868276716
R: @TheNeuralBit @robertwb
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r667497140
##########
File path: sdks/java/expansion-service/src/test/java/org/apache/beam/sdk/expansion/service/ExpansionServerTest.java
##########
@@ -45,4 +47,20 @@ public void testHostPortAvailableAfterClose() throws Exception {
assertThat(expansionServer.getHost(), is("localhost"));
assertThat(expansionServer.getPort(), greaterThan(0));
}
+
+ @Test
Review comment:
Do you also need an end-to-end integration test to make sure that "PROCESS" mode works for cross-language transforms on Flink ? (can be a separate PR).
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -462,6 +488,9 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
throw new RuntimeException(exn);
}
}));
+
+ SplittableParDo.convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary(pipeline);
Review comment:
But this call "SplittableParDo.convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary" being in the default path, it looks like we will convert SDFs to "PrimitiveBoundedRead" if "use_sdf_read" is not set. Hence my request to move this to the else block above.
https://github.com/apache/beam/blob/403ad51d595677d580c7a5cfd9ba9aea6f5793c2/runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/SplittableParDo.java#L655
(I understand that we set "use_sdf_read" above for now but removal of that in the future will make default path break for SDF which is required by all runners other than Flink)
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +515,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ PipelineOptions effectiveOpts = PipelineOptionsFactory.create();
Review comment:
I think this is OK for now but let's add a TODO and a Jira to implement proper validation.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-878038902
Run Java PreCommit
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662120548
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
The problem is that `:sdks:java:expansion-service` would (in fact) depend on :runners:direct-java`. It would require this at least for `testRuntimeOnly`, but actually all uses of this module would need to add that (or any other runner and specify --runner, but that will get ignored). I think that the biggest problem is that runner is marked `@Validation.Required`, athough it is not required (apparently, expansion service works without runner). That is the root issue and everything else seems like a (smaller or bigger) hack. Implementing a `NoOpRunner` seems like the smallest issue, as that does not leak or break users anyhow.
--
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
[GitHub] [beam] ibzib commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872516504
> > Why would annotations be safer?
>
> That might be only opinion-based. But creating new `ExternalServiceOptions` means you have two places to add an option when you want it to be respected by both runners and external services - you will not add an option to ExternalServiceOptions _only_. That is only a projection, not a complete domain. That is why I think, that selection is better done with annotations.
Creating ExternalServiceOptions doesn't mean we need to define new options. For example, we could refactor environment-related options into a new PortableEnvironmentOptions class, and then make both PortablePipelineOptions and ExternalServiceOptions extend PortableEnvironmentOptions.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872519819
> Creating ExternalServiceOptions doesn't mean we need to define new options. For example, we could refactor environment-related options into a new PortableEnvironmentOptions class, and then make both PortablePipelineOptions and ExternalServiceOptions extend PortableEnvironmentOptions.
That makes sense, but that applies to current state only, right? When a new option is added, would the author of the change know exactly where to put the option? The ExternalServiceOptions 'look' like being applied to external service only. That is not the case. We would have to move complete ExperimentalOptions to ExternalServiceOptions, which seems problematic.
--
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
[GitHub] [beam] chamikaramj commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-873143843
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662522137
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -391,17 +410,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
request.getTransform().getSpec().getUrn());
LOG.debug("Full transform: {}", request.getTransform());
Set<String> existingTransformIds = request.getComponents().getTransformsMap().keySet();
- Pipeline pipeline = Pipeline.create();
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "beam_fn_api");
- // TODO(BEAM-10670): Remove this when we address performance issue.
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "use_sdf_read");
+ Pipeline pipeline = createPipeline();
+ if (!ExperimentalOptions.hasExperiment(pipelineOptions, "use_deprecated_read")) {
Review comment:
Not as part of this PR, but I believe there are tests for that. We need it so that we don't override `use_deprecated_read` with `use_sdf_read` (opt-out, then opt-in).
--
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
[GitHub] [beam] je-ik edited a comment on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik edited a comment on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872519819
> Creating ExternalServiceOptions doesn't mean we need to define new options. For example, we could refactor environment-related options into a new PortableEnvironmentOptions class, and then make both PortablePipelineOptions and ExternalServiceOptions extend PortableEnvironmentOptions.
That makes sense, but that applies to current state only, right? When a new option is added, would the author of the change know exactly where to put the option? The ExternalServiceOptions 'look' like being applied to external service only. That is not the case. And we would have to move complete ExperimentalOptions to ExternalServiceOptions, which seems problematic.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-871664878
Run Java PreCommit
--
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
[GitHub] [beam] ibzib commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872531325
Oops -- I meant to say ExpansionServiceOptions.
> The ExternalServiceOptions 'look' like being applied to external service only. That is not the case.
I'm not sure what you mean here.
> And we would have to move complete ExperimentalOptions to ExternalServiceOptions, which seems problematic.
I don't see how this would be different with annotations. We don't need to move any options, just capture a subset. (To be clear, I was imagining something like `ExpansionServiceOptions extends PortableEnvironmentOptions, ExperimentalOptions`.)
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r663373860
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -362,6 +368,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
}
private @MonotonicNonNull Map<String, TransformProvider> registeredTransforms;
+ private final PipelineOptions pipelineOptions;
+
+ public ExpansionService() {
+ this(new String[] {});
+ }
+
+ public ExpansionService(String[] args) {
+ this(PipelineOptionsFactory.fromArgs(args).create());
+ }
+
+ public ExpansionService(PipelineOptions opts) {
Review comment:
I have a sketch, but I'm missing access to the annotations. https://github.com/je-ik/beam/commit/a78ed9b87270f131c5eee4bdcd6f9de772d6aef0
The AllowedScopes might be renamed (maybe SupportedScopes), but as I said, I'd prefer to push this to different PR to have the discussion.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-874530848
Run Java PreCommit
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-871669904
@robertwb will you help me review this, please?
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r663366088
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -391,17 +410,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
request.getTransform().getSpec().getUrn());
LOG.debug("Full transform: {}", request.getTransform());
Set<String> existingTransformIds = request.getComponents().getTransformsMap().keySet();
- Pipeline pipeline = Pipeline.create();
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "beam_fn_api");
- // TODO(BEAM-10670): Remove this when we address performance issue.
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "use_sdf_read");
+ Pipeline pipeline = createPipeline();
+ if (!ExperimentalOptions.hasExperiment(pipelineOptions, "use_deprecated_read")) {
Review comment:
:+1:
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -524,4 +554,15 @@ public static void main(String[] args) throws Exception {
server.start();
server.awaitTermination();
}
+
+ private static class NoOpRunner extends PipelineRunner<PipelineResult> {
Review comment:
:+1:
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -524,4 +554,15 @@ public static void main(String[] args) throws Exception {
server.start();
server.awaitTermination();
}
+
+ private static class NoOpRunner extends PipelineRunner<PipelineResult> {
+ public static NoOpRunner fromOptions(PipelineOptions opts) {
+ return new NoOpRunner();
+ }
+
Review comment:
The Pipeline in ExpansionService is not supposed to be run. First of all, the Pipeline specification is not finished at that time, so calling run() should result in some sort of error. UnsupportedOperationException seems to be like one of the best options in this sense. That is probably even better than using DirectRunner, which might in that case run somewhat partial Pipeline.
I generally like having tests for everything that makes sense to be tested, but in this case, I'm not sure what to test, because the `NoOpRunner` (we might come up with a better name, for instance `ExpansionRunner`?) is simply throwing exception and not doing enything else.
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -362,6 +368,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
}
private @MonotonicNonNull Map<String, TransformProvider> registeredTransforms;
+ private final PipelineOptions pipelineOptions;
+
+ public ExpansionService() {
+ this(new String[] {});
+ }
+
+ public ExpansionService(String[] args) {
+ this(PipelineOptionsFactory.fromArgs(args).create());
+ }
+
+ public ExpansionService(PipelineOptions opts) {
Review comment:
That is more complex than I expected. I actually don't know how to do that. The only "legal" option seems to be serializing the options to JSON and then parse them back. That is more complicated than I would expect. The current approach to validation is that it validates only requirements - and the validation is called in `as(Class)`. I could add a new validation type there, but it would require changing public API of PipelineOptionsFactory - probably the best would be to add annotation of allowed scopes to getter methods. The PipelineOptionsFactory.fromArgs(args) would have to be extended to accept the set of allowed scopes (default all scopes). This seems to be a non-trivial change. I'll try to do a sketch, but I think this would really deserve new JIRA and a new PR. We might try to get that to 2.32.0 as well.
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
Understand that. I think that the NoOpRunner (or if we rename it to ExpansionRunner) is actually better than DirectRunner, as mentioned in comment below.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r667308702
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +515,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ PipelineOptions effectiveOpts = PipelineOptionsFactory.create();
Review comment:
I created new PipelineOptions per expansion and copy the important options from the command-line. On the other hand I think, that from user perspective this has very low difference, because the main issue here is that the user (with good intentions) specifies an option that is not supported by the service (e.g. most obviously the `runner`). The command line will accept it, but it will be simply silently ignored. I think that is the case we need to address by the validation scope annotations.
--
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
[GitHub] [beam] chamikaramj commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r663144696
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -362,6 +368,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
}
private @MonotonicNonNull Map<String, TransformProvider> registeredTransforms;
+ private final PipelineOptions pipelineOptions;
+
+ public ExpansionService() {
+ this(new String[] {});
+ }
+
+ public ExpansionService(String[] args) {
+ this(PipelineOptionsFactory.fromArgs(args).create());
+ }
+
+ public ExpansionService(PipelineOptions opts) {
Review comment:
I think we should implement some sort of a validation to limit the supported options in the first version of the PR (this can be based on annotations as you mentioned in the mailing list).
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -391,17 +410,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
request.getTransform().getSpec().getUrn());
LOG.debug("Full transform: {}", request.getTransform());
Set<String> existingTransformIds = request.getComponents().getTransformsMap().keySet();
- Pipeline pipeline = Pipeline.create();
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "beam_fn_api");
- // TODO(BEAM-10670): Remove this when we address performance issue.
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "use_sdf_read");
+ Pipeline pipeline = createPipeline();
+ if (!ExperimentalOptions.hasExperiment(pipelineOptions, "use_deprecated_read")) {
Review comment:
As discussed in the mailing list, "use_deprecated_read" only works for Flink since it has special handling for non-SDF sources in the runner. Such sources cannot be read in the SDK Harness without SDF. We should add a warn/info log for this.
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -524,4 +554,15 @@ public static void main(String[] args) throws Exception {
server.start();
server.awaitTermination();
}
+
+ private static class NoOpRunner extends PipelineRunner<PipelineResult> {
Review comment:
I'm bit hesitant regarding the NoOpRunner since I'm not sure if it implements the runner contract correctly (for expansion part) but I'm OK with it dost not result in the failure of existing tests or use-cases.
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -524,4 +554,15 @@ public static void main(String[] args) throws Exception {
server.start();
server.awaitTermination();
}
+
+ private static class NoOpRunner extends PipelineRunner<PipelineResult> {
+ public static NoOpRunner fromOptions(PipelineOptions opts) {
+ return new NoOpRunner();
+ }
+
Review comment:
Please move NoOpRunner to a separate class and add basic unit tests (pipeline creation with and without options, expansion etc.).
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
I would not remove current validations (which have been around forever) without additional discussions in the mailing list.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r665537989
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -362,6 +368,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
}
private @MonotonicNonNull Map<String, TransformProvider> registeredTransforms;
+ private final PipelineOptions pipelineOptions;
+
+ public ExpansionService() {
+ this(new String[] {});
+ }
+
+ public ExpansionService(String[] args) {
+ this(PipelineOptionsFactory.fromArgs(args).create());
+ }
+
+ public ExpansionService(PipelineOptions opts) {
Review comment:
We can do that, but the question is which case exactly do we want to solve. My initial feeling was that we want to throw exception when user sets option that is not supported (that is, might not be respected by the ExpansionService). Extracting 'supported' features will not help this - unsupported options will still be silently ignored by the service, pretty much the same way as we do not extract it.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PortablePipelineOptions to be specified on command line
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r658525402
##########
File path: sdks/java/expansion-service/build.gradle
##########
@@ -38,6 +38,7 @@ dependencies {
compile project(path: ":sdks:java:core", configuration: "shadow")
compile project(path: ":runners:core-construction-java")
compile project(path: ":runners:java-fn-execution")
+ compile project(path: ":runners:direct-java")
Review comment:
This is unfortunate, but PipelineOptionsFactory does not permit not to have a runner.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662100479
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -486,6 +504,11 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
.build();
}
+ protected Pipeline createPipeline() {
+ pipelineOptions.setRunner(NoOpRunner.class);
Review comment:
Runner is `@Validation.Required`, so it fails in `PipelineOptionsValidator#validate`.
--
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
[GitHub] [beam] je-ik commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r665543945
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -462,6 +488,9 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
throw new RuntimeException(exn);
}
}));
+
+ SplittableParDo.convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary(pipeline);
Review comment:
The `convertReadBasedSplittableDoFnsToPrimitiveReadsIfNecessary` already contains this check. Actually, it has the 'canonical' implementation of it - it tests both `use_deprecated_read` and `beam_fn_api_use_deprecated_read` (and can be overridden by `use_sdf_read`). I'd have to reimplement this check or else I would change the semantics.
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -524,4 +554,15 @@ public static void main(String[] args) throws Exception {
server.start();
server.awaitTermination();
}
+
+ private static class NoOpRunner extends PipelineRunner<PipelineResult> {
+ public static NoOpRunner fromOptions(PipelineOptions opts) {
+ return new NoOpRunner();
+ }
+
Review comment:
I'd say, that the expansion process is tested in https://github.com/apache/beam/blob/efc9b7fd8d4f7cedf006911d86b4467f40812046/sdks/java/expansion-service/src/test/java/org/apache/beam/sdk/expansion/service/ExpansionServiceTest.java#L120
The main point is that the expansion cannot call `run` method, as that really does not make sense in this context. It would simplify things in the context of ExpansionService, if the runner can be `null`, but that seems to be the case only for ExpansionService, and I'd prefer to keep the validations as they are. Maybe we can rename NoOpRunner to ThrowingRunner? Or NotRunnableRunner?
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872547191
> I don't see how this would be different with annotations. We don't need to move any options, just capture a subset. (To be clear, I was imagining something like `ExpansionServiceOptions extends PortableEnvironmentOptions, ExperimentalOptions`.)
Yes, inheritance works the same, if we can easily do that, then it would be OK in this sense. Still, it would require that all the options (`PortableEnvironmentOptions`) be respected by `ExpansionServiceOptions`, which is not the case.
--
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
[GitHub] [beam] ibzib commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872486593
Why would annotations be safer?
--
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
[GitHub] [beam] chamikaramj commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r668177463
##########
File path: sdks/java/expansion-service/src/test/java/org/apache/beam/sdk/expansion/service/ExpansionServerTest.java
##########
@@ -45,4 +47,20 @@ public void testHostPortAvailableAfterClose() throws Exception {
assertThat(expansionServer.getHost(), is("localhost"));
assertThat(expansionServer.getPort(), greaterThan(0));
}
+
+ @Test
Review comment:
I think current tests run with Docker mode.
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-876993139
I renamed the `NoOpRunner` to `NotRunnableRunner` to make it more clear what it actually does.
--
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
[GitHub] [beam] chamikaramj merged pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
chamikaramj merged pull request #15082:
URL: https://github.com/apache/beam/pull/15082
--
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
[GitHub] [beam] je-ik commented on pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
je-ik commented on pull request #15082:
URL: https://github.com/apache/beam/pull/15082#issuecomment-872000087
R: @ibzib
--
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
[GitHub] [beam] ibzib commented on a change in pull request #15082: [BEAM-12538] Allow PipelineOptions to be specified on command line of ExpansionService
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #15082:
URL: https://github.com/apache/beam/pull/15082#discussion_r662504981
##########
File path: sdks/java/expansion-service/src/main/java/org/apache/beam/sdk/expansion/service/ExpansionService.java
##########
@@ -391,17 +410,19 @@ default InputT createInput(Pipeline p, Map<String, PCollection<?>> inputs) {
request.getTransform().getSpec().getUrn());
LOG.debug("Full transform: {}", request.getTransform());
Set<String> existingTransformIds = request.getComponents().getTransformsMap().keySet();
- Pipeline pipeline = Pipeline.create();
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "beam_fn_api");
- // TODO(BEAM-10670): Remove this when we address performance issue.
- ExperimentalOptions.addExperiment(
- pipeline.getOptions().as(ExperimentalOptions.class), "use_sdf_read");
+ Pipeline pipeline = createPipeline();
+ if (!ExperimentalOptions.hasExperiment(pipelineOptions, "use_deprecated_read")) {
Review comment:
Where are these options read? Do we test that these options are respected?
--
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