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-11289] [Java] Integrate Google Cloud Recommendations AI functio… #13645

Merged
merged 17 commits into from Jun 19, 2021

Conversation

matthiasa4
Copy link
Contributor

@matthiasa4 matthiasa4 commented Jan 3, 2021

Integrate Google Cloud Recommendations AI functionality


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 Dataflow Flink Samza Spark Twister2
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
Build Status
Python Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
--- Build Status ---
XLang Build Status Build Status Build Status --- Build Status ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website Whitespace Typescript
Non-portable Build Status Build Status
Build Status
Build Status
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.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@matthiasa4
Copy link
Contributor Author

Quick question - due to the usage of GenericJsons in the transforms, I had to add 2 coders (DelegatingAtomicCoder.java & GenericJsonCoder.java). I don't think that the current folder makes sense to put these, but wasn't entirely sure if they belonged in sdks/java/core/src/main/java/org/apache/beam/sdk/coders either. What would you suggest?

@matthiasa4 matthiasa4 force-pushed the BEAM-11289-java-recommendation-ai branch from cbf20b2 to ff7fba0 Compare January 9, 2021 16:38
@pabloem
Copy link
Member

pabloem commented Jan 20, 2021

taking a look at this...

@pabloem
Copy link
Member

pabloem commented Jan 20, 2021

to get passing RAT PreCommit, you need to add apache license headers to all new files : ) or excemptions for files that can't fit a comment with the license

@matthiasa4 matthiasa4 force-pushed the BEAM-11289-java-recommendation-ai branch 4 times, most recently from 2a1fb72 to 51120a3 Compare January 25, 2021 08:29
@matthiasa4
Copy link
Contributor Author

@pabloem thanks - made some changes and looks like the tests are passing now :)

@aaltay aaltay marked this pull request as ready for review February 9, 2021 01:49
@matthiasa4 matthiasa4 force-pushed the BEAM-11289-java-recommendation-ai branch from 51120a3 to c385c2c Compare February 28, 2021 22:38
Copy link
Member

@pabloem pabloem left a comment

Choose a reason for hiding this comment

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

added a few comments

@pabloem
Copy link
Member

pabloem commented Apr 23, 2021

Run Java PostCommit

@matthiasa4 matthiasa4 force-pushed the BEAM-11289-java-recommendation-ai branch from 880a80d to a8604af Compare April 27, 2021 19:00
@matthiasa4 matthiasa4 requested a review from pabloem April 28, 2021 12:00
@matthiasa4
Copy link
Contributor Author

hey @pabloem - I have addressed the comments we discussed, can you take another look please? :)

@pabloem
Copy link
Member

pabloem commented Apr 28, 2021

Run Java PreCommit

@matthiasa4
Copy link
Contributor Author

@pabloem looks like those test failures are not related to this PR?

@pabloem
Copy link
Member

pabloem commented Apr 29, 2021

Run Java PreCommit

Copy link
Member

@pabloem pabloem left a comment

Choose a reason for hiding this comment

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

Sorry, I just added one more comment to remove public where possible, and to depend on BeamModulePlugin. All else looks good.

@matthiasa4 matthiasa4 requested a review from pabloem April 30, 2021 17:57
@github-actions
Copy link
Contributor

The Workflow run is cancelling this PR. It is an earlier duplicate of 2173354 run.

@github-actions
Copy link
Contributor

The Workflow run is cancelling this PR. It is an earlier duplicate of 2173354 run.

@pabloem
Copy link
Member

pabloem commented Apr 30, 2021

Run Java PostCommit

@pabloem
Copy link
Member

pabloem commented Apr 30, 2021

Run PostCommit_Java_DataflowV2

@pabloem
Copy link
Member

pabloem commented May 1, 2021

note that some tests are failing: https://ci-beam.apache.org/job/beam_PostCommit_Java_PR/680/

@matthiasa4
Copy link
Contributor Author

Hey @pabloem - the tests (for CatalogItem and UserEvent) work locally for me, but I am hitting my own GCP project (since the tests are calling APIs inside a project that needs the API enabled). I think this is why the tests are returning null values. Not sure if this is the only cause but is the apache-beam-testing project a real GCP project, does the service account that runs the test have access to it and are the APIs enabled on the other side?

For the Predict test I think the reason is related: since the model has no data yet (and we are trying to get a prediction based on recently_viewed_items) the resulting prediction returns empty. Either we need to put data in there or, alternatively, we could check if the faulty PCollection is empty. Wdyt?

Adjusting docs
Adjusting prediction eventtype
@matthiasa4
Copy link
Contributor Author

Run Java PostCommit

@matthiasa4
Copy link
Contributor Author

@pabloem could it be that the serviceaccount running the tests doesn't have the correct permissions yet? (e.g. Recommendations AI Admin)

@pabloem
Copy link
Member

pabloem commented May 19, 2021

Run Java PostCommit

@aaltay
Copy link
Member

aaltay commented May 27, 2021

post commit test is still failing. - What is the next step on this PR?

@matthiasa4
Copy link
Contributor Author

@aaltay I would assume the same trouble as we have on the Python PR (service account permissions)

@pabloem
Copy link
Member

pabloem commented Jun 7, 2021

Run Java PostCommit

1 similar comment
@matthiasa4
Copy link
Contributor Author

Run Java PostCommit

@matthiasa4
Copy link
Contributor Author

@pabloem @aaltay looks like tests are passing now! Thanks for sorting permissions. Shall I squash & merge?

@pabloem
Copy link
Member

pabloem commented Jun 18, 2021

sorry about the delay! - yes, I think you can squash and merge please @matthiasa4 : )

@pabloem
Copy link
Member

pabloem commented Jun 18, 2021

I can do it if you prefer, but since you're a committer, feel free to merge it yourself. Thanks!

@matthiasa4 matthiasa4 merged commit c986168 into apache:master Jun 19, 2021
@Override
public PCollectionTuple expand(PCollection<GenericJson> input) {
return input.apply(
ParDo.of(new WriteUserEvent(projectId(), catalogName(), eventStore()))
Copy link
Member

Choose a reason for hiding this comment

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

Based on the usage here, it seems like projectId cannot actually be null. I see that it is explicitly nullable, though. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Is it to allow the withXYZ pattern? And then we should validate the final state of everything at the beginning of expand?

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.

None yet

4 participants