Skip to content

Conversation

@pan3793
Copy link
Member

@pan3793 pan3793 commented Jan 14, 2025

What changes were proposed in this pull request?

Update GitHub workflow files and related scripts to enable SBT CI for the profiler module.

Why are the changes needed?

Currently, the profiler module is not covered in regular PR's SBT CI.

Does this PR introduce any user-facing change?

No, dev only.

How was this patch tested?

https://github.com/pan3793/spark/actions/runs/12770426351/job/35595345586

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

No.

@pan3793 pan3793 marked this pull request as ready for review January 14, 2025 07:20
@github-actions github-actions bot added the INFRA label Jan 14, 2025
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.

It sounds a little strange because SBT already compiles it like the following.

Currently, the profiler module only supports building via Maven, we should add SBT support too.

$ ./build/sbt -Pjvm-profiler clean "profiler/package"
...
[info] compiling 3 Scala sources to /Users/dongjoon/APACHE/spark-merge/connector/profiler/target/scala-2.13/classes ...
[success] Total time: 38 s, completed Jan 14, 2025, 7:12:50 AM
$ ls -al connector/profiler/target/scala-2.13/classes/org/apache/spark/executor/profiler/

total 128
drwxr-xr-x  8 dongjoon  staff    256 Jan 14 07:12 .
drwxr-xr-x  3 dongjoon  staff     96 Jan 14 07:12 ..
-rw-r--r--  1 dongjoon  staff    967 Jan 14 07:12 ExecutorJVMProfiler$$anon$1.class
-rw-r--r--  1 dongjoon  staff  26001 Jan 14 07:12 ExecutorJVMProfiler.class
-rw-r--r--  1 dongjoon  staff   1229 Jan 14 07:12 ExecutorProfilerPlugin.class
-rw-r--r--  1 dongjoon  staff  14175 Jan 14 07:12 JVMProfilerExecutorPlugin.class
-rw-r--r--  1 dongjoon  staff   5398 Jan 14 07:12 package$.class
-rw-r--r--  1 dongjoon  staff   1197 Jan 14 07:12 package.class

Could you revise the PR description more accurately?

@pan3793
Copy link
Member Author

pan3793 commented Jan 14, 2025

Interesting ... it was not listed in project/SparkBuild.scala but still works, I'm not very familiar with the SBT building system, maybe @LuciferYang knows why? and should we list the profiler module in project/SparkBuild.scala explicitly?

@pan3793
Copy link
Member Author

pan3793 commented Jan 14, 2025

maybe I should convert the purpose of this PR to "Enable SBT CI for profiler module"

@dongjoon-hyun
Copy link
Member

Yes, +1 for re-write the scope. Please describe the problem exactly. Then, I can help you easily.

@pan3793 pan3793 changed the title [SPARK-50810][BUILD] SBT supports building profiler module [SPARK-50810][BUILD] Enable SBT CI for profiler module Jan 14, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

After enabling -Pjvm-profiler, the sbt-pom-reader plugin will complete the import of the SBT configuration for the profiler module. Therefore, if we only wish to trigger the compilation of this module during the GitHub Action execution, there is no need to modify SparkBuild.scala.

Copy link
Member Author

@pan3793 pan3793 Jan 15, 2025

Choose a reason for hiding this comment

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

@LuciferYang thanks for the guidance, after some local experiments, I think we must modify SparkBuild.scala to explicitly list the profiler module, otherwise MimaKeys.mimaFailOnNoPrevious := false won't take effect.

@LuciferYang
Copy link
Contributor

spark/project/SparkBuild.scala

Lines 1468 to 1478 in 2d498d5

lazy val settings = baseSettings ++ Seq(
(ScalaUnidoc / unidoc / unidocProjectFilter) :=
inAnyProject -- inProjects(OldDeps.project, repl, examples, tools, kubernetes,
yarn, tags, streamingKafka010, sqlKafka010, connectCommon, connect, connectClient,
connectShims, protobuf),
(JavaUnidoc / unidoc / unidocProjectFilter) :=
inAnyProject -- inProjects(OldDeps.project, repl, examples, tools, kubernetes,
yarn, tags, streamingKafka010, sqlKafka010, connectCommon, connect, connectClient,
connectShims, protobuf),
)
}

Perhaps there is a potential requirement to define the profiler in SparkBuild.scala to exclude this module from the unidoc build. However, even if such a requirement exists, it should be addressed in a separate PR.

@pan3793 pan3793 marked this pull request as draft January 14, 2025 16:18
@pan3793 pan3793 marked this pull request as ready for review January 15, 2025 07:45
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, @pan3793 .
Merged to master for Apache Spark 4.0.0.

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