Skip to content

ENH: Introduce manylinux option with ARM #17

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
Merged
tbirdso merged 4 commits into from
Jan 5, 2023
Merged

ENH: Introduce manylinux option with ARM #17

tbirdso merged 4 commits into from
Jan 5, 2023

Conversation

@tbirdso
Copy link
Collaborator

@tbirdso tbirdso commented Dec 1, 2022 •
edited
Loading

Exposes input parameter to allow the user to build for multiple Linux
toolsets and target architectures via available manylinux docker images.

Depends on ARM support introduced in ITKPythonPackage:
InsightSoftwareConsortium/ITKPythonPackage#236

@tbirdso tbirdso mentioned this pull request Dec 1, 2022
Closed
@tbirdso tbirdso force-pushed the manylinux-options branch 3 times, most recently from 8047b02 to bf42032 Compare December 1, 2022 22:57
thewtex
thewtex reviewed Dec 5, 2022
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Nice!

@tbirdso tbirdso force-pushed the manylinux-options branch 4 times, most recently from 63d3ec8 to f4a50bf Compare December 20, 2022 01:35
@tbirdso tbirdso changed the title WIP: Introduce manylinux option with ARM ENH: Introduce manylinux option with ARM Dec 20, 2022
@tbirdso tbirdso requested a review from dzenanz December 20, 2022 01:54
@tbirdso tbirdso force-pushed the manylinux-options branch from 3b071e5 to 8fc3d42 Compare January 2, 2023 16:57
tbirdso added 2 commits January 4, 2023 13:47
@tbirdso
ENH: Introduce manylinux option with ARM
7ad0d64 
Exposes input parameter to allow the user to build for multiple Linux
toolsets and target architectures via available manylinux docker images.

Depends on ARM support introduced in ITKPythonPackage:
InsightSoftwareConsortium/ITKPythonPackage#236
@tbirdso
DOC: Update README with manylinux input parameter
7d53ecd
@tbirdso tbirdso force-pushed the manylinux-options branch from 4c2e1f7 to 57e0bba Compare January 4, 2023 18:47
@tbirdso
Copy link
Collaborator Author

tbirdso commented Jan 4, 2023

Updated with respect to #46 such that jobs are dynamically scheduled based on requested platforms. Linux jobs now produce one wheel per job.

Advantages of this approach:

  • Fail-fast principle: initial wheel build can succeed more quickly. In ITKSplitComponents the first Linux job to run is (Python 3.7, _2_28, x64) and completes in approximately 5 minutes, whereas under the status quo running builds for _2_28-x64 + 2014-x64 + _2_28-aarch64 all in one job took approximately 30 minutes.
  • It is obvious when one platform is failing, i.e. can tell at a glance if x64 builds pass but ARM fails
  • Build outputs are cached more easily, i.e. if a URL request fails then it is less likely that earlier builds will have to be re-run.

Disadvantage of this approach:

  • Easy to get the JSON input formatting wrong. Mitigated with README documentation and default parameter for reference. Also, the list of target platforms is not expected to be very long.
  • Increases boilerplate, but not by very much. Linux job setup is typically quick and then each wheel build uses its own toolset and tarball, so very little was being reused between wheel builds for a given Python version under the status quo.

Testing at https://github.com/tbirdso/ITKSplitComponents/actions/runs/3840735170/jobs/6540141356 .

@dzenanz
Copy link
Member

dzenanz commented Jan 4, 2023 •
edited
Loading

Consider shortening the names from build-{OS}-python-packages-{furtherSpecs} to {OS}-packages-{furtherSpecs} or {OS}-py-{furtherSpecs}. The names overflow the horizontal space on the left panel, and the important parts are cut off.

@tbirdso
Copy link
Collaborator Author

tbirdso commented Jan 4, 2023 •
edited
Loading

@dzenanz Good call. I have shortened to build-linux-py to preserve the "{verb}-{noun}" format for job titles.

EDIT: Much better: https://github.com/tbirdso/ITKSplitComponents/actions/runs/3840941792

@tbirdso tbirdso force-pushed the manylinux-options branch from ec7be54 to bdc5501 Compare January 4, 2023 19:20
@tbirdso tbirdso marked this pull request as ready for review January 4, 2023 19:47
@tbirdso tbirdso mentioned this pull request Jan 5, 2023
Merged
@tbirdso
Copy link
Collaborator Author

tbirdso commented Jan 5, 2023

@thewtex @dzenanz ping for re-review

dzenanz
dzenanz approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

I would prefer shortening Windows job names too, but this is good as is.

This was linked to issues Jan 5, 2023
Investigate splitting parameterized Linux jobs #46
Closed
Expose ARM build parameter #14
Closed
thewtex
thewtex approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Minor comment inline

tbirdso added 2 commits January 5, 2023 12:07
@tbirdso
ENH: Launch Linux jobs via manylinux-platform matrix
05ba990 
Refactors job scheduling in Github Actions so that each Linux job builds
exactly one Python wheel. The `manylinux-platforms` input parameter
is parsed as a JSON array in order to dynamically form a matrix for
scheduling.

Scheduling one manylinux platform and target architecture per job
improves granularity of results and improves issue traceability. Minimal
impact to overhead is observed as very little boilerplate was previously
reused between wheel build procedures with different targets.

Closes #46
@tbirdso
COMP: Shorten Linux job names
68f3ead 
Avoids cutting off matrix information in job entries on Github Actions
sidebar.
@tbirdso tbirdso force-pushed the manylinux-options branch from bdc5501 to 68f3ead Compare January 5, 2023 17:08
@tbirdso
Copy link
Collaborator Author

tbirdso commented Jan 5, 2023

Force-pushed documentation updates.

I would prefer shortening Windows job names too, but this is good as is.

Given that this is stable and tests have passed I will move forward as-is, but I have no issue with shortening other job names in the future.

@tbirdso tbirdso merged commit 55b88c4 into main Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thewtex thewtex thewtex approved these changes

@dzenanz dzenanz dzenanz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate splitting parameterized Linux jobs Expose ARM build parameter

4 participants

@tbirdso @dzenanz @thewtex