-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
6067902
to
fac09b1
Compare
0a83537
to
055248e
Compare
Normal CI build passing, time to test the flags |
93bb86d
to
3956f8b
Compare
1684988
to
803d4fe
Compare
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 |
There was a problem hiding this 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).
packages/perspective-jupyterlab/test/config/jupyter/jlab_start.js
Outdated
Show resolved
Hide resolved
packages/perspective-jupyterlab/test/config/jupyter/jlab_start.js
Outdated
Show resolved
Hide resolved
@@ -52,6 +52,8 @@ function check() { | |||
} | |||
} | |||
|
|||
if (!check()) { | |||
upgrade(); | |||
if (!process.env.PSP_SKIP_EMSDK_INSTALL) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
6ba9156
to
27721e7
Compare
sliced binder changes over to #1855 |
flatbuffers changes consolidated in https://github.com/finos/perspective/pull/1820/files |
configuration of which subsets of python builds
As per discussion offline, I've force pushed this branch with a real commit message. Additional changes I've made
|
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. |
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
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:
Because perspective builds its python assets for deploy on azure, we need to support the following build matrix
All required technologies, operating systems, compilers, environments are supported for GitHub actions.
Build assets
Azure and this PR both emit the following wheel assets
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.
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 withv
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:
To run a workflow manually, the syntax looks like this (via
gh
command line)This runs the
tkp/gha
branch with the provided set of flagsTests performed on this PR
See the comments for more information