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 workflow for Linux Arm64 #767

Merged
merged 30 commits into from
Jun 18, 2024
Merged

Add workflow for Linux Arm64 #767

merged 30 commits into from
Jun 18, 2024

Conversation

rauletorresc
Copy link
Contributor

@rauletorresc rauletorresc commented May 27, 2024

Context: GitHub action must be available that automatically builds, on a schedule, Linux Arm64 wheels of Catalyst

Description of the Change: It builds and tests Catalyst AARCH64 Linux wheels inside a manylinux_aarch64 docker image on the macOS M2 runner

Benefits: Support for a new Linux Arm64

[sc-60597]

@rauletorresc rauletorresc self-assigned this May 27, 2024
@rauletorresc rauletorresc added the author:build-wheels Run the wheel building workflows on this Pull Request label May 27, 2024
@rauletorresc rauletorresc force-pushed the linux_arm64_wheels_workflow branch 27 times, most recently from 27b348e to 9455cf7 Compare May 31, 2024 00:48
@rauletorresc rauletorresc force-pushed the linux_arm64_wheels_workflow branch 2 times, most recently from a4d91b4 to 5b9372d Compare June 15, 2024 21:07
@rauletorresc rauletorresc requested a review from dime10 June 17, 2024 03:29
@rauletorresc rauletorresc added this to the v0.7.0 milestone Jun 17, 2024
@sergei-mironov
Copy link
Contributor

sergei-mironov commented Jun 17, 2024

Hi @rauletorresc , the code looks much better, thanks! One minor concern that I have is about the folder naming: you put the scripts into the linux_arm64 folder but the scripts use dnf package manager which is specific to RedHat Linux distro family. Same for /opt/rh/* paths. Should we narrow down the folder name to capture the dependency on the RedHat infrastructure?

@rauletorresc
Copy link
Contributor Author

Hi @rauletorresc , the code looks much better, thanks! One minor concern that I have is about the folder naming: you put the scripts into the linux_arm64 folder but the scripts use dnf package manager which is specific to RedHat Linux distro family. Same for /opt/rh/* paths. Should we narrow down the folder name to capture the dependency on the RedHat infrastructure?

This is because manylinux_2_28 uses AlmaLinux. Maybe renaming the folder to 'manylinux_2_28_aarch64' would reflect those dependencies better without being too specific?

@mlxd
Copy link
Member

mlxd commented Jun 17, 2024

Hi @rauletorresc , the code looks much better, thanks! One minor concern that I have is about the folder naming: you put the scripts into the linux_arm64 folder but the scripts use dnf package manager which is specific to RedHat Linux distro family. Same for /opt/rh/* paths. Should we narrow down the folder name to capture the dependency on the RedHat infrastructure?

This is because manylinux_2_28 uses AlmaLinux. Maybe renaming the folder to 'manylinux_2_28_aarch64' would reflect those dependencies better without being too specific?

Since these are CI builder specific, and manylinux_2_x is the standard builder for Python wheels, I think it's probably fine to leave as is. If we want to be specific linux_arm64/rh8/build_catalyst.sh could be the path, and should imply compatibility with RedHat8 (incl. AlmaLinux8/manylinux_2_28 and Rocky Linux 8). Though, I'd say this only matters if the goal is to provide bash scripts to end users outside the CI env (i.e. not in the .github folder, but part of the package source scripts).

Though, I'll leave the decision to you both on what to do here.

@sergei-mironov
Copy link
Contributor

Hi @rauletorresc , the code looks much better, thanks! One minor concern that I have is about the folder naming: you put the scripts into the linux_arm64 folder but the scripts use dnf package manager which is specific to RedHat Linux distro family. Same for /opt/rh/* paths. Should we narrow down the folder name to capture the dependency on the RedHat infrastructure?

This is because manylinux_2_28 uses AlmaLinux. Maybe renaming the folder to 'manylinux_2_28_aarch64' would reflect those dependencies better without being too specific?

Since these are CI builder specific, and manylinux_2_x is the standard builder for Python wheels, I think it's probably fine to leave as is. If we want to be specific linux_arm64/rh8/build_catalyst.sh could be the path, and should imply compatibility with RedHat8 (incl. AlmaLinux8/manylinux_2_28 and Rocky Linux 8). Though, I'd say this only matters if the goal is to provide bash scripts to end users outside the CI env (i.e. not in the .github folder, but part of the package source scripts).

Though, I'll leave the decision to you both on what to do here.

Leaving it without change would confuse other linux' users I think.

linux_arm64/rh8/build_catalyst.sh looks good, lets go with it!

@rauletorresc
Copy link
Contributor Author

Hi @rauletorresc , the code looks much better, thanks! One minor concern that I have is about the folder naming: you put the scripts into the linux_arm64 folder but the scripts use dnf package manager which is specific to RedHat Linux distro family. Same for /opt/rh/* paths. Should we narrow down the folder name to capture the dependency on the RedHat infrastructure?

This is because manylinux_2_28 uses AlmaLinux. Maybe renaming the folder to 'manylinux_2_28_aarch64' would reflect those dependencies better without being too specific?

Since these are CI builder specific, and manylinux_2_x is the standard builder for Python wheels, I think it's probably fine to leave as is. If we want to be specific linux_arm64/rh8/build_catalyst.sh could be the path, and should imply compatibility with RedHat8 (incl. AlmaLinux8/manylinux_2_28 and Rocky Linux 8). Though, I'd say this only matters if the goal is to provide bash scripts to end users outside the CI env (i.e. not in the .github folder, but part of the package source scripts).
Though, I'll leave the decision to you both on what to do here.

Leaving it without change would confuse other linux' users I think.

linux_arm64/rh8/build_catalyst.sh looks good, lets go with it!

Done!

@rauletorresc rauletorresc merged commit 422c609 into main Jun 18, 2024
70 checks passed
@rauletorresc rauletorresc deleted the linux_arm64_wheels_workflow branch June 18, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author:build-wheels Run the wheel building workflows on this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants