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

AZP/RELEASE: Add ARM release - Part #2 #8895

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

Alexey-Rivkin
Copy link
Collaborator

What
Build and release ARM-based artifacts.
Part 2: Build DEB/RPM artifacts on ARM.

Why ?
#8762

@Alexey-Rivkin Alexey-Rivkin force-pushed the topic/arm_release-part2 branch 3 times, most recently from 0c501ef to ccfef0f Compare February 21, 2023 12:54
@Alexey-Rivkin Alexey-Rivkin added the WIP-DNM Work in progress / Do not review label Mar 23, 2023
@Alexey-Rivkin Alexey-Rivkin force-pushed the topic/arm_release-part2 branch 4 times, most recently from 8dbb5b1 to 3214713 Compare April 9, 2023 14:24
@Alexey-Rivkin Alexey-Rivkin removed the WIP-DNM Work in progress / Do not review label Apr 10, 2023
artemry-nv
artemry-nv previously approved these changes Apr 11, 2023
@Alexey-Rivkin
Copy link
Collaborator Author

@yosefe please, review

buildlib/tools/common.sh Outdated Show resolved Hide resolved
buildlib/dockers/docker-compose-aarch64.yml Show resolved Hide resolved
buildlib/az-GitHubDraft.yml Outdated Show resolved Hide resolved
Comment on lines 20 to 21
build_container: centos7_cuda11_x86_64
artifact_name: $(POSTFIX)-centos7-mofed5-cuda11-x86_64.tar.bz2
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do something like

build_container: centos7_cuda11_${{ parameters.name }}
artifact_name: $(POSTFIX)-centos7-mofed5-cuda11-${{ parameters.name }}.tar.bz2

in order to avoid ${{ if eq(parameters.name, 'x86_64') }}:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it will cause duplicate key names:

build_container: centos7_cuda11_${{ parameters.name }}   # x86_64
build_container: centos7_cuda11_${{ parameters.name }}   # aarch64

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will fail on Azure YAML schema validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

why? container name cannot be parametrized?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

image

centos8_cuda11_${{ parameters.name }}: is really listed twice in the file.
Can you try to remove this duplication?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, those dups might be removed. Yet, we have a different set of containers per OS architecture.
We can either reduce the support matrix or separate container lists by if.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe first list all common containers, and then list specific containers under "if" condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will work!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed by the latest commit + changed the key name to better reflect expected values.

- job: distro_release
displayName: distro
- job: distro_release_${{ parameters.arch }}
displayName: Distro ${{ parameters.arch }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the job title appears as Distro aarch64 ubuntu20_cuda11_aarch64
Can we rename it to just "ubuntu20_cuda11_aarch64" or similar?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image
Close enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe set job name to "Build" or "Distro" - instead of parameters.arch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Much better indeed

image

@Alexey-Rivkin Alexey-Rivkin force-pushed the topic/arm_release-part2 branch 6 times, most recently from c3d7d04 to f8c7102 Compare April 17, 2023 09:15
yosefe
yosefe previously approved these changes Apr 17, 2023
Copy link
Contributor

@yosefe yosefe left a comment

Choose a reason for hiding this comment

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

pls sqaush

@Alexey-Rivkin
Copy link
Collaborator Author

squashed

@yosefe yosefe enabled auto-merge April 17, 2023 16:57
@yosefe yosefe merged commit c24f807 into openucx:master Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants