[BEAM-9146] Integrate GCP Video Intelligence functionality for Python SDK

[EDjur]

This PR relates to https://issues.apache.org/jira/browse/BEAM-9146 and integrates GCP Video Intelligence functionality as part of a PTransform that implicitly accepts a PCollection of GCS URIs or raw bytes.

---

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

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

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

Post-Commit Tests Status (on master branch)

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

Pre-Commit Tests Status (on master branch)

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

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

[EDjur]

R: @aaltay @kamilwu

[aaltay]

Thank you @EDjur.

I was hoping that we can implement most of the functionality in tfx_bsl, and add cloud AI services by defining new endpoints. We had a discussion on dev list [1] related to this.

[1] https://lists.apache.org/thread.html/r3d9b0e3270668fda989f363eab1f79e4ef0e7fa3e0fcbe9c26344f14%40%3Cdev.beam.apache.org%3E

[EDjur]

I see. I signed up to the dev mailing list just last week so must've missed that discussion.

What does that mean in regards to this ticket?

I agree that it would be nice to avoid duplicated behaviour across libraries.

[aaltay]

@EDjur - Sorry for the miscommunication. These types of decisions should have reflected on JIRA as well. I feel bad that you spent time on this and we are changing direction after your PR is out.

I think what it means for this ticket is that:

• If tfx_bsl has ability to call into services (which we need to add and TFX team agreed to support us with reviews)
• We can add thin transforms for different things (e.g. AnnotateVideo) and each of these transform will have quite a bit of shared code and behavior.

@kamilwu may have other thoughts, since he was also working on this. I would like to hear what is his opinion.

For this PR, let's try to re-use as much as possible.

[EDjur]

@aaltay No worries - I've learnt a lot by creating this PR anyway! Curious to hear @kamilwu's thoughts on it too.

I'm open to modify / work on another PR related to tfx_bsl if needed.

[kamilwu]

Thanks @EDjur.

tfx_bsl seems to be useful only in some cases. One of these cases is AI Platform Prediction, which, by the way, we also would like to support in Beam in the nearest future.

Video Intelligence API is pretty much different. We should treat it as a fully-managed machine learning service. There is no machine learning model which could be trained and deployed. Instead, videos are annotated using pre-trained custom model provided by Google. All a user has to do is to enable API and make a request (using REST API or client library). The only input is a GCS path to a video file. As a result, there is little behavior that could be shared.

If you have any questions regarding tfx_bsl and how it connects with Beam, just let me know, as I'm currently working to provide support for remote inference (an ability to call into services) in tfx_bsl.

[kamilwu]

@EDjur Because you've added a new dependency, we have to specify it in setup.py file (sdks/python/setup.py)

[aaltay]

Thank you @kamilwu for the comment. That makes sense to me.

Related question, what is a good location for these family of transforms? io/... does not sounds right in this case. How about a top level ml/ folder similar to io. These transform could be put under ml/gcp?

[kamilwu]

Related question, what is a good location for these family of transforms? io/... does not sounds right in this case. How about a top level ml/ folder similar to io. These transform could be put under ml/gcp?

I agree io/... can be somewhat misleading. ml/gcp sounds good.

[EDjur]

Thanks for the feedback! I've adjusted the code to conform to py23 standards as well as added the extra arguments to annotate_video.

I also added the google-cloud-videointelligence dependency which I've tested from version 1.8.0 up until 1.12.1.

[EDjur]

Got one question regarding the naming. Is video_intelligence a good name for the module? I'm slightly worried it might cause confusion with the google.cloud.videointelligence.

[aaltay]

Got one question regarding the naming. Is video_intelligence a good name for the module? I'm slightly worried it might cause confusion with the google.cloud.videointelligence.

I think it is a good name reflects the underlying service as it is. Not very different from other gcp io with the similar names to the gcp services.

[aaltay]

LGTM. I will wait for @kamilwu to complete his review.

[kamilwu]

LGTM.

[EDjur]

Got one question regarding the naming. Is video_intelligence a good name for the module? I'm slightly worried it might cause confusion with the google.cloud.videointelligence.

I think it is a good name reflects the underlying service as it is. Not very different from other gcp io with the similar names to the gcp services.

I started work on https://issues.apache.org/jira/browse/BEAM-9247 to integrate the vision API in a similar PTransform. However, here the naming gets more complex. Consider e.g.

  from google.cloud import vision
  from apache_beam.ml.gcp import vision

One solution for the vision transforms is to rename the module vision_api instead. But I think we should be consistent across the different GCP ML APIs, so perhaps video_intelligence will need renaming too.

What's your take? @aaltay

[aaltay]

Got one question regarding the naming. Is video_intelligence a good name for the module? I'm slightly worried it might cause confusion with the google.cloud.videointelligence.

I think it is a good name reflects the underlying service as it is. Not very different from other gcp io with the similar names to the gcp services.

I started work on https://issues.apache.org/jira/browse/BEAM-9247 to integrate the vision API in a similar PTransform. However, here the naming gets more complex. Consider e.g.

  from google.cloud import vision
  from apache_beam.ml.gcp import vision

One solution for the vision transforms is to rename the module vision_api instead. But I think we should be consistent across the different GCP ML APIs, so perhaps video_intelligence will need renaming too.

What's your take? @aaltay

I do not have a good idea. Users could solve this with something like the following:

from google.cloud import vision as gcp_vision_api
from apache_beam.ml.gcp import vision

So, maybe it is not a big issue. I agree, it would be good if we can avoid this state.

Related, in the io folder we have modules named gcsio, bigtableio etc. but we also have modules named bigquery, pubsub. So it is not very consistent.

We can take the io example and change the names by appending ml, your example will look like:

from google.cloud import vision
from apache_beam.ml.gcp import visionml
from apache_beam.ml.gcp import videointelligenceml (Notice I also dropped _ between the words.)

What do you think?

[mwalenia]

retest this please

[mwalenia]

retest this please

[mwalenia]

@EDjur your phrase triggers unfortunately won't trigger the tests - there are Jenkins restrictions that won't let you do it

[EDjur]

@EDjur your phrase triggers unfortunately won't trigger the tests - there are Jenkins restrictions that won't let you do it

Gotcha. Thanks for triggering them!

[mwalenia]

retest this please

[EDjur]

I'm having a small discussion regarding the video_context with @kamilwu here: https://issues.apache.org/jira/browse/BEAM-9247

Perhaps this is something we should look at integrating to this PR before (or after considering the use-case seems small) merging too.

[mwalenia]

retest this please

[EDjur]

Thanks for retesting! Test failures look unrelated to this PR.

11:04:57 self = <apache_beam.runners.interactive.caching.streaming_cache_test.InMemoryReader object at 0x7fdcc6b742d0>
11:04:57 watermark = 0, processing_time = 1
11:04:57 
11:04:57     def advance_watermark(self, watermark, processing_time):
11:04:57       record = TestStreamFileRecord(
11:04:57           watermark=Timestamp.of(watermark).to_proto(),
11:04:57 >         processing_time=Timestamp.of(processing_time).to_proto())
11:04:57 E     ValueError: Protocol message TestStreamFileRecord has no "processing_time" field.

[aaltay]

Test error seems to be related to: https://github.com/apache/beam/pull/10856/files

[aaltay]

retest this please

[aaltay]

retest this please

[aaltay]

retest this please

[aaltay]

retest this please

[aaltay]

:sdks:python:test-suites:tox:pycommon:docs task is failing -- There might be pydocs issues in the change.

[EDjur]

Seems yapf doesn't format docstrings 🤷‍♂ Should be fixed now.

[kkucharc]

retest this please

[aaltay]

@EDjur -- Thank you, merged this.

Could we update 'google-cloud-videointelligence>=1.8.0<=1.12.1', to use the recently released 1.13.0 version as well? Do we need anything special?

[EDjur]

Looks fine to update! The 1.13.0 essentially added two new enums to the types.Feature, but since these are specified by the user, all should be well.

I also manually reran all the tests on version 1.13.0 without mocking the videointelligence client and they ran as expected.

Cheers!