-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add arm64 support for ml-pipeline #2507
Conversation
Hi @MrXinWang. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test @IronPan @neuromage for reviewing backend dockerfile. |
backend/Dockerfile
Outdated
@@ -1,4 +1,40 @@ | |||
FROM l.gcr.io/google/bazel:0.24.0 as builder | |||
FROM ubuntu:18.04 as builder |
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.
Can you please split out this part into another Dcokerfile that creates and image that's analogous to l.gcr.io/google/bazel
, but works on ARM64?
Maybe that Dockerfile can later be added to the Bazel project.
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.
Thanks for your comment, and I think yes, however can you please be more specific? Shall I upload another Dockerfile named "Dockerfile.aarch64" to build the backend and leave the existing Dockerfile for backend as something like "Dockerfile.x86_64", or just split the bazel part as "Dockerfile.bazel"?
Thanks!
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 would be great if you create Dockerfile.bazel
.
Maybe you can then select the base image based on Docker build argument or some other mechanism.
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.
Hi @Ark-kun ! Thanks for your timely response.
Referencing comment#1 and comment#2 in moby#3378, I don't think we have a official way to achieve this for now, unless google releases the multiarch version of bazel image, but I did refine the Dockerfile for pipeline backend to make it a little bit lighter. Since the builder image will not be used as the final image of pipeline backend, I think the method for now would also work fine. The bazel installation commands in the updated backend/Dockerfile
can serve as the Dockerfile.bazel
in the future.
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.
Is it possible to solve this using the following Dockerfile?
ARG BAZEL_IMAGE=l.gcr.io/google/bazel:0.24.0
FROM $BAZEL_IMAGE as builder
...
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.
Ahhh cool @Ark-kun ! Thanks very much for this awesome advice. I have pushed a new commit by following this, please review.
/retest |
/retest |
/lgtm |
/approve |
Change-Id: Ib8cbcf3fd75e9ddb126bb54a97f15367b8bcb49b Signed-off-by: Henry Wang <henry.wang@arm.com>
--build-arg google_application_credentials="${GCP_CREDENTIALS}" | ||
if [ $MACHINE_ARCH == "aarch64" ]; then | ||
docker build \ | ||
-t bazel:0.24.0 \ |
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.
Should the tag be bazel:0.24.0-arm64
(or bazel:0.24.0-aarch64
)?
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.
Well.....Personally I think there is nothing in the Dockerfile that is specific to arm64 architecture, and it is just a building and installation of bazel. Referencing PR#2233, if this Dockerfile is compiled on ppc64le, then it will be bazel:0.24.0-ppc64le
, and thus I didn't specific machine architecture.
However if you want its name to be bazel:0.24.0-arm64
, I can also change. :)
Also, I think just in terms of file's name, arm64
is the same as aarch64
. On arm64 machines, arm64
can be printed by shell command dpkg --print-architecture
and aarch64
can be printed by uname -m
.
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy, IronPan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
@neuromage Could you please kindly review this updated PR? Thank you very much! |
Referencing comment and issue #2337
We are working on Kubeflow supporting arm64 platform. We now rebuilt some images and installed Kubeflow on arm64, and hence contribute the Dockerfile.
Tested on both arm64 machine and x86_64 machine
Signed-off-by: Henry Wang <henry.wang@arm.com>
This change is