Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-9147] Add a VideoIntelligence transform to Java SDK #11261

Merged
merged 5 commits into from
Apr 8, 2020

Conversation

mwalenia
Copy link
Member

This PR adds a VideoIntelligence transform to Java SDK, analogous to Python transforms introduced
in #10764.


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

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

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

Post-Commit Tests Status (on master branch)

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

Pre-Commit Tests Status (on master branch)

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

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

@mwalenia
Copy link
Member Author

R: @Ardagan
CC: @kamilwu

@mwalenia
Copy link
Member Author

Run CommunityMetrics PreCommit

1 similar comment
@mwalenia
Copy link
Member Author

Run CommunityMetrics PreCommit

@mwalenia
Copy link
Member Author

mwalenia commented Apr 1, 2020

@Ardagan can you take a look at this or delegate to somebody else? Thanks!

Copy link
Contributor

@Ardagan Ardagan left a comment

Choose a reason for hiding this comment

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

I'm a bit confused which binary will these classes be added to. Do we need update of release process?

@Override
public Void apply(Iterable<List<VideoAnnotationResults>> input) {
List<Boolean> labelEvaluations = new ArrayList<>();
input.forEach(
Copy link
Contributor

Choose a reason for hiding this comment

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

This part is really hard to read. Would be good to add named methods or use similar approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, done. What do you think?

settings.gradle Outdated
@@ -165,3 +165,6 @@ include "beam-test-infra-metrics"
project(":beam-test-infra-metrics").dir = file(".test-infra/metrics")
include "beam-test-tools"
project(":beam-test-tools").dir = file(".test-infra/tools")
include 'sdks:java:extensions:ml'
Copy link
Contributor

Choose a reason for hiding this comment

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

move this up to the rest of extensions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, fixed

@mwalenia
Copy link
Member Author

mwalenia commented Apr 3, 2020

I'm a bit confused which binary will these classes be added to. Do we need update of release process?

I'm not sure how I can check this, do you have any pointers?

@mwalenia
Copy link
Member Author

mwalenia commented Apr 3, 2020

Run CommunityMetrics PreCommit

@mwalenia
Copy link
Member Author

mwalenia commented Apr 3, 2020

Run Python PreCommit

@mwalenia
Copy link
Member Author

mwalenia commented Apr 3, 2020

Run CommunityMetrics PreCommit

@Ardagan
Copy link
Contributor

Ardagan commented Apr 7, 2020

Run Python Precommit

@Ardagan
Copy link
Contributor

Ardagan commented Apr 7, 2020

Run CommunityMetrics PreCommit

@Ardagan
Copy link
Contributor

Ardagan commented Apr 7, 2020

Community metrics should be irrelevvant failure.

@mwalenia
Copy link
Member Author

mwalenia commented Apr 7, 2020

Run CommunityMetrics PreCommit

1 similar comment
@mwalenia
Copy link
Member Author

mwalenia commented Apr 8, 2020

Run CommunityMetrics PreCommit

@mwalenia mwalenia merged commit 825363b into master Apr 8, 2020
suztomo pushed a commit to suztomo/beam that referenced this pull request Apr 10, 2020
* [BEAM-9147] Add Google Cloud AI VideoIntelligence integration transform
@jkff
Copy link
Contributor

jkff commented Apr 17, 2020

Hi!

Drive-by comment: in the current form this transform violates the PTransform Style Guide, specifically, it exposes a set of DoFns rather than exposing a set of PTransforms.

There are several very compelling advantages to exposing PTransforms, mainly, compatibility: once it's a DoFn and clients use it via ParDo, you can never add anything else into it (e.g. a reshuffle) without breaking client code - but you can change the internal implementation of a PTransform at will. Many PTransforms that used to be DoFns before that style guide was created, eventually evolved to be more complex.

Could you consider restructuring it in a follow-up PR? A similar DoFn-like PTransform you can take example from is Regex.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants