[BEAM-10961] Enabled strict dependency on sdks-extensions #13686

sonam-vend · 2021-01-07T08:45:02Z

Please add a meaningful description for your change here

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	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang		---	---

Pre-Commit Tests Status (on master branch)

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

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

GitHub Actions Tests Status (on master branch)

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

sonam-vend · 2021-01-07T12:11:02Z

Run Java PreCommit

sonam-vend · 2021-01-25T06:37:33Z

Run Java PreCommit

shehzaadn-vd · 2021-01-27T14:06:58Z

R: @ibzib Requesting your attention on this PR too.

ibzib · 2021-01-27T18:55:51Z

Similar to sdks/java/io, extensions is a large folder that contains many different projects. It'd be better to break this one up too, by going one directory deeper (so sdks/java/extensions/sql can be in one largeish PR and everything else is by itself).

codecov · 2021-01-28T04:06:01Z

Codecov Report

Merging #13686 (7352ed1) into master (2946e4b) will decrease coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13686      +/-   ##
==========================================
- Coverage   82.99%   82.83%   -0.17%     
==========================================
  Files         469      466       -3     
  Lines       58330    57596     -734     
==========================================
- Hits        48412    47710     -702     
+ Misses       9918     9886      -32

Impacted Files	Coverage Δ
...n/apache_beam/runners/direct/evaluation_context.py
...rcs/sdks/python/apache_beam/typehints/typehints.py
...hon/apache_beam/examples/wordcount_with_metrics.py
...srcs/sdks/python/apache_beam/io/gcp/dicomclient.py
...beam/testing/benchmarks/nexmark/queries/query11.py
.../snippets/transforms/elementwise/withtimestamps.py
...am/examples/complete/juliaset/juliaset/__init__.py
...srcs/sdks/python/apache_beam/transforms/display.py
.../examples/snippets/transforms/aggregation/count.py
...ache_beam/examples/cookbook/datastore_wordcount.py
... and 925 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 643ad9e...619d078. Read the comment docs.

shehzaadn-vd · 2021-01-28T05:22:06Z

Run Java PreCommit

shehzaadn-vd · 2021-02-01T02:36:26Z

@ibzib I don't seem to have the access/capability to re-run the failing "Java Tests / Java Unit Tests (macos-latest) (pull_request) " test above (per your tips in another PR). Could you please re-run it for me? Thanks in advance.

…with permitUnusedDeclared (2) alphabeticize some lines

shehzaadn-vd · 2021-02-06T12:15:12Z

@ibzib please see if with the latest changes this is good to go. thanks.

shehzaadn-vd · 2021-02-11T00:54:18Z

R: @ibzib

shehzaadn-vd · 2021-02-11T00:56:17Z

sdks/java/extensions/ml/build.gradle

    testCompile library.java.mockito_core
-    testCompile 'com.google.cloud:google-cloud-video-intelligence:1.2.0'


Removing these testCompile lines since the same dependencies already have compile lines.

ibzib · 2021-02-11T18:46:27Z

sdks/java/extensions/kryo/build.gradle

    compile "com.esotericsoftware:kryo:${kryoVersion}"
+    compile "org.objenesis:objenesis:2.5.1"


Noting that these dependencies are also included in the Java nature configuratioin's shadowClosure.dependencies. I'm don't think it's a problem, but I'm curious what is the difference between specifying dependencies in shadowClosure vs the regular dependencies block.

ibzib · 2021-02-11T18:49:39Z

sdks/java/extensions/ml/build.gradle


 description = 'Apache Beam :: SDKs :: Java :: Extensions :: ML'

 dependencies {
    compile project(path: ":sdks:java:core", configuration: "shadow")
    compile project(":sdks:java:expansion-service")
+    permitUnusedDeclared project(":sdks:java:expansion-service")


Please link the jira in a comment (BEAM-11761) for the permitUnusedDeclared instances we're not sure if we can remove.

ibzib · 2021-02-11T18:55:08Z

sdks/java/extensions/ml/build.gradle

    provided library.java.junit
    testCompile project(path: ':sdks:java:core', configuration: 'shadowTest')
    compile 'com.google.cloud:google-cloud-vision:1.99.3'
+    permitUsedUndeclared "com.google.auto.value:auto-value-annotations:1.7"


Why does this need to be permitUsedUndeclared? Don't we already declare it in BeamModulePlugin?

beam/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Line 870 in 705649d

"com.google.auto.value:auto-value-annotations:$autovalue_version",

@ibzib When i remove this line, i get the error:

Execution failed for task ':sdks:java:extensions:ml:analyzeClassesDependencies'. > Dependency analysis found issues. usedUndeclaredArtifacts: - com.google.auto.value:auto-value-annotations:1.7@jar

To dig into this further, I ran ../../../../gradlew -q dependencies in sdks/java/extensions/ml and captured the output here:
https://docs.google.com/document/d/1FNxea7Qbn8ckrphV25U7qh75a6z-PEl3iKgdJJwld5E/edit

It seems that via com.google.cloud:google-cloud-video-intelligence:1.2.0, we are getting runtimeClasspath dependencies to com.google.auto.value:auto-value-annotations:1.7 - which is not being resolved to 1.7.2, the version declared in BeamModulePlugin.groovy as a compileOnly dependency. See the highlighted lines in the doc I linked to above.

Maybe this is because BeamModulePlugin.groovy declares auto-value-annotations as compileOnly whereas it is actually needed at runtime.

Not sure if this could point to the the cause or it's just a red herring...?

ibzib · 2021-02-11T18:57:20Z

sdks/java/extensions/schemaio-expansion-service/build.gradle

@@ -31,11 +31,15 @@ applyJavaNature(

 dependencies {
    compile project(path: ":sdks:java:expansion-service")
+    permitUnusedDeclared project(path: ":sdks:java:expansion-service")


These are likely candidates for removal. We may even want to make a JIRA specifically for permitUnusedDeclared instances in *-expansion-service, because the solution for all these expansion services will be the same.

@ibzib By *-expansion-service, did you mean all these

./sdks/java/io/kinesis/expansion-service
./sdks/java/io/expansion-service
./sdks/java/io/snowflake/expansion-service
./sdks/java/io/google-cloud-platform/expansion-service
./sdks/java/expansion-service
./sdks/java/extensions/schemaio-expansion-service
./sdks/java/extensions/sql/expansion-service
./sdks/java/testing/expansion-service

I can file the JIRA as a sub-task of BEAM-11761.

I think we already merged some of these without a new JIRA. I don't think a new JIRA is strictly necessary - it's easy enough to find these instances if you're looking for them. So let's just use BEAM-11761.

ibzib · 2021-02-11T18:58:08Z

sdks/java/extensions/zetasketch/build.gradle

@@ -32,6 +33,7 @@ dependencies {
    compile library.java.slf4j_api
    compile library.java.vendored_guava_26_0_jre
    compile project(path: ":sdks:java:core", configuration: "shadow")
+    compile "com.google.auto.value:auto-value-annotations:1.6.3"


Why is this needed? Aren't we already importing autovalue in BeamModulePlugin?

beam/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy

Line 870 in 705649d

"com.google.auto.value:auto-value-annotations:$autovalue_version",

Same kind of thing as the comment i added for sdks/java/extensions/ml/build.gradle. Here there is a runtime dependency on "com.google.auto.value:auto-value-annotations:1.6.3" via "com.google.zetasketch:zetasketch:0.1.0"

\--- com.google.zetasketch:zetasketch:0.1.0 +--- com.google.auto.value:auto-value-annotations:1.6.3 +--- com.google.code.findbugs:jsr305:3.0.2 +--- com.google.errorprone:error_prone_annotations:2.3.2 -> 2.3.1 +--- it.unimi.dsi:fastutil:8.2.2 \--- org.checkerframework:checker-qual:2.8.1 -> 3.7.0

ibzib · 2021-02-11T19:03:40Z

sdks/java/extensions/sketching/build.gradle

@@ -26,15 +27,14 @@ def streamlib_version = "2.9.5"
 def tdigest_version = "3.2"

 dependencies {
+  compile library.java.slf4j_api


I'm pretty sure this one is actually safe to remove. slf4j is used for logging, and this project doesn't contain any loggers. If anyone wants to add logging to this project, it's easy to add this dependency back in.

@ibzib I'm curious how to determine whether this project contains loggers, and why removing this particular dependency entirely seems safer than removing others (where we are adding permitUnusedDeclared).

…sketching/build.gradle

shehzaadn-vd · 2021-02-20T01:44:19Z

sdks/java/extensions/schemaio-expansion-service/build.gradle

    testCompile library.java.mockito_core
-    // TODO(BEAM-10632): remove this dependency
-    testCompile "org.checkerframework:checker-qual:3.10.0"


This was introduced by @kennknowles back in Oct. Since then we've moved the checker-qual dependency into BeamModulePlugin. Checking if it's ok to remove it from here.

shehzaadn-vd · 2021-02-25T08:23:48Z

Similar to sdks/java/io, extensions is a large folder that contains many different projects. It'd be better to break this one up too, by going one directory deeper (so sdks/java/extensions/sql can be in one largeish PR and everything else is by itself).

@ibzib Sorry - I had overlooked this comment earlier. Since then the review has made forward progress. Would you still like this PR split into individual files?

aaltay · 2021-03-11T21:21:36Z

Can we close this PR? It looks like it was split into smaller ones and merged.

probot-autolabeler bot added build euphoria extensions google-cloud-platform-core jackson java join-library kryo protobuf sketching sorter sql zetasketch labels Jan 7, 2021

sonam-vend force-pushed the sdks-extensions branch from 255ff5b to 0ef8888 Compare January 22, 2021 11:16

Enabled strict dependency on sdks-extensions

6c9550c

shehzaadn-vd force-pushed the sdks-extensions branch from 7352ed1 to 6c9550c Compare January 28, 2021 03:56

shehzaadn-vd added 2 commits January 27, 2021 20:06

move sdks/java/extensions/sql changes to their own PR

29b2183

enableStrictDependencies and other cleanup

57b9451

shehzaadn-vd mentioned this pull request Jan 28, 2021

[BEAM-10961] Enable strict dependency checking on sdks/java/extensions/sql #13830

Merged

4 tasks

declare auto-value-annotations

3a2fa11

shehzaadn-vd added 2 commits February 5, 2021 17:00

Merge branch 'master' into sdks-extensions

d794f6a

[BEAM-10961] (1) reintroduce previously removed compile lines paired …

b679b53

…with permitUnusedDeclared (2) alphabeticize some lines

[BEAM-10961] some cleanup

77fddd3

shehzaadn-vd reviewed Feb 11, 2021

View reviewed changes

ibzib reviewed Feb 11, 2021

View reviewed changes

ibzib changed the title ~~Enabled strict dependency on sdks-extensions~~ [BEAM-10961] Enabled strict dependency on sdks-extensions Feb 11, 2021

shehzaadn-vd added 2 commits February 12, 2021 23:00

[BEAM-10961] remove library.java.slf4j_api from sdks/java/extensions/…

ac654b3

…sketching/build.gradle

merge with master

73dfeae

shehzaadn-vd reviewed Feb 20, 2021

View reviewed changes

trigger build

7ff6f4f

shehzaadn-vd added 2 commits February 25, 2021 00:28

[BEAM-10961] add reference to BEAM-11761

740e349

Merge branch 'master' into sdks-extensions

619d078

ibzib closed this Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-10961] Enabled strict dependency on sdks-extensions #13686

[BEAM-10961] Enabled strict dependency on sdks-extensions #13686

sonam-vend commented Jan 7, 2021

sonam-vend commented Jan 7, 2021

sonam-vend commented Jan 25, 2021

shehzaadn-vd commented Jan 27, 2021

ibzib commented Jan 27, 2021

codecov bot commented Jan 28, 2021 •

edited

shehzaadn-vd commented Jan 28, 2021

shehzaadn-vd commented Feb 1, 2021 •

edited

shehzaadn-vd commented Feb 6, 2021

shehzaadn-vd commented Feb 11, 2021

shehzaadn-vd Feb 11, 2021

ibzib Feb 11, 2021

ibzib Feb 11, 2021

ibzib Feb 11, 2021

shehzaadn-vd Feb 12, 2021 •

edited

ibzib Feb 11, 2021

shehzaadn-vd Feb 12, 2021 •

edited

ibzib Feb 26, 2021

ibzib Feb 11, 2021

shehzaadn-vd Feb 12, 2021 •

edited

ibzib Feb 11, 2021

shehzaadn-vd Feb 12, 2021

shehzaadn-vd Feb 13, 2021

shehzaadn-vd Feb 20, 2021 •

edited

shehzaadn-vd commented Feb 25, 2021 •

edited

aaltay commented Mar 11, 2021

		testCompile library.java.mockito_core
		testCompile 'com.google.cloud:google-cloud-video-intelligence:1.2.0'

		compile "com.esotericsoftware:kryo:${kryoVersion}"
		compile "org.objenesis:objenesis:2.5.1"

[BEAM-10961] Enabled strict dependency on sdks-extensions #13686

[BEAM-10961] Enabled strict dependency on sdks-extensions #13686

Conversation

sonam-vend commented Jan 7, 2021

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

sonam-vend commented Jan 7, 2021

sonam-vend commented Jan 25, 2021

shehzaadn-vd commented Jan 27, 2021

ibzib commented Jan 27, 2021

codecov bot commented Jan 28, 2021 • edited

Codecov Report

shehzaadn-vd commented Jan 28, 2021

shehzaadn-vd commented Feb 1, 2021 • edited

shehzaadn-vd commented Feb 6, 2021

shehzaadn-vd commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehzaadn-vd Feb 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehzaadn-vd Feb 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehzaadn-vd Feb 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shehzaadn-vd Feb 20, 2021 • edited

Choose a reason for hiding this comment

shehzaadn-vd commented Feb 25, 2021 • edited

aaltay commented Mar 11, 2021

codecov bot commented Jan 28, 2021 •

edited

shehzaadn-vd commented Feb 1, 2021 •

edited

shehzaadn-vd Feb 12, 2021 •

edited

shehzaadn-vd Feb 12, 2021 •

edited

shehzaadn-vd Feb 12, 2021 •

edited

shehzaadn-vd Feb 20, 2021 •

edited

shehzaadn-vd commented Feb 25, 2021 •

edited