Cross-compile all shims from JDK17 to JDK8 #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Eliminate Logging inheritance to prevent shimming of unshimmable API classes Signed-off-by: Gera Shegalov <[email protected]>
Signed-off-by: Gera Shegalov <[email protected]>
Signed-off-by: Gera Shegalov <[email protected]>
razajafri
reviewed
Jun 27, 2024
| object ShimLoader extends Logging { | ||
| logDebug(s"ShimLoader object instance: $this loaded by ${getClass.getClassLoader}") | ||
| object ShimLoader { | ||
| val log = org.slf4j.LoggerFactory.getLogger(getClass().getName().stripSuffix("$")) |
There was a problem hiding this comment.
So much better than what we were previously doing! I don't know why we even bothered with the Logging trait
There was a problem hiding this comment.
It is/was a convenient pattern but it is an internal class that would now need shimming. And resulted in a chicken-and-egg problem for shim loading.
| @@ -173,12 +173,12 @@ function verify_same_sha_for_unshimmed() { | |||
| # TODO currently RapidsShuffleManager is "removed" from /spark* by construction in | |||
| # dist pom.xml via ant. We could delegate this logic to this script | |||
| # and make both simmpler | |||
| if [[ ! "$class_file_quoted" =~ (com/nvidia/spark/rapids/spark[34].*/.*ShuffleManager.class|org/apache/spark/sql/rapids/shims/spark[34].*/ProxyRapidsShuffleInternalManager.class) ]]; then | |||
| if [[ ! "$class_file_quoted" =~ com/nvidia/spark/rapids/spark[34].*/.*ShuffleManager.class ]]; then | |||
There was a problem hiding this comment.
I don't understand why we got rid of the ProxyRapidsShuffleInternalManager
There was a problem hiding this comment.
tech debt, should have been done in NVIDIA#6030
|
Javac is failing |
Signed-off-by: Gera Shegalov <[email protected]>
|
the failing check are JDK8 builds. Fixed some issues, you can fix the rest, |
razajafri
merged commit 96e0843
into
razajafri:SP-9259-POM-changes
5 of 7 checks passed
razajafri
added a commit
that referenced
this pull request
Jun 28, 2024
razajafri
added a commit
to NVIDIA/spark-rapids
that referenced
this pull request
Jul 16, 2024
…s] (#10994) * POM changes for Spark 4.0.0 Signed-off-by: Raza Jafri <[email protected]> * validate buildver and scala versions * more pom changes * fixed the scala-2.12 comment * more fixes for scala-2.13 pom * addressed comments * add in shim check to account for 400 * add 400 for premerge tests against jdk 17 * temporarily remove 400 from snapshotScala213 * fixed 2.13 pom * Remove 400 from jdk17 as it will compile with Scala 2.12 * github workflow changes * added quotes to pom-directory * update version defs to include scala 213 jdk 17 * Cross-compile all shims from JDK17 to JDK8 Eliminate Logging inheritance to prevent shimming of unshimmable API classes Signed-off-by: Gera Shegalov <[email protected]> * dummy * undo api pom change Signed-off-by: Gera Shegalov <[email protected]> * Add preview1 to the allowed shim versions Signed-off-by: Gera Shegalov <[email protected]> * Scala 2.13 to require JDK17 Signed-off-by: Gera Shegalov <[email protected]> * Removed unused import left over from razajafri#3 * Setup JAVA_HOME before caching * Only upgrade the Scala plugin for Scala 2.13 * Regenerate Scala 2.13 poms * Remove 330 from JDK17 builds for Scala 2.12 * Revert "Remove 330 from JDK17 builds for Scala 2.12" This reverts commit 1faabd4. * Downgrade scala.plugin.version for cloudera * Updated comment to include the issue * Upgrading the scala.maven.plugin version to 4.9.1 which is the same as Spark 4.0.0 * Downgrade scala-maven-plugin for Cloudera * revert mvn verify changes * Avoid cache for JDK 17 * removed cache dep from scala 213 * Added Scala 2.13 specific checks * Handle the change for UnaryPositive now extending RuntimeReplaceable * Removing 330 from jdk17.buildvers as we only support Scala2.13 and fixing the enviornment variable in version-defs.sh that we read for building against JDK17 with Scala 213 * Update Scala 2.13 poms * fixed scala2.13 verify to actually use the scala2.13/pom.xml * Added missing csv files * Skip Opcode tests There is a bytecode incompatibility which is why we are skipping these until we add support for it. For details please see the following two issues #11174 #10203 * upmerged and fixed the new compile error introduced * addressed review comments * Removed jdk17 cloudera check and moved it inside the 321,330 and 332 cloudera profiles * fixed upmerge conflicts * reverted renaming of id * Fixed HiveGenericUDFShim * addressed review comments * reverted the debugging code * generated Scala 2.13 poms --------- Signed-off-by: Raza Jafri <[email protected]> Signed-off-by: Gera Shegalov <[email protected]> Co-authored-by: Gera Shegalov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Additional changes
Testing
Ran smoke tests in spark-shell to the tune of
Run 4.0.0-preview1 with JDK 17
$ TZ=UTC JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 \ ~/dist/spark-4.0.0-preview1-bin-hadoop3/bin/spark-shell \ --master local-cluster[2,1,1024] \ --jars scala2.13/dist/target/rapids-4-spark_2.13-24.08.0-SNAPSHOT-cuda11.jar \ --conf spark.plugins=com.nvidia.spark.SQLPlugin \ --conf spark.rapids.sql.explain=ALL \ --conf spark.rapids.memory.gpu.allocSize=1536mRun 3.3.0 with JDK 8 with the same jar
$ TZ=UTC JAVA_HOME=/usr/lib/jvm/java-8-openjdk-amd64 \ ~/dist/spark-3.3.0-bin-hadoop3-scala2.13/bin/spark-shell \ --master local-cluster[2,1,1024] \ --jars scala2.13/dist/target/rapids-4-spark_2.13-24.08.0-SNAPSHOT-cuda11.jar \ --conf spark.plugins=com.nvidia.spark.SQLPlugin \ --conf spark.rapids.sql.explain=ALL \ --conf spark.rapids.memory.gpu.allocSize=1536mNote that RapidsShuffleManager does not work yet because we switched the plugin and SM initialization order in 4.0, and so the plugin cannot call initialize on null SM.
Signed-off-by: Gera Shegalov [email protected]