Skip to content
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

Merged
merged 1 commit into from
Nov 8, 2019
Merged

Add arm64 support for ml-pipeline #2507

merged 1 commit into from
Nov 8, 2019

Conversation

MrXinWang
Copy link
Contributor

@MrXinWang MrXinWang commented Oct 29, 2019

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 Reviewable

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

backend/Dockerfile Outdated Show resolved Hide resolved
@Bobgy
Copy link
Contributor

Bobgy commented Oct 31, 2019

/ok-to-test

@IronPan @neuromage for reviewing backend dockerfile.

@@ -1,4 +1,40 @@
FROM l.gcr.io/google/bazel:0.24.0 as builder
FROM ubuntu:18.04 as builder
Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Contributor

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.

Copy link
Contributor Author

@MrXinWang MrXinWang Nov 1, 2019

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.

Copy link
Contributor

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
...

Copy link
Contributor Author

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.

@MrXinWang
Copy link
Contributor Author

/retest

@MrXinWang
Copy link
Contributor Author

/retest

@IronPan
Copy link
Member

IronPan commented Nov 4, 2019

/lgtm

@Bobgy
Copy link
Contributor

Bobgy commented Nov 5, 2019

/approve
for frontend image change

Change-Id: Ib8cbcf3fd75e9ddb126bb54a97f15367b8bcb49b
Signed-off-by: Henry Wang <henry.wang@arm.com>
@MrXinWang
Copy link
Contributor Author

MrXinWang commented Nov 7, 2019

Followed by suggestions from @Ark-kun , changed the way to include bazel base image on multi-arch machines for backend and updated documents. @Bobgy @IronPan Please review again, thanks very much!

--build-arg google_application_credentials="${GCP_CREDENTIALS}"
if [ $MACHINE_ARCH == "aarch64" ]; then
docker build \
-t bazel:0.24.0 \
Copy link
Contributor

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)?

Copy link
Contributor Author

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.

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 7, 2019

/lgtm

@IronPan
Copy link
Member

IronPan commented Nov 7, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Ark-kun
Copy link
Contributor

Ark-kun commented Nov 7, 2019

/retest

@MrXinWang
Copy link
Contributor Author

@neuromage Could you please kindly review this updated PR? Thank you very much!

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.

None yet

5 participants