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.

Sign up for GitHub

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

Jump to bottom

build: fix workflow details making it compatible on s390x #602

Merged
merged 2 commits into from
Dec 2, 2024
Merged

build: fix workflow details making it compatible on s390x #602

merged 2 commits into from
Dec 2, 2024

Conversation

modassarrana89
Copy link
Contributor

Multi-architecture support for model-registry

Changes Updated

  • Updated Makefile for timeout & Image version which will be required later for github workflow build
  • Updated install_protoc scripts for multi-architecture

Reference to OLD PR that has been closed :- #66

@modassarrana89
Copy link
Contributor Author

@tarilabs As discussed these are the few changes which were missing as part of multi-arch. I have tested it locally on my environment on s390x .

tarilabs
tarilabs reviewed Dec 2, 2024
View reviewed changes
Copy link
Member

@tarilabs tarilabs 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 very much @modassarrana89 for this contribution!

In general,
/lgtm

my only remaning question, as previously stated, is how can the KF community test this.

Is there any emulation which can be done on GHA side, as far as you know?
Some runner which can be used.

Noted, I think it's good anyway to produce multiple images, but specifically I can only see ways of testing x86 and Arm, so far.

Kindly let us know!

Makefile
$(DOCKER) buildx build --push --platform=$(PLATFORMS) --tag ${IMG} -f ${DOCKERFILE} .
$(DOCKER) buildx build --push --platform=$(PLATFORMS) --tag ${IMG}:$(IMG_VERSION) -f ${DOCKERFILE} .
Copy link
Member

Choose a reason for hiding this comment

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

Makes it consistent with

model-registry/Makefile

Lines 241 to 243 in 4099d46

.PHONY: image/build
image/build:
${DOCKER} build ${BUILD_PATH} -f ${DOCKERFILE} -t ${IMG}:$(IMG_VERSION)

lgtm

Makefile
$(DOCKER) buildx build --platform=$(PLATFORMS) --manifest ${IMG} -f ${DOCKERFILE} .
$(DOCKER) buildx build --platform=$(PLATFORMS) --manifest ${IMG}:$(IMG_VERSION) -f ${DOCKERFILE} .
Copy link
Member

Choose a reason for hiding this comment

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

as above.

Makefile
Comment on lines +206 to +207
${GOLANGCI_LINT} run main.go --timeout 3m
${GOLANGCI_LINT} run cmd/... internal/... ./pkg/... --timeout 3m
Copy link
Member

Choose a reason for hiding this comment

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

not sure why this was needed, but okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In s390x environment , it was failing without timeout . By giving timeout option of 3 min , it waits for the operation to complete during the interval & is working fine post that

scripts/install_protoc.sh
Comment on lines -10 to +16
ARCH="x86_64"
if [[ $(uname -m) =~ arm ]]; then
ARCH="aarch_64"
ARCH=$(uname -m)
if [[ "$ARCH" == "arm"* ]]; then
ARCH="aarch_64"
elif [[ "$ARCH" == "s390x" ]]; then
ARCH="s390_64"
elif [[ "$ARCH" == "ppc64le" ]] ; then
ARCH="ppcle_64"
Copy link
Member

Choose a reason for hiding this comment

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

tested locally

image

lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Dec 2, 2024
@modassarrana89
Copy link
Contributor Author

thank you very much @modassarrana89 for this contribution!

In general, /lgtm

my only remaning question, as previously stated, is how can the KF community test this.

Is there any emulation which can be done on GHA side, as far as you know? Some runner which can be used.

Noted, I think it's good anyway to produce multiple images, but specifically I can only see ways of testing x86 and Arm, so far.

Kindly let us know!

Already working on updating the github workflow for all architecture. Either i can add the changes as part of this PR or i can raise separate PR . Please suggest.

@tarilabs
Copy link
Member

tarilabs commented Dec 2, 2024

Either i can add the changes as part of this PR or i can raise separate PR . Please suggest.

I would do separately. Please allow some times for others to chime-in if they wanted on this PR.

@tarilabs
Copy link
Member

tarilabs commented Dec 2, 2024

btw, @modassarrana89 kindly provide DCO.

For more information:

otherwise this won't be merged eventually by the bot.

@google-oss-prow google-oss-prow bot removed the lgtm label Dec 2, 2024
@modassarrana89
Copy link
Contributor Author

btw, @modassarrana89 kindly provide DCO.

For more information:

otherwise this won't be merged eventually by the bot.

Done

@modassarrana89
Copy link
Contributor Author

Either i can add the changes as part of this PR or i can raise separate PR . Please suggest.

I would do separately. Please allow some times for others to chime-in if they wanted on this PR.

ok. I also wanted to handle it separately.

@tarilabs tarilabs changed the title Multi arch build: fix workflow details making it compatible on s390x Dec 2, 2024
dhirajsb
dhirajsb approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@dhirajsb dhirajsb left a comment

Choose a reason for hiding this comment

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

/lgtm

@google-oss-prow Google OSS Prow
Copy link

@dhirajsb: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@modassarrana89
Copy link
Contributor Author

@zijianjoy @tarilabs Please approve

tarilabs
tarilabs approved these changes Dec 2, 2024
View reviewed changes
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

per previous comments

@google-oss-prow google-oss-prow bot added the lgtm label Dec 2, 2024
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhirajsb, tarilabs

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

@google-oss-prow google-oss-prow bot merged commit d816da7 into kubeflow:main Dec 2, 2024
3 checks passed
@tarilabs tarilabs mentioned this pull request Dec 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhirajsb dhirajsb dhirajsb approved these changes

@tarilabs tarilabs tarilabs approved these changes

@andreyvelich andreyvelich Awaiting requested review from andreyvelich

@zijianjoy zijianjoy Awaiting requested review from zijianjoy

Assignees

@tarilabs tarilabs

Labels
approved lgtm size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@modassarrana89 @tarilabs @dhirajsb