Fix container images building on arm64 #3839
Conversation
|
This change is |
|
/assign @jingzhang36 @IronPan |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jingzhang36 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| # As some of the AArch64 dependancies are installed from source, | ||
| # running pip freeze will give additional package information after | ||
| # an " @", we need to remove that part. | ||
| # The license checking on some additional installed packages on AArch64 should also be skipped. |
There was a problem hiding this comment.
Are you sure the license checking should be skipped for those packages? Maybe just add the licenses to the list.
There was a problem hiding this comment.
Hi @Ark-kun , thanks for your comment. Well, there are two cases for these skipped packages:
-
As some of the dependancies in the arm64 docker image are built from source, they need
Cythonto build. However I checked the x86_64 image,Cythonis not installed in that image. ThereforeCythonis skipped. -
The arm64 base image proposed in this PR does not contain some of the required packages, and I have to install them manually by pip from Dockerfile.visualization#L43. This will lead to some additional packages
cffi,jeepney,pycairo,pycparseralso installed. As x86_64 image also does not have them, these four packages should be skipped.
I think if I add these packages to the license list, the x86_64 building will be broken. So I chose to only skip them for arm64.
There was a problem hiding this comment.
I think if I add these packages to the license list, the x86_64 building will be broken.
That should not be the case. The license script just uses the list to get URLs for the package licenses and downloads licenses for all packages returned by pip freeze.
There was a problem hiding this comment.
Hi @Ark-kun, thanks for your patience.
Yes and no. The license.sh does work as you described. However these 4 packages are only installed in the arm64 image. If their licenses are added to the .csv file. The license.sh will also check the license of these 4 packages on x86_64, and will ask us to install them in x86_64 images, which is unnecessary and break the docker build on x86_64 as shown in license.sh#L84.
There was a problem hiding this comment.
Oh. Sorry for my misconception. The license.sh we use for components does not have that part. https://github.com/kubeflow/pipelines/blob/ef73aab59f8fc06db19d5e6f0ca35337e98bcf77/components/license.sh
I think we should remove exit 1 from that script. You can do that.
There was a problem hiding this comment.
Hi @Ark-kun! Thanks, yes that is a solution, however I am worried a little bit about this removing the exit 1....as I personally think it is a little bit too aggressive. Personally I would like to achieve the goal that we can completely encapsulate the arm64 stuff without affecting the x86_64, before we officially support arm64 in kubeflow.
Also I did a quick test about adding the licenses in the .csv file, and it seems if I add them to the csv, I have to run the license-download.sh and also commit these newly downloaded licenses in this PR....Would that be also ok?
If the community is ok with these changes, I am also happy to update this PR with related license changes.
Thanks again for your patience.
There was a problem hiding this comment.
I am worried a little bit about this removing the exit 1....as I personally think it is a little bit too aggressive.
I think that the script should fail when there is a missing license. But an extra license is fine in my opinion.
Also I did a quick test about adding the licenses in the .csv file, and it seems if I add them to the csv, I have to run the license-download.sh and also commit these newly downloaded licenses in this PR....Would that be also ok?
I think this would be OK.
I consider it more readable and safe than hacking the license.sh script.
There was a problem hiding this comment.
Thanks @Ark-kun. That would be awesome. I have updated related files in this PR and added everything you suggested. Would you mind taking a look again? Thanks!
There was a problem hiding this comment.
It will be harder to remove obsolete license files, because we don't know which packages are not needed in x86 but needed in arm, even though the message exists.
Obsolete licenses is not a big issue I agree, just not ideal.
|
New changes are detected. LGTM label has been removed. |
|
/retest |
This commit fixes the container images building on arm64, including: 1. Enabled cache server, visualization server and metadata_envoy containers building on arm64. 2. Fixed broken api-server build on arm64 by adding an Dockerfile for arm64 as base image. This image will contain some dependancies, including tensorflow, pyarrow, tfx-bsl, tensorflow-data-validation, and ml-metadata, which cannot be directly installed by pip. 3. Updated doc and building script. Signed-off-by: Henry Wang <[email protected]>
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
the
lifecycle/stale
|
This issue has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it. |
This commit fixes the container images building on arm64, including:
Enabled cache server, visualization server and metadata_envoy containers building on arm64.
Fixed broken api-server build on arm64 by adding a Dockerfile for arm64 as base image. This image will contain some dependancies (tensorflow, pyarrow, tfx-bsl, tensorflow-data-validation, and ml-metadata) which cannot be directly installed by pip on arm.
Updated doc and the building script.
Signed-off-by: Henry Wang [email protected]