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

Move CI/CD from Azure Pipelines to GitHub Actions #1808

Merged
merged 2 commits into from
Jun 14, 2022
Merged

Move CI/CD from Azure Pipelines to GitHub Actions #1808

merged 2 commits into from
Jun 14, 2022

Conversation

timkpaine
Copy link
Member

@timkpaine timkpaine commented May 14, 2022

This PR replaces the Perspective CI/CD infrastructure. We are currently using Azure Pipelines, and this PR replaces that with GitHub actions

Motivations

Azure pipelines are very similar to GitHub Actions (coming from the same company), both with a number of important distinctions

  • Separate interface, login, controls, etc, in Azure Devops instead of natively in GitHub
  • More difficult pipeline syntax
  • Smaller / fewer community extensions

Additionally, anecdotal evidence suggest that the machines and pipelines used for azure pipelines are slower than for GitHub actions, and Perspective's complex build process (which is becoming more complex due to ongoing JS / Python integration work) leads to long build times.

After some initial exploration, the easier-to-grok syntax and anecdotally faster speed of GitHub Actions was enough to motivate experiments for migrating.

Requirements

Perspective builds with the following languages and technologies:

  • node for scripting / project orchestration
  • emscripten emsdk for C++ compilation to webassembly
  • C/C++ compilers for python integration
  • Python + dependencies
  • rust for frontend components via webassembly

Because perspective builds its python assets for deploy on azure, we need to support the following build matrix

  • OS X 10.15
  • Windows 2019
  • Any linux (linux packages built via manylinux docker images which provide python 3.6-3.9)
  • Python 3.6-3.9 (mac)
  • Python 3.7-3.9 (windows)

All required technologies, operating systems, compilers, environments are supported for GitHub actions.

Build assets

Azure and this PR both emit the following wheel assets

  • cp36-cp36m-macosx_10_14_x86_64
  • cp37-cp37m-macosx_10_15_x86_64
  • cp38-cp38-macosx_10_15_x86_64
  • cp39-cp39-macosx_10_15_x86_64
  • cp36-cp36m-manylinux2010_x86_64
  • cp37-cp37m-manylinux2010_x86_64
  • cp38-cp38-manylinux2010_x86_64
  • cp39-cp39-manylinux2010_x86_64
  • cp36-cp36m-manylinux2014_x86_64
  • cp37-cp37m-manylinux2014_x86_64
  • cp38-cp38-manylinux2014_x86_64
  • cp39-cp39-manylinux2014_x86_64
  • cp37-cp37m-win_amd64
  • cp38-cp38-win_amd64
  • cp39-cp39-win_amd64

Caching

Caching is easier to configure on GitHub Actions vs Azure. However, it has caused some problems before so we may need to revisit it.

  • CCache for C++ compilation
  • Node cache for node_modules
  • Pip cache for python deps
  • rust/cargo cache for rust deps
  • homebrew cache for mac deps
  • vcpkg for windows C++ deps

Build Flags / Partial Builds

On Azure, we have overridable flags to check if the build is from a git tag (e.g. something starting with v), so that we can performa a full build. This flag is overridable so that manual full builds can be run.

This PR provides the following set of flags, which can be specified via:

  • commit message

  • PR title

  • manual workflow run

  • [ci-full] run a full build across all environments. This is the default when a git tag starts with v for releases

  • [ci-skip] no CI, e.g. for docs or non-build configuration

  • [ci-skip-python] for pure JS changes, reduces the build time by roughly 50%

  • [ci-include-windows] include the windows build, increases the build time by roughly 30%

  • [ci-skip-cache] omits all caching, which has caused problems in the past on azure. Increases the build time by roughly 20%

Helpful scripts have been added to make empty commits with these messages:

        "ci:full": "git commit --allow-empty -m \"[ci-full]\"",
        "ci:skip-python": "git commit --allow-empty -m \"[ci-skip-python]\"",
        "ci:include-windows": "git commit --allow-empty -m \"[ci-include-windows]\"",
        "ci:skip-cache": "git commit --allow-empty -m \"[ci-skip-cache]\"",

To run a workflow manually, the syntax looks like this (via gh command line)

gh workflow run "Build Status" --ref tkp/gha  -f ci-full=true -f ci-skip-cache=false  -f ci-skip-python=false -f ci-include-windows=true

This runs the tkp/gha branch with the provided set of flags

Tests performed on this PR

  • Normal CI build
  • Skip CI
  • Full CI Build
  • Skip Python
  • Include Windows
  • Skip Cache

See the comments for more information

@timkpaine
Copy link
Member Author

Normal CI build passing, time to test the flags

@timkpaine timkpaine force-pushed the tkp/gha branch 10 times, most recently from 93bb86d to 3956f8b Compare June 6, 2022 17:36
@timkpaine timkpaine added this to the 1.4 milestone Jun 6, 2022
@timkpaine timkpaine changed the title work on moving some stuff to gha [ci-skip-python] work on moving some stuff to gha Jun 6, 2022
@timkpaine timkpaine force-pushed the tkp/gha branch 5 times, most recently from 1684988 to 803d4fe Compare June 10, 2022 19:00
@timkpaine
Copy link
Member Author

timkpaine commented Jun 10, 2022

Manually triggered run https://github.com/finos/perspective/actions/runs/2476949750

Looks like results dont get pushed into the PR itself but we'll have to manage that difference between gha and azure

@timkpaine timkpaine marked this pull request as ready for review June 10, 2022 19:04
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I would like to merge this but it cannot be the default CI in this PR. I propose this be split into 2 PRs, one with just what is needed to enable GHA for builds, and a second which promotes this to the default; I've marked some places I believe should go in the latter above.

There is some minor debug/logging code that should be cleaned up or justified, and some test condition changes and "timeout inflation" that looks like leftover debug code I'd like removed or traced (to my knowledge there are no races in the existing jlab test code, I spent quite some time on this issue in #1823 and others).

As discussed, I'll give in on the directives in commit messages due to necessity, on the agreement that they not be used in open PRs unless startlingly necessary, and that they never be exclusively-directive commits (e.g. --allow-empty commits).

README.md Outdated Show resolved Hide resolved
azure-pipelines.yml Outdated Show resolved Hide resolved
docker/python/manylinux2010/Dockerfile Outdated Show resolved Hide resolved
@@ -52,6 +52,8 @@ function check() {
}
}

if (!check()) {
upgrade();
if (!process.env.PSP_SKIP_EMSDK_INSTALL) {
Copy link
Member

Choose a reason for hiding this comment

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

I understand you don't want to always auto-install emsdk as a side effect of other commands, but putting this check here disables installation globally including when it is set in .perspectiverc. Can we do this where install_emsdk is invoked as a side effect instead (presumably post_install?)

I'd like to say no developer is stupid enough to get confused by this, but I just was, for an embarrassingly long time.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you set PSP_SKIP_EMSDK_INSTALL, a very specific env var, you will not install emsdk. Otherwise it will operate as before.

Copy link
Member

Choose a reason for hiding this comment

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

I cited an explicit example of how this fails (env vars injected through .perspectiverc), and it is so easy to fix

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't reproduce, please share your configuration and commands

scripts/install_tools.js Outdated Show resolved Hide resolved
scripts/publish_python.js Show resolved Hide resolved
scripts/test_python.js Show resolved Hide resolved
scripts/test_js.js Outdated Show resolved Hide resolved
This was referenced Jun 13, 2022
@timkpaine timkpaine force-pushed the tkp/gha branch 4 times, most recently from 6ba9156 to 27721e7 Compare June 13, 2022 14:52
@timkpaine
Copy link
Member Author

sliced binder changes over to #1855

@timkpaine
Copy link
Member Author

flatbuffers changes consolidated in https://github.com/finos/perspective/pull/1820/files

@timkpaine timkpaine requested a review from texodus June 13, 2022 16:20
@texodus
Copy link
Member

texodus commented Jun 13, 2022

full build

@texodus
Copy link
Member

texodus commented Jun 14, 2022

As per discussion offline, I've force pushed this branch with a real commit message. Additional changes I've made

  • Removed README.md badge (this is not stable yet and I don't want a scary red warning on our docs site).
  • Removed kernel metadata and kernel selection bypass in test utils. I suspect strongly this was hand-tested against a python environment that is inconsistent with out development/CI structure, as the default kernel name is not apt to change between CI runs ... if this work has merit, it should be able to be justified on its own PR.
  • Removed Jupyter test changes. Further removed condition timeouts entirely. Despite public outcry, this seems to work just fine.
  • Removed sdist builds from default GHA runs. sdist is the slowest build of them all, and IMO the lowest risk for failure.
  • Revert jupyter widgets version bump - I don't want to guess why timing and/or correctness issues happen on this branch so there should not be any superfluous code changes.
  • Removed fake superstore arrow test entirely. Merits of the sdist testing aside, we need fewer mystery blobs in this repo, not more, and this test is not worth fighting over.

full gha
full azure

@texodus texodus merged commit 82cafab into master Jun 14, 2022
@texodus texodus deleted the tkp/gha branch June 14, 2022 09:20
@timkpaine
Copy link
Member Author

Note that the kernel can change between CI runs as the valid kernels are specified by the installation of jupyter, not by anything in this package.

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.

2 participants