Skip to content

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
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

@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