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 support for m1 wheel builds in CI. #15738

Merged
merged 1 commit into from
Jun 5, 2022
Merged

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Jun 3, 2022

Since we only have one self-hosted M1 runner, and we don't want it to become a bottleneck,
we don't (yet?) run tests on M1, just bootstrap and build wheels.

Refactors generate_github_workflows.py to break out the reusable bootstrapping steps.

This involves creating a Helper class that knows about the target platform and can
affect generation appropriately.

Various cache keys that were previously only qualfied by os are now qualified
by platform (os+arch).

This refactoring also regularizes some of the previously haphazard environment settings,
log uploading and smoke test running.

It also moves the CI config validation after the bootstrapping, previously they
were intermingled.

Also removes a superfluous invocation of the label check steps inside an unrelated job
which already depends on the "proper" label check job.

@benjyw benjyw force-pushed the m1_ci branch 16 times, most recently from 8d200c2 to c4c4881 Compare June 4, 2022 23:22
@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Jun 5, 2022
@benjyw benjyw changed the title WIP Add support for m1 wheel builds in CI. Jun 5, 2022
@benjyw
Copy link
Contributor Author

benjyw commented Jun 5, 2022

Reviewers: I recommend reviewing the generate_github_workflows.py changes first, to get a sense, then carefully examine the generated file diffs to verify that the impacts are correct on the existing workflows, and then review the new m1 workflow.

path: .pants.d/pants.log
- name: Upload native binaries
uses: actions/upload-artifact@v3
with:
name: native_binaries.${{ matrix.python-version }}.${{ runner.os }}
name: native_binaries.${ matrix.python-version }.Linux-x86_64
Copy link
Contributor Author

@benjyw benjyw Jun 5, 2022

Choose a reason for hiding this comment

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

We can't use "${{ runner.os }}-${{ runner.arch }}" because the M1 runner is an X86 binary running under Rosetta, and it sees its arch as x86_64... But since we know the platform we're targeting, we can just inject that into the string directly.

@@ -100,10 +102,6 @@ jobs:
- name: Bootstrap Pants
run: './pants --version

'
- name: Validate CI config
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved below, after the bootstrapping is complete.

steps:
- env:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was here by mistake. The dependency on check_labels has already run this check, we just neglected to remove these steps from this job when we broke that out into its own job.

@@ -253,18 +246,34 @@ jobs:
run: './pants --version

'
- name: Run smoke tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No harm in running smoke tests as part of bootstrapping, as they should be really fast, and it saved me having to make this conditional. But we can do so if it turns out to be a problem.


'
- if: always()
name: Upload pants.log
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We weren't previously uploading the pants.log in all cases, now we do.

@@ -251,32 +268,17 @@ def bootstrap_caches() -> Sequence[Step]:
]


def native_binaries_upload() -> Step:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were just moved into Helper

"needs": "check_labels",
"strategy": {"matrix": {"python-version": python_versions}},
"env": DISABLE_REMOTE_CACHE_ENV,
"timeout-minutes": 40,
"if": IS_PANTS_OWNER,
"steps": [
*checkout(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All moved into Helper.bootstrap_pants()

@@ -376,21 +475,7 @@ def test_python_linux(shard: str) -> dict[str, Any]:
),
},
{
"name": "Run smoke tests",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into Helper.bootstrap_pants()

@@ -527,15 +605,6 @@ def build_steps(*, is_macos: bool) -> list[Step]:
"env": DISABLE_REMOTE_CACHE_ENV,
"if": IS_PANTS_OWNER,
}
deploy_to_s3_step = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a global helper function above

@benjyw benjyw force-pushed the m1_ci branch 3 times, most recently from 7b7f8aa to d00f199 Compare June 5, 2022 12:42
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Yay!!! You also need to update here:

if is_cross_platform(local_files) and len(local_files) != 6:
formatted_local_files = ", ".join(f.name for f in local_files)
missing_packages.append(
softwrap(
f"""
{package.name} (expected 6 wheels, {{macosx, linux}} x {{cp37m, cp38, cp39}},
but found {formatted_local_files})
"""
)
)

Since we only have one self-hosted M1 runner, and we
don't want it to become a bottleneck, we don't (yet?)
run tests on M1, just bootstrap and build wheels.

Refactors generate_github_workflows.py to break out the
reusable bootstrapping steps.

This involves creating a Helper class that knows about
the target platform and can affect generation appropriately.

Various cache keys that were previously only qualfied by os
are now qualified by platform (os+arch).

This refactoring also regularizes some of the previously
haphazard environment settings, log uploading and smoke test
running.

It also moves the CI config validation after the
bootstrapping, previously they were intermingled.

Also removes a superfluous invocation of the label check
steps inside an unrelated job which already depends on
the "proper" label check job.

[ci skip-rust]

[ci skip-build-wheels]
@benjyw
Copy link
Contributor Author

benjyw commented Jun 5, 2022

Yeah, I'll change that in a follow up, when I know I have real wheels in S3 to test on.

@benjyw
Copy link
Contributor Author

benjyw commented Jun 5, 2022

@benjyw benjyw merged commit 0233b81 into pantsbuild:main Jun 5, 2022
@benjyw benjyw deleted the m1_ci branch June 5, 2022 16:37
@stuhood stuhood mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants