chore: Add GitHub workflow to publish Docker image by andygrove · Pull Request #847 · apache/datafusion-comet

andygrove · 2024-08-19T16:23:09Z

Which issue does this PR close?

Part of #721

Rationale for this change

Make it easier for users to use Comet

What changes are included in this PR?

New GitHub workflow
Modified Dockerfile to build from local repo contents instead of cloning Comet

How are these changes tested?

I tested building the Docker image locally.

I don't think I can test the workflow until this PR is merged 😞

parthchandra · 2024-08-19T16:32:17Z

+    - name: Set up Java
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'temurin'


Is this just for the build environment or does this get included in the image? Why not OpenJDK?

This is just for the build environment so that we can use Maven to get the Comet version number.

I removed this line - this was copied from an example

comphead · 2024-08-19T17:29:04Z

+      run: echo "The current Maven version is ${{ env.COMET_VERSION }}"
+    - name: Build and push Docker image
+      run: |
+        docker build -t datafusion-comet:spark-3.4-scala-2.12-${{ env.COMET_VERSION }} -f kube/Dockerfile .


are we planning to parametrize spark and scala versions later? the same like its done for comet version

good point, I will update that

I think also it might be important here to use docker buildx to create images that are compatible with arm if comet supports it, see https://docs.docker.com/build/ci/github-actions/multi-platform/

Thanks @edmondop I have made this change

comphead

it looks good to me. thanks @andygrove

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

edmondop · 2024-08-19T19:02:25Z

+        docker push ghcr.io/apache/datafusion-comet:$DOCKER_TAG
+      env:
+        DOCKER_USER: ${{ github.actor }}
+        DOCKER_PASS: ${{ secrets.GITHUB_TOKEN }}


Missing newline here

edmondop · 2024-08-19T19:07:03Z

-COPY --from=builder  /opt/datafusion-comet/spark/target/comet-spark-spark${SPARK_VERSION}_$SCALA_VERSION-0.1.0-SNAPSHOT.jar $SPARK_HOME/jars
+
+# ntoe the use of a wildcard in the file name so that this works with both snapshot and final release versions
+COPY --from=builder  /comet/spark/target/comet-spark-spark${SPARK_VERSION}_$SCALA_VERSION-0.2.0*.jar $SPARK_HOME/jars


Newline at EOF

edmondop · 2024-08-19T19:08:48Z

+      run: echo "The current Maven version is ${{ env.COMET_VERSION }}"
+    - name: Build and push Docker image
+      run: |
+        docker build -t datafusion-comet:spark-3.4-scala-2.12-${{ env.COMET_VERSION }} -f kube/Dockerfile .


I think also it might be important here to use docker buildx to create images that are compatible with arm if comet supports it, see https://docs.docker.com/build/ci/github-actions/multi-platform/

Co-authored-by: Edmondo Porcu <edmondo.porcu@gmail.com>

…t into docker-publish

codecov-commenter · 2024-08-19T21:08:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.19%. Comparing base (c9af3f4) to head (3cddf4b).
Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #847      +/-   ##
============================================
+ Coverage     34.05%   34.19%   +0.14%     
- Complexity      879      885       +6     
============================================
  Files           112      112              
  Lines         42959    42950       -9     
  Branches       9488     9486       -2     
============================================
+ Hits          14629    14688      +59     
+ Misses        25330    25268      -62     
+ Partials       3000     2994       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

edmondop

Looks good, can you please correct the newline at the end of kube/Dockerfile before merging?

parthchandra

lgtm

andygrove · 2024-08-19T22:54:11Z

Looks good, can you please correct the newline at the end of kube/Dockerfile before merging?

I'll be following up with another PR almost immediately after merging this, so will include in that

andygrove · 2024-08-19T22:59:04Z

Looks good, can you please correct the newline at the end of kube/Dockerfile before merging?

I'll be following up with another PR almost immediately after merging this, so will include in that

#851

eejbyfeldt · 2024-08-20T06:59:19Z

+# if only Scala code gets modified
+COPY rust-toolchain.toml /comet/rust-toolchain.toml
+COPY native /comet/native
+RUN cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release


Is the -Ctarget-cpu=native a good idea? My guess would have been that github runners might use a range of different generations of cpus and therefore have different features? If that is the case different runs here might for example enable different simd instructions and cause varying performance.

Would it not be better to target some specific set of features and document that? And user what something else they should build from source.

andygrove added 4 commits August 19, 2024 09:50

Add workflow to publish Docker images

85e6e13

update workflow name

2c52741

remove regex check

073d202

improve

342ae84

andygrove requested a review from comphead August 19, 2024 16:23

parthchandra reviewed Aug 19, 2024

View reviewed changes

fix

e7f7c5d

andygrove marked this pull request as draft August 19, 2024 16:41

andygrove added 7 commits August 19, 2024 10:45

use maven to get project version

91203b4

add scalastyle config

f2b0a76

fix

d6a3979

remove java distro name

68be564

add dev folder

ec6d0e5

save progress

005f137

docker build works

19e2ba3

andygrove marked this pull request as ready for review August 19, 2024 17:18

andygrove mentioned this pull request Aug 19, 2024

Comet 0.2.0 Release #843

Closed

5 tasks

comphead reviewed Aug 19, 2024

View reviewed changes

Comment thread kube/Dockerfile Outdated

comphead reviewed Aug 19, 2024

View reviewed changes

comphead approved these changes Aug 19, 2024

View reviewed changes

Update kube/Dockerfile

0565cd6

Co-authored-by: Oleks V <comphead@users.noreply.github.com>

edmondop reviewed Aug 19, 2024

View reviewed changes

andygrove and others added 3 commits August 19, 2024 13:56

Update .github/workflows/docker-publish.yml

51f70a7

Co-authored-by: Edmondo Porcu <edmondo.porcu@gmail.com>

address feedback

70dbd93

Merge branch 'docker-publish' of github.com:andygrove/datafusion-come…

3cddf4b

…t into docker-publish

edmondop reviewed Aug 19, 2024

View reviewed changes

parthchandra approved these changes Aug 19, 2024

View reviewed changes

andygrove merged commit 364887d into apache:main Aug 19, 2024

andygrove deleted the docker-publish branch August 19, 2024 22:54

eejbyfeldt reviewed Aug 20, 2024

View reviewed changes

Conversation

andygrove commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmondop Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmondop Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 19, 2024

Codecov Report

Uh oh!

edmondop left a comment

Choose a reason for hiding this comment

Uh oh!

parthchandra left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Aug 19, 2024

Uh oh!

andygrove commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andygrove commented Aug 19, 2024 •

edited

Loading

edmondop Aug 19, 2024 •

edited

Loading

edmondop Aug 19, 2024 •

edited

Loading