chore: Create initial release process scripts for official ASF source release #429

andygrove · 2024-05-14T16:09:01Z

Which issue does this PR close?

Part of #394

Rationale for this change

We need to establish the basic ASF source release process

What changes are included in this PR?

Copy create-tarball, release-tarball, verify-release-candidate scripts from DataFusion and modify them for Comet
Change the crate name from comet to datafusion-comet (comet was taken, and I have reserved datafusion-comet in crates.io)
Add placeholder README

How are these changes tested?

They are not tested yet. I will test them prior to our first release.

andygrove · 2024-05-14T16:10:09Z

core/Cargo.toml

@@ -16,7 +16,7 @@
 # under the License.

 [package]
-name = "comet"
+name = "datafusion-comet"


There is already a comet crate, so I renamed this. I have reserved the crate: https://crates.io/crates/datafusion-comet

andygrove · 2024-05-14T16:14:39Z

@snmvaughan @parthchandra please review when you have the time

viirya

Hmm, I think this is for publishing cargo package. But I think Comet should be released as a maven package at maven repository? So users can simply add Comet as a dependency in their pom file?

andygrove · 2024-05-14T18:00:20Z

Hmm, I think this is for publishing cargo package. But I think Comet should be released as a maven package at maven repository? So users can simply add Comet as a dependency in their pom file?

It depends whether we want to allow other Rust projects to be able to use the Spark-compatible expressions that we are implementing, outside of the Comet/Spark integration.

The maven release will be needed for sure.

core/Cargo.toml

Co-authored-by: Oleks V <[email protected]>

andygrove · 2024-05-15T15:02:42Z

Hmm, I think this is for publishing cargo package. But I think Comet should be released as a maven package at maven repository? So users can simply add Comet as a dependency in their pom file?

Also, regardless of whether we choose to publish the crate or not, I think it is better to not use the name of an existing crate, and good to have the author information be consistent with other DataFusion crates.

andygrove · 2024-06-04T12:12:48Z

@viirya @comphead @parthchandra @kazuyukitanimura @huaxingao @advancedxy Could I get more feedback on this PR so that we can prepare for the upcoming 0.1.0 source release, which we are planning to do once we have switched back to depending on an official DataFusion release.

advancedxy

Left some minor comments. I think the scripts are in good shape.

advancedxy · 2024-06-04T13:14:27Z

core/Cargo.toml

+homepage = "https://datafusion.apache.org/comet"
+repository = "https://github.com/apache/datafusion-comet"
+authors = ["Apache DataFusion <[email protected]>"]
+description = "Apache DataFusion Comet: Apache Spark native query execution plugin"


to be consistent with the README.md in the repo, this could be changed as Apache DataFusion Comet: a high-performance accelerator for Apache Spark?

It would be good to standardize our description

I would vote for the description in the README.
nit: high performance is two words (i.e. not hyphenated)

I have updated this

advancedxy · 2024-06-04T13:28:47Z

dev/release/create-tarball.sh

+
+set -e
+
+SOURCE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"


I would rather call it DEV_RELEASE_DIR instead.

advancedxy · 2024-06-04T13:29:17Z

dev/release/create-tarball.sh

+(cd "${SOURCE_TOP_DIR}" && git archive ${release_hash} --prefix ${release}/ | gzip > ${tarball})
+
+echo "Running rat license checker on ${tarball}"
+${SOURCE_DIR}/run-rat.sh ${tarball}


I think you missed this run-rat.sh in this commit.

advancedxy · 2024-06-04T13:39:52Z

dev/release/verify-release-candidate.sh

+  cargo build
+  cargo test --all --features=avro


If we are going to build and test rust code, we should do it for java/scala code as well?

It just occurred to me maybe we should make this script as a github action and provided in the vote email so that the basic verification is already performed and checked. The community is also welcome to test more combinations.
The previous statement is not a blocker.

Great point about testing the scala code. I will add that.

…et into release-process

comphead · 2024-06-04T15:13:53Z

dev/release/check-rat-report.py

+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0


we do have a RAT check from maven, is this check a replacement or covers other usecase?

It looks like we somehow missed adding the RAT check in maven. I was also surprised.

@comphead I misread your comment. Where is the rat check in maven?

nm, I see it now.

We need to run the rat check against the source tarball before we release it. The maven project is checking the contents of the repo not the release tarball, so maybe we need both. I will check how we are doing this in DataFusion.

yeah, ideally to remove duplications, perhaps we can remove RAT check in maven then and live only with the release one

DataFusion uses the same rat script from CI and from verify-release.sh.

If we do the same with Comet and remove the maven task it could cause more work for developers because they would not catch issues as part of a standard maven build and would have to run a separate script. I am not sure if it is worth the effort to change this right now.

kazuyukitanimura

Mostly looks good. I feel we can merge and start using this.

kazuyukitanimura · 2024-06-04T20:14:11Z

dev/release/verify-release-candidate.sh

+  pushd core
+    RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+  popd
+  ./mvnw install -Prelease -DskipTests -P"spark-3.4" -Dmaven.gitcommitid.skip=true


Spark-3.4 is hardcoded now. What about making it a parameter.

Also does it make sense to use one of our make commands

We don't currently have a make command that specifies -Dmaven.gitcommitid.skip=true although one is being added in #512

Right, one hack could be something like make release PROFILES="-Dmaven.gitcommitid.skip=true"

Should we run tests as well? All comet tests pass for me when I run them though they do take a while to run.

Right, one hack could be something like make release PROFILES="-Dmaven.gitcommitid.skip=true"

That would be a hack. I would leave it as it is. I assume that we will update/deprecate some of this once we move to publishing binary releases.

Should we run tests as well? All comet tests pass for me when I run them though they do take a while to run.

I think that the tests take too long. We create the release candidate tarball from a tag in GitHub where we have already run all of the tests, so running them all again is potentially redundant.

Perhaps there is some subset of tests that we could run? 🤔

I think we should at least build the scala/java code here, the test could be skipped as all the tests should be ran on CI actions.

Spark-3.4 is hardcoded now. What about making it a parameter.

+1 for this. It could be default to 3.4 though.

./mvnw install -Prelease -DskipTests

IIRC, we should use ./mvnw verify instead? Because the install command will also deploy the comet jar into the local repository, which might mess up other tests/projects that will depend on comet-jar in the future.

Spark-3.4 is hardcoded now. What about making it a parameter.

I added a comment explaining that we are testing the latest supported Spark version.

I think we can make more improvements in the future but I would like to get the basic scripts in place first so that we have the ability to make a release.

parthchandra

LGTM. Some minor comments.

parthchandra · 2024-06-05T00:24:37Z

core/Cargo.toml

+homepage = "https://datafusion.apache.org/comet"
+repository = "https://github.com/apache/datafusion-comet"
+authors = ["Apache DataFusion <[email protected]>"]
+description = "Apache DataFusion Comet: Apache Spark native query execution plugin"


I would vote for the description in the README.
nit: high performance is two words (i.e. not hyphenated)

parthchandra · 2024-06-05T00:33:38Z

dev/release/create-tarball.sh

+(cd ${distdir} && shasum -a 512 ${tarname}) > ${tarball}.sha512
+
+
+echo "Uploading to datafusion dist/dev to ${url}"


Can committers do this or only PMC?

We should probably add to the README that the entire release process requires PMC (or committer if that is sufficient).

Some steps can be completed by committers (such as creating the PR to update the project version number and generating the changelog) but some steps do require PMC members.

I can add this to the README.

I think committer permission should be sufficient? The bandwidth of PMC is limited, it would be great that we can make this committer sufficient. When releasing on apache uniffle, I don't think we need PMC to be the release manager.

I have expanded the README to explain the different steps in the release process and who can do each part.

I also added some better info on verifying release candidates.

I think committer permission should be sufficient? The bandwidth of PMC is limited, it would be great that we can make this committer sufficient. When releasing on apache uniffle, I don't think we need PMC to be the release manager.

Only PMC members have write access to the subversion repositories as far as I know.

Also, IIRC, when we do binary releases, for maven artifacts, promoting release candidates to releases also requires PMC.

Thanks for the information, good to know.

parthchandra · 2024-06-05T00:45:06Z

dev/release/verify-release-candidate.sh

+  pushd core
+    RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+  popd
+  ./mvnw install -Prelease -DskipTests -P"spark-3.4" -Dmaven.gitcommitid.skip=true


Should we run tests as well? All comet tests pass for me when I run them though they do take a while to run.

parthchandra · 2024-06-05T00:46:32Z

dev/release/verify-release-candidate.sh

+  pushd core
+    RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+  popd
+  ./mvnw install -Prelease -DskipTests -P"spark-3.4" -Dmaven.gitcommitid.skip=true


Right, one hack could be something like make release PROFILES="-Dmaven.gitcommitid.skip=true"

That would be a hack. I would leave it as it is. I assume that we will update/deprecate some of this once we move to publishing binary releases.

advancedxy

Left some minor comments, otherwise LGTM.

advancedxy · 2024-06-05T13:36:46Z

dev/release/create-tarball.sh

+(cd ${distdir} && shasum -a 512 ${tarname}) > ${tarball}.sha512
+
+
+echo "Uploading to datafusion dist/dev to ${url}"


I think committer permission should be sufficient? The bandwidth of PMC is limited, it would be great that we can make this committer sufficient. When releasing on apache uniffle, I don't think we need PMC to be the release manager.

advancedxy · 2024-06-05T13:42:50Z

dev/release/run-rat.sh

+# generate the rat report
+$RAT $1 > rat.txt
+python3 $RELEASE_DIR/check-rat-report.py $RELEASE_DIR/rat_exclude_files.txt rat.txt > filtered_rat.txt
+cat filtered_rat.txt


I don't think this line is needed?

This shows the list of files that failed the rat test

I see. Nit:
How about moving this line to the else block in L41 and L42? So that it doesn't pollute the stdout.

Good idea. I have made that change.

advancedxy · 2024-06-05T13:44:42Z

dev/release/rat_exclude_files.txt

+core/Cargo.lock
+core/testdata/backtrace.txt
+core/testdata/stacktrace.txt
+dev/copyright/scala-header.txt


why this line? I think scala-header.txt is just a soft link to java-header.txt, which are identical

advancedxy · 2024-06-05T23:25:42Z

dev/release/run-rat.sh

+# generate the rat report
+$RAT $1 > rat.txt
+python3 $RELEASE_DIR/check-rat-report.py $RELEASE_DIR/rat_exclude_files.txt rat.txt > filtered_rat.txt
+cat filtered_rat.txt


I see. Nit:
How about moving this line to the else block in L41 and L42? So that it doesn't pollute the stdout.

advancedxy · 2024-06-05T23:31:04Z

dev/release/verify-release-candidate.sh

+  pushd core
+    RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+  popd
+  ./mvnw install -Prelease -DskipTests -P"spark-3.4" -Dmaven.gitcommitid.skip=true


Spark-3.4 is hardcoded now. What about making it a parameter.

+1 for this. It could be default to 3.4 though.

./mvnw install -Prelease -DskipTests

IIRC, we should use ./mvnw verify instead? Because the install command will also deploy the comet jar into the local repository, which might mess up other tests/projects that will depend on comet-jar in the future.

andygrove · 2024-06-07T15:24:16Z

@parthchandra @advancedxy I think I have addressed all of the feedback now

parthchandra · 2024-06-08T00:53:27Z

@parthchandra @advancedxy I think I have addressed all of the feedback now

LGTM

andygrove · 2024-06-08T12:39:29Z

@viirya @kazuyukitanimura @huaxingao could I get a committer review/approval

comphead · 2024-06-08T17:51:01Z

dev/release/verify-release-candidate.sh

+    RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+  popd
+  # test with the latest supported version of Spark
+  ./mvnw verify -Prelease -DskipTests -P"spark-3.4" -Dmaven.gitcommitid.skip=true


does it mean if we support Spark 3.5, Spark4.0, we need to change this line?
Wondering should we run it against all supported versions?

It probably makes sense to verify against the latest Spark version here. I'm hoping that people verify release by building for their Spark version and running against their workflows. This script is just a basic verification that the release builds ok.

comphead

lgtm thanks @andygrove
left a small comment

… release (apache#429)

Create initial release process scripts

a1ad401

andygrove commented May 14, 2024

View reviewed changes

Add Cargo meta

cbd0f27

andygrove marked this pull request as ready for review May 14, 2024 16:14

andygrove requested review from sunchao and viirya May 14, 2024 16:14

add ASF header

9ffd276

viirya reviewed May 14, 2024

View reviewed changes

andygrove changed the title ~~chore: Create initial release process scripts~~ chore: Create initial release process scripts for official ASF source release May 14, 2024

comphead reviewed May 15, 2024

View reviewed changes

core/Cargo.toml Outdated Show resolved Hide resolved

Update core/Cargo.toml

3d666a1

Co-authored-by: Oleks V <[email protected]>

andygrove requested review from kazuyukitanimura and huaxingao May 31, 2024 16:50

advancedxy reviewed Jun 4, 2024

View reviewed changes

andygrove added 3 commits June 4, 2024 08:35

Merge remote-tracking branch 'apache/main' into release-process

34d6d27

add rat

8c14352

Merge branch 'release-process' of github.com:andygrove/datafusion-com…

d56980b

…et into release-process

comphead reviewed Jun 4, 2024

View reviewed changes

andygrove added 3 commits June 4, 2024 10:37

build scala project

6bf9351

rename env vars

1f17f77

fix rat

3d55c98

kazuyukitanimura reviewed Jun 4, 2024

View reviewed changes

parthchandra reviewed Jun 5, 2024

View reviewed changes

advancedxy reviewed Jun 5, 2024

View reviewed changes

andygrove added 2 commits June 5, 2024 10:37

Add more content to README

56bca9f

Add more content to README

3d28857

advancedxy approved these changes Jun 5, 2024

View reviewed changes

address feedback

d07d3b9

advancedxy approved these changes Jun 8, 2024

View reviewed changes

comphead reviewed Jun 8, 2024

View reviewed changes

comphead approved these changes Jun 8, 2024

View reviewed changes

andygrove merged commit c87fabe into apache:main Jun 8, 2024
43 checks passed

himadripal pushed a commit to himadripal/datafusion-comet that referenced this pull request Sep 7, 2024

chore: Create initial release process scripts for official ASF source…

855f542

… release (apache#429)

andygrove deleted the release-process branch December 3, 2024 04:35


		set -e

		SOURCE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"

		(cd ${distdir} && shasum -a 512 ${tarname}) > ${tarball}.sha512


		echo "Uploading to datafusion dist/dev to ${url}"

chore: Create initial release process scripts for official ASF source release #429

chore: Create initial release process scripts for official ASF source release #429

Conversation

andygrove commented May 14, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Choose a reason for hiding this comment

andygrove commented May 14, 2024

viirya left a comment

Choose a reason for hiding this comment

andygrove commented May 14, 2024

andygrove commented May 15, 2024

andygrove commented Jun 4, 2024

advancedxy left a comment

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazuyukitanimura left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthchandra left a comment

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

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

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

Choose a reason for hiding this comment

advancedxy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

advancedxy Jun 5, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Jun 7, 2024

parthchandra commented Jun 8, 2024

andygrove commented Jun 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

andygrove commented May 14, 2024 •

edited

Loading

kazuyukitanimura left a comment •

edited

Loading

advancedxy Jun 5, 2024 •

edited

Loading