-
Notifications
You must be signed in to change notification settings - Fork 8
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
[DPE-4628] Add image with spark-rapids support #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work! (the number of Spark configurations required for GPU are 🤯) I have left some trivial comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files being repeated looks a bit off to my eyes. Is there a way we could reuse these files that already exist from the spark base image directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I agree with Bikalpa, I would not copy and paste, seems repetition we'd better avoid. Maybe you can also symlink the folder in the worst case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, but in general I would like to reduce the copy and paste parts. Why not just keeping a delta dockerfile only, such that we use the main rockcraft.yaml
(that already has all the files and layout) and then use the Dockerfile to only custom two things:
- GPU
- Java versions to be used (JDK8)
Are there other things that we need in order to have a dedicated rockcraft.yaml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I agree with Bikalpa, I would not copy and paste, seems repetition we'd better avoid. Maybe you can also symlink the folder in the worst case
ARG BASE_IMAGE=staged-charmed-spark:latest | ||
FROM $BASE_IMAGE | ||
# Provide Default Entrypoint for Pebble | ||
ENTRYPOINT [ "/bin/pebble", "enter", "--verbose", "--args", "sparkd" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor I believe that now rockcraft should have the primary service concept, and we could drop the entrypoint override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to proceed and address the re-use of base image in a subsequent PR once we figure out the JDK issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's proceed with merging this and then start working on the new java version.
No description provided.