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

updated dockerfiles for grpc builds for powerPC compilation~ #2262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pranavpandit1
Copy link

This PR targets to get the katib/suggestions built on PowerPC.

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pranavpandit1
Once this PR has been reviewed and has the lgtm label, please assign johnugeorge for approval. For more information see the Kubernetes Code Review Process.

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

Signed-off-by: pranav pandit <pranav.pandit1@ibm.com>
@tenzen-y
Copy link
Member

tenzen-y commented Mar 2, 2024

Uhm, I'm wondering if we shouldn't support ppc64le env since the validation environment generally doesn't exist.
@andreyvelich @johnugeorge WDYT?

@lehrig
Copy link

lehrig commented Mar 7, 2024

@tenzen-y - note that this PR is part of a greater effort to enable Kubeflow and all its dependencies for multiple processor architectures (we start with ppc64le): kubeflow/kubeflow#6684

We do have an enterprise-supported ppc64le Kubeflow distribution (https://www.ibm.com/docs/en/announcements/rocket-ai-hub-power) and are ensuring validation / take care of upstream fixing if needed. I hope that helps...

@tenzen-y
Copy link
Member

tenzen-y commented Mar 9, 2024

@tenzen-y - note that this PR is part of a greater effort to enable Kubeflow and all its dependencies for multiple processor architectures (we start with ppc64le): kubeflow/kubeflow#6684

We do have an enterprise-supported ppc64le Kubeflow distribution (https://www.ibm.com/docs/en/announcements/rocket-ai-hub-power) and are ensuring validation / take care of upstream fixing if needed. I hope that helps...

I see. I didn't know it.
@kubeflow/wg-automl-leads Did you approve this ppc64le projects? If so, I'm ok with proceeding with this PR.

Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update CI as well?
For example, we should update this platform

platforms: linux/amd64,linux/arm64

platforms: linux/amd64,linux/arm64

@@ -5,9 +5,12 @@ ENV TARGET_DIR /opt/katib
ENV SUGGESTION_DIR cmd/suggestion/hyperband/v1beta1
ENV PYTHONPATH ${TARGET_DIR}:${TARGET_DIR}/pkg/apis/manager/v1beta1/python:${TARGET_DIR}/pkg/apis/manager/health/python

RUN if [ "${TARGETARCH}" = "ppc64le" ] || [ "${TARGETARCH}" = "arm64" ]; then \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reasons to remove arm64 from here?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No specific reason but I think its typo and we can fix it by nested if-else condition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I couldn't follow it. where is a typo?
Could you clarify it? Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, these packages are required to support arm64, isn't ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We verify grpcio installation only for ppc64le but not for arm64 and while raising the PR, I think arm64 has been mistakenly removed in hyperband dockerfile. So will update dockerfile and add a condition where if it is ppc64le then only install grpcio from source...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We verify grpcio installation only for ppc64le but not for arm64 and while raising the PR, I think arm64 has been mistakenly removed in hyperband dockerfile. So will update dockerfile and add a condition where if it is ppc64le then only install grpcio from source...

Ah, I see. Thanks.

Comment on lines +11 to +13
export REPO_ROOT=grpc && git clone -b v1.60.1 https://github.com/grpc/grpc $REPO_ROOT && \
cd $REPO_ROOT && git submodule update --init && \
pip install -r requirements.txt && GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1 pip install . && \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any other approach using a package manager like apt?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm checking alternate approach for grpcio installation..once done will add it here.Thanks

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tenzen-y Yes we can install grpcio by setting flag GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1 to pip install and have verified all the dockerfiles with the same changes. I will update the PR.

@aditijadhav38
Copy link

Could you update CI as well? For example, we should update this platform

platforms: linux/amd64,linux/arm64

platforms: linux/amd64,linux/arm64

@tenzen-y Actually we are adding ppc64le support for other Katib components as well which is in progress. So once we are in good position where all katib components's dockerfile have ppc64le support then we can proceed with updating CI.. WDYT?

@tenzen-y
Copy link
Member

tenzen-y commented Mar 13, 2024

Could you update CI as well? For example, we should update this platform

platforms: linux/amd64,linux/arm64

platforms: linux/amd64,linux/arm64

@tenzen-y Actually we are adding ppc64le support for other Katib components as well which is in progress. So once we are in good position where all katib components's dockerfile have ppc64le support then we can proceed with updating CI.. WDYT?

We shouldn't add any images without verifying in CI. It will cause us to lose maintainability and stability.
I didn't mean that "I can not believe you", and my primary concern is the future maintainability.

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for starting to add support powerPC builds in Katib! A few questions.

@@ -5,9 +5,12 @@ ENV TARGET_DIR /opt/katib
ENV SUGGESTION_DIR cmd/suggestion/hyperband/v1beta1
ENV PYTHONPATH ${TARGET_DIR}:${TARGET_DIR}/pkg/apis/manager/v1beta1/python:${TARGET_DIR}/pkg/apis/manager/health/python

RUN if [ "${TARGETARCH}" = "ppc64le" ] || [ "${TARGETARCH}" = "arm64" ]; then \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, these packages are required to support arm64, isn't ?

@andreyvelich
Copy link
Member

cc @kubeflow/wg-training-leads

@lehrig
Copy link

lehrig commented Mar 14, 2024

@andreyvelich, I have directly added a comment there: argoproj/argo-workflows#12449 (comment)

What I describe there also holds true here - we are happy to help maintaining, especially if anything architecture-specific comes up. Hope that helps.

apt-get -y install gfortran libopenblas-dev liblapack-dev libssl-dev gcc g++ pkg-config git && \
export REPO_ROOT=grpc && git clone -b v1.60.1 https://github.com/grpc/grpc $REPO_ROOT && \
cd $REPO_ROOT && git submodule update --init && \
pip install -r requirements.txt && GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1 pip install . && \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a git clone here, you can just set this flag GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1 down in the pip install block to ensure pip source building doesn't pick up the BoringSSL library.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkumatag Thanks for your response and yes I was able to verify the grpc installation by setting the flag GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1. I will update PR with the changes.

@aditijadhav38
Copy link

@tenzen-y I have raised new PR #2290 with the suggested changes and I've also updated one more dockerfile earlystopping/medianstop which needs similar changes for grpc installation.
Also as per your suggestion I have updated CI files as well. But please note that we haven't added support for all Katib components, hence for few images (for which dockerfiles are not yet updated for eg. nas/enas, katib-ui, katib/tfevent-metricscollector) the build might fail. We are currently working on remaining components mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants