Skip to content

MNT Parallel platforms for cibuildwheel [cd build] #18818

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

Merged
merged 11 commits into from
Nov 12, 2020

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Nov 12, 2020

The 32bit + 64bit sequential builds for Linux and Windows lasts for more than 40 min. The goal of this PR is to try to parallelize them by adjusting the github actions build matrix.

@ogrisel
Copy link
Member Author

ogrisel commented Nov 12, 2020

This is really weird, when I define the 3 level build matrix, the matrix.python variable is undefined when matrix.bitness is 32 while it is define correctly when matrix.bitness is 64...

Copy link
Member

@alfaro96 alfaro96 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 @ogrisel for your PR!

LGTM.

This kind of PRs make me feel really happy. Let us wait for green in the Wheel builder workflow, but looks nice.

@@ -36,7 +36,7 @@ jobs:

# Build the wheels for Linux, Windows and macOS for Python 3.6 and newer
build_wheels:
name: Build wheels on ${{ matrix.os }} for Python ${{ matrix.python }}
name: Build wheels cp${{ matrix.python }}-${{ matrix.platform_id }}
Copy link
Member

@alfaro96 alfaro96 Nov 12, 2020

Choose a reason for hiding this comment

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

I really like this naming.

@alfaro96
Copy link
Member

This is really weird, when I define the 3 level build matrix, the matrix.python variable is undefined when matrix.bitness is 32 while it is define correctly when matrix.bitness is 64...

It is working now, is it not?

@alfaro96
Copy link
Member

Approximately, this should reduce the build process to 20 minutes, right?

@ogrisel
Copy link
Member Author

ogrisel commented Nov 12, 2020

It is working now, is it not?

Yes I found my typo.

@ogrisel
Copy link
Member Author

ogrisel commented Nov 12, 2020

Approximately, this should reduce the build process to 20 minutes, right?

I would say 25 min.

@alfaro96
Copy link
Member

alfaro96 commented Nov 12, 2020

Two (minor) questions @ogrisel:

Thank you!

@ogrisel
Copy link
Member Author

ogrisel commented Nov 12, 2020

Should #18807 speed-up the tests?

I am not sure it works as intended. I will debug it further. But I would not expect a big speed-up. It's more a protection if github actions workers are reconfigured to run on a 72 cores host with only 2 cores available via docker for instance.

Now that we have even more Wheel builder jobs, should we have a look to #18774 (comment) (#18801)?

That would be great indeed.

Approximately, this should reduce the build process to 20 minutes, right?
I would say 25 min.

23 completed jobs in 28m 14s => nice! (instead of 50min for the total workflow previously).

@ogrisel ogrisel merged commit 1e69823 into scikit-learn:master Nov 12, 2020
@ogrisel ogrisel deleted the cibuildwheel-parallel-platform branch November 12, 2020 13:03
@ogrisel
Copy link
Member Author

ogrisel commented Nov 12, 2020

Merged :)

I will rebase #18807 on top.

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

Successfully merging this pull request may close these issues.

2 participants