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

[SPARK-48049][BUILD] Upgrade Scala to 2.13.14 #46288

Closed
wants to merge 17 commits into from

Conversation

panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Apr 29, 2024

What changes were proposed in this pull request?

The pr aims to upgrade scala from 2.13.13 to 2.13.14.

Why are the changes needed?

https://github.com/scala/scala/releases/tag/v2.13.14
image

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

@panbingkun
Copy link
Contributor Author

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Wow. Nice timing. Thank you, @panbingkun !

@dongjoon-hyun
Copy link
Member

Like we observed before, I guess we need to wait new compatible plugins and Ammonite shell additionally. Isn't it?

cc @LuciferYang , too

@panbingkun
Copy link
Contributor Author

Like we observed before, I guess we need to wait new compatible plugins and Ammonite shell additionally. Isn't it?

cc @LuciferYang , too

Yeah, that's right. 😄
I guess it should be able to catch up with spark4.0 release.

@panbingkun panbingkun changed the title [WIP][SPARK-48049][BUILD] Upgrade Scala to 2.13.14 [SPARK-48049][BUILD] Upgrade Scala to 2.13.14 Apr 30, 2024
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

@SethTisue
Copy link

https://repo1.maven.org/maven2/com/typesafe/genjavadoc/genjavadoc-plugin_2.13.14/0.19/

@dongjoon-hyun
Copy link
Member

Thank you so much, @SethTisue !

@dongjoon-hyun
Copy link
Member

Although we are waiting for Ammonite still, could you base this PR once more, @panbingkun ?

@panbingkun
Copy link
Contributor Author

Although we are waiting for Ammonite still, could you base this PR once more, @panbingkun ?

Sure, done.

@panbingkun
Copy link
Contributor Author

https://repo1.maven.org/maven2/com/typesafe/genjavadoc/genjavadoc-plugin_2.13.14/0.19/

I have updated the version of genjavadoc in the file project/SparkBuild.scala.

@dongjoon-hyun
Copy link
Member

Thanks!

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Just a question. CI seems to complain the following. Is this related to this PR?

Error:  Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.4.1:enforce (enforce-versions) on project spark-tools_2.13: 
Error:  Rule 3: org.codehaus.mojo.extraenforcer.dependencies.EnforceBytecodeVersion failed with message:
Error:  Found Banned Dependency: org.jline:jline:jar:3.25.1

@panbingkun
Copy link
Contributor Author

panbingkun commented May 2, 2024

jline

I guess it may be related to the fact that the Ammonite supporting scala 2.13.14 has not been released.
image

@@ -73,7 +73,7 @@
</dependency>
<dependency>
<groupId>com.lihaoyi</groupId>
<artifactId>ammonite_${scala.version}</artifactId>
<artifactId>ammonite_2.13.13</artifactId>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for test
After the Ammonite supports scala 2.13.14, I will restore it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's merged ten minutes ago. Looking forward to seeing the new release.

@dongjoon-hyun
Copy link
Member

Although we already use ammonite_2.13.13 without this jline issue, let's wait and see~

I guess it may be related to the fact that the Ammonite supporting scala 2.13.14 has not been released.

@github-actions github-actions bot removed the CONNECT label May 3, 2024
@panbingkun
Copy link
Contributor Author

panbingkun commented May 3, 2024

@SethTisue
Just right, you are here too
scala/scala#10717
https://github.com/scala/scala/pull/10717/files#diff-87e6fe6a086cec9825e72357099f0ae6b84800ea025c5aee97b6cc7088c6cd4d

I found that in scala-2.13.14, the version of org.jline:jline was upgraded from 3.14.1 to 3.15.1 because:
scala/bug#12957
image

But I found that org.jline:jline:3.15.1 is compiled based on JDK 21.
image
So the maven plugin maven-enforcer-plugin helped us find this issue when Spark upgraded the scala version from 2.13.13 to 2.13.14.
Because our current Spark master branch supports JDK 17 and JDK 21.
Based on the above reasons, I would like to ask if it is also reasonable and feasible for us to exclude org.jline:jline:3.15.1 from org.scala-lang:scala-compiler:jar:2.13.14, and explicitly let it use org.jline:jline:3.14.1?
image

Thank you!

@LuciferYang
Copy link
Contributor

@LuciferYang
Copy link
Contributor

LuciferYang commented May 3, 2024

[INFO] Restricted to JDK 17 yet org.jline:jline:jar:3.25.1:compile contains org/jline/terminal/impl/ffm/CLibrary$termios.class targeted to 65.-257

Oh... It seems that CLibrary$termios.class is not compatible with Java 8.

In jline 3.25.1, CLibrary references some implementations that were moved to the java.lang.foreign package in Java 21, but in Java 17, they were still in the jdk.incubator.foreign package.

However, in jline 3.24.1, CLibrary also import classes in java.lang.foreign package, it's just that the final jline jar does not include the terminal-ffm module, so it won't trigger a compilation error from the maven-enforcer-plugin.

So it seems that using jline 3.25.1 should not pose any issues. It's unclear whether it's possible to workaround this by modifying the rules of the maven-enforcer-plugin. @panbingkun

@panbingkun
Copy link
Contributor Author

Do we have a corresponding issue in the Scala community or JLine community (or GitHub issue)?

IMO, a. We can ignore Scala 2.13.14 completely due to this issue. b. We can cut the transitive dependency as you mentioned as Option 1. However, it's Scala's dependency. So, the downstream projects or users (who are not aware of this) will complain again and again. c. We cannot choose Option 2 because the downstream projects or users will be affected and complain again.

For Apache Spark 4.0.0-preview, let's choose (a) because we didn't have enough time to play around with this new Scala version.

Until 4.0.0 release, we had better keep tracking the upstream community (Scala/JLine) activity about this topic. The best case is for them to fix it for all downstream (including us).

@dongjoon-hyun @LuciferYang
Let's keep an eye on it. I have submitted this issue to the scala community
scala/bug#12994
Thank all!

@panbingkun
Copy link
Contributor Author

panbingkun commented May 6, 2024

@dongjoon-hyun @LuciferYang
Based on the detailed explanation of scala/bug#12994,
image

and double-check by debug jline on local env. The verification process is as follows:
1.Based on this pr upgrade (the version of scala is 2.13.14 , the version of jline is 3.25.1)
compile spark by commands:

./build/mvn -Pyarn -Pkubernetes -Pvolcano -Phive -Phive-thriftserver -Phadoop-cloud -Pspark-ganglia-lgpl -Pkinesis-asl clean package -DskipTests
image

2.When open spark-shell with the debug mode on local , and we will see the following run thread stack

export SPARK_SUBMIT_OPTS=-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=8888
image

3.The code logic of org.jline:jline:3.25.1 is as follows:
https://github.com/jline/jline3/blob/jline-parent-3.25.1/terminal/src/main/java/org/jline/terminal/TerminalBuilder.java#L654-L675
image

When loading org.jline.terminal.ffm, it will automatically skip because of the exception

java.lang.UnsupportedClassVersionError: org/jline/terminal/impl/ffm/FfmTerminalProvider has been compiled by a more recent version of the Java Runtime (class file version 65.65535), this version of the Java Runtime only recognizes class file versions up to 61.0

so we can completely ignore it in the maven plugin maven-enforcer-plugin#enforceBytecodeVersion, because it will automatically skip because of loading failure in the lower version of JDK (JDK < 21).

@@ -2981,6 +2981,9 @@
<maxJdkVersion>${java.version}</maxJdkVersion>
<ignoredScopes>test</ignoredScopes>
<ignoredScopes>provided</ignoredScopes>
<ignoreClasses>
<ignoreClass>org.jline.terminal.impl.ffm.*</ignoreClass>
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment with the pointer scala/bug#12994 .

@dongjoon-hyun
Copy link
Member

Thank you, @panbingkun .

@dongjoon-hyun
Copy link
Member

@LuciferYang
Copy link
Contributor

I think we need wait a new Ammonite release version, as 3.0-M1 does not support Scala 2.13.14.

image

@panbingkun
Copy link
Contributor Author

I use this version [3.0.0-M1-19-a7973e17](https://mvnrepository.com/artifact/com.lihaoyi/ammonite_3.3.3/3.0.0-M1-19-a7973e17), which currently supports scala 2.13.14
https://mvnrepository.com/artifact/com.lihaoyi/ammonite
image

@LuciferYang
Copy link
Contributor

I use this version [3.0.0-M1-19-a7973e17](https://mvnrepository.com/artifact/com.lihaoyi/ammonite_3.3.3/3.0.0-M1-19-a7973e17), which currently supports scala 2.13.14 https://mvnrepository.com/artifact/com.lihaoyi/ammonite image

This seems to be a snapshot or test version, not an official release?

@panbingkun
Copy link
Contributor Author

panbingkun commented May 7, 2024

I have asked the community to release an official version for us at his convenience
com-lihaoyi/Ammonite#1481 (comment)

@dongjoon-hyun
Copy link
Member

Thank you for confirming. Ya, let's wait.

@LuciferYang
Copy link
Contributor

https://github.com/com-lihaoyi/Ammonite/releases/tag/3.0.0-M2

3.0.0-M2 released ~ @panbingkun

@panbingkun
Copy link
Contributor Author

https://github.com/com-lihaoyi/Ammonite/releases/tag/3.0.0-M2

3.0.0-M2 released ~ @panbingkun

Thanks~ ❤️
Updated.

@panbingkun panbingkun marked this pull request as ready for review May 14, 2024 09:24
@@ -266,7 +266,7 @@ object SparkBuild extends PomBuild {
.orElse(sys.props.get("java.home").map { p => new File(p).getParentFile().getAbsolutePath() })
.map(file),
publishMavenStyle := true,
unidocGenjavadocVersion := "0.18",
unidocGenjavadocVersion := "0.19",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please upgrade GenJavadoc in a separate pr first. @panbingkun

Copy link
Contributor Author

@panbingkun panbingkun May 14, 2024

Choose a reason for hiding this comment

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

Done
A separate pr for upgrade GenJavadoc: #46579

Copy link
Contributor Author

@panbingkun panbingkun May 14, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

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

+1, LGTM

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @panbingkun and @LuciferYang and all.
Merged to master for Apache Spark 4.0.0-preview2-rc2.

szehon-ho pushed a commit to szehon-ho/spark that referenced this pull request Aug 7, 2024
The pr aims to upgrade `scala` from `2.13.13` to `2.13.14`.

https://github.com/scala/scala/releases/tag/v2.13.14
<img width="984" alt="image" src="https://github.com/apache/spark/assets/15246973/179274bf-11c0-472c-a6b6-217b9904832d">

No.

Pass GA.

No.

Closes apache#46288 from panbingkun/SPARK-48049.

Authored-by: panbingkun <panbingkun@baidu.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
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.

4 participants