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 2020/04/20 11:17:25 UTC
[GitHub] [beam] mwalenia opened a new pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
mwalenia opened a new pull request #11464:
URL: https://github.com/apache/beam/pull/11464
After the remark in #11261 I modified the code to be in line with the PTransform creation guide.
Thanks, @jkff for your keen eye!
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] 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).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
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.
----------------------------------------------------------------
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] mwalenia commented on pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on pull request #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-619999316
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mwalenia commented on issue #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on issue #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-616484959
R: @jkff
CC: @lukecwik
----------------------------------------------------------------
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] mwalenia commented on pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on pull request #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-620451009
@ibzib @jkff Can we merge this?
----------------------------------------------------------------
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] ibzib commented on issue #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
ibzib commented on issue #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-617905163
@mwalenia Please resolve merge conflicts.
----------------------------------------------------------------
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] mwalenia commented on pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on pull request #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-620411630
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mwalenia commented on pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on pull request #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-619808362
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mwalenia commented on a change in pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on a change in pull request #11464:
URL: https://github.com/apache/beam/pull/11464#discussion_r412151907
##########
File path: sdks/java/extensions/ml/src/main/java/org/apache/beam/sdk/extensions/ml/AnnotateVideoFn.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.ml;
+
+import com.google.api.gax.longrunning.OperationFuture;
+import com.google.cloud.videointelligence.v1.AnnotateVideoProgress;
+import com.google.cloud.videointelligence.v1.AnnotateVideoRequest;
+import com.google.cloud.videointelligence.v1.AnnotateVideoResponse;
+import com.google.cloud.videointelligence.v1.Feature;
+import com.google.cloud.videointelligence.v1.VideoAnnotationResults;
+import com.google.cloud.videointelligence.v1.VideoContext;
+import com.google.cloud.videointelligence.v1.VideoIntelligenceServiceClient;
+import com.google.protobuf.ByteString;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import org.apache.beam.sdk.annotations.Experimental;
+import org.apache.beam.sdk.transforms.DoFn;
+import org.apache.beam.sdk.values.PCollectionView;
+
+/**
+ * Base class for DoFns used in VideoIntelligence transforms.
+ *
+ * @param <T> Class of input data being passed in - either ByteString - video data encoded into.
+ * String or String - a GCS URI of the video to be annotated.
+ */
+@Experimental
+abstract class AnnotateVideoFn<T> extends DoFn<T, List<VideoAnnotationResults>> {
+
+ protected final PCollectionView<Map<T, VideoContext>> contextSideInput;
+ protected final List<Feature> featureList;
+ VideoIntelligenceServiceClient videoIntelligenceServiceClient;
+
+ public AnnotateVideoFn(
+ PCollectionView<Map<T, VideoContext>> contextSideInput, List<Feature> featureList) {
+ this.contextSideInput = contextSideInput;
+ this.featureList = featureList;
+ }
+
+ public AnnotateVideoFn(List<Feature> featureList) {
+ contextSideInput = null;
+ this.featureList = featureList;
+ }
+
+ @StartBundle
Review comment:
Moved, thanks for the tip
----------------------------------------------------------------
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] jkff commented on a change in pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
jkff commented on a change in pull request #11464:
URL: https://github.com/apache/beam/pull/11464#discussion_r411789295
##########
File path: sdks/java/extensions/ml/src/main/java/org/apache/beam/sdk/extensions/ml/AnnotateVideo.java
##########
@@ -17,87 +17,44 @@
*/
package org.apache.beam.sdk.extensions.ml;
-import com.google.api.gax.longrunning.OperationFuture;
-import com.google.cloud.videointelligence.v1.AnnotateVideoProgress;
-import com.google.cloud.videointelligence.v1.AnnotateVideoRequest;
-import com.google.cloud.videointelligence.v1.AnnotateVideoResponse;
import com.google.cloud.videointelligence.v1.Feature;
import com.google.cloud.videointelligence.v1.VideoAnnotationResults;
import com.google.cloud.videointelligence.v1.VideoContext;
-import com.google.cloud.videointelligence.v1.VideoIntelligenceServiceClient;
-import com.google.protobuf.ByteString;
-import java.io.IOException;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.ExecutionException;
-import org.apache.beam.sdk.transforms.DoFn;
+import org.apache.beam.sdk.annotations.Experimental;
+import org.apache.beam.sdk.transforms.PTransform;
+import org.apache.beam.sdk.values.PCollection;
import org.apache.beam.sdk.values.PCollectionView;
/**
- * Base class for Video Intelligence transform.
+ * Base class for VideoIntelligence PTransform.
Review comment:
* PTransforms should almost never need inheritance; based on the usage of this class, it's not needed here either - the current base class serves only the purpose of storing configuration variables, but contains no functionality - these variables can just be packaged into a configuration object, and current subclasses of AnnotateVideo can derive from PTransform directly and each store this configuration object.
* Even if you decide to keep using inheritance, please at least make this class non-public.
##########
File path: sdks/java/extensions/ml/src/main/java/org/apache/beam/sdk/extensions/ml/AnnotateVideoFn.java
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.ml;
+
+import com.google.api.gax.longrunning.OperationFuture;
+import com.google.cloud.videointelligence.v1.AnnotateVideoProgress;
+import com.google.cloud.videointelligence.v1.AnnotateVideoRequest;
+import com.google.cloud.videointelligence.v1.AnnotateVideoResponse;
+import com.google.cloud.videointelligence.v1.Feature;
+import com.google.cloud.videointelligence.v1.VideoAnnotationResults;
+import com.google.cloud.videointelligence.v1.VideoContext;
+import com.google.cloud.videointelligence.v1.VideoIntelligenceServiceClient;
+import com.google.protobuf.ByteString;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ExecutionException;
+import org.apache.beam.sdk.annotations.Experimental;
+import org.apache.beam.sdk.transforms.DoFn;
+import org.apache.beam.sdk.values.PCollectionView;
+
+/**
+ * Base class for DoFns used in VideoIntelligence transforms.
+ *
+ * @param <T> Class of input data being passed in - either ByteString - video data encoded into.
+ * String or String - a GCS URI of the video to be annotated.
+ */
+@Experimental
+abstract class AnnotateVideoFn<T> extends DoFn<T, List<VideoAnnotationResults>> {
+
+ protected final PCollectionView<Map<T, VideoContext>> contextSideInput;
+ protected final List<Feature> featureList;
+ VideoIntelligenceServiceClient videoIntelligenceServiceClient;
+
+ public AnnotateVideoFn(
+ PCollectionView<Map<T, VideoContext>> contextSideInput, List<Feature> featureList) {
+ this.contextSideInput = contextSideInput;
+ this.featureList = featureList;
+ }
+
+ public AnnotateVideoFn(List<Feature> featureList) {
+ contextSideInput = null;
+ this.featureList = featureList;
+ }
+
+ @StartBundle
Review comment:
Should this be in `@Setup` instead?
----------------------------------------------------------------
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] jkff commented on pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
jkff commented on pull request #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-620718600
Yes, feel free to merge.
----------------------------------------------------------------
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] mwalenia commented on a change in pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on a change in pull request #11464:
URL: https://github.com/apache/beam/pull/11464#discussion_r412152083
##########
File path: sdks/java/extensions/ml/src/main/java/org/apache/beam/sdk/extensions/ml/AnnotateVideo.java
##########
@@ -17,87 +17,44 @@
*/
package org.apache.beam.sdk.extensions.ml;
-import com.google.api.gax.longrunning.OperationFuture;
-import com.google.cloud.videointelligence.v1.AnnotateVideoProgress;
-import com.google.cloud.videointelligence.v1.AnnotateVideoRequest;
-import com.google.cloud.videointelligence.v1.AnnotateVideoResponse;
import com.google.cloud.videointelligence.v1.Feature;
import com.google.cloud.videointelligence.v1.VideoAnnotationResults;
import com.google.cloud.videointelligence.v1.VideoContext;
-import com.google.cloud.videointelligence.v1.VideoIntelligenceServiceClient;
-import com.google.protobuf.ByteString;
-import java.io.IOException;
import java.util.List;
import java.util.Map;
-import java.util.concurrent.ExecutionException;
-import org.apache.beam.sdk.transforms.DoFn;
+import org.apache.beam.sdk.annotations.Experimental;
+import org.apache.beam.sdk.transforms.PTransform;
+import org.apache.beam.sdk.values.PCollection;
import org.apache.beam.sdk.values.PCollectionView;
/**
- * Base class for Video Intelligence transform.
+ * Base class for VideoIntelligence PTransform.
Review comment:
You're right that it wasn't necessary. I got rid of the inheritance.
----------------------------------------------------------------
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] mwalenia commented on issue #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on issue #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-616531011
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mwalenia commented on issue #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on issue #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-616523414
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] mwalenia commented on pull request #11464: [BEAM-9147] Make VideoIntelligence use PTransform on user-facing API
Posted by GitBox <gi...@apache.org>.
mwalenia commented on pull request #11464:
URL: https://github.com/apache/beam/pull/11464#issuecomment-619889279
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org