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

CI: Merge workflows into build.yml #36446

Closed
wants to merge 37 commits into from

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Oct 11, 2023

Simplifying our workflows running on every ticket, as suggested in https://groups.google.com/g/sage-devel/c/Cisdhf3nSbM

Unchanged as of this PR: build, doc-build, doc-build-pdf jobs all build the image. This can be improved by building a container first.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

…nor versions (environment-3.9.yml etc.)

This is convenient for conda-lock.

Partial cherry-pick from "Use conda-lock for reproducible conda env"
…: Use configure --enable-system-site-packages
@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 11, 2023

@kwankyu Here's an attempt to reorganize the CI workflows. Checks tab: https://github.com/sagemath/sage/pull/36446/checks

This is not the final implementation; the merge of the separate workflows also enables sharing the build phases of build and docbuild workflows via Docker.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 11, 2023

Also ci-conda could be merged here, but I'm not touching it for the moment because other PRs are working on it

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 11, 2023

We may have separate "Build & Test" and "Build & Test using Conda". They are well distinguished. As far as I understand, Building on conda is still not as stable as on the vanilla system. Hence merging them would make "Build & Test" unstable and too big. "Build & Test" is our core workflow, and better to be kept rock-reliable.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 15, 2023
…-check`

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

We add `make` targets for testing wheel-building script packages that
have already been installed. For example:
```
$ make SAGE_WHEELS=yes sagemath_categories  # builds and stores (but
does not install) a wheel
$ make sagemath_categories-check            # tests the wheel using tox
```
The target `make pypi-wheels-check` calls all of them. We restructure
the Build & Test CI so that all tests are run after build has been
completed. Later, as a follow up of this and sagemath#36446, we can store the
result of Build as a container image and then parallelize the various
tests on top of it.

This is implemented on top of an improvement of how we record
information on our wheels.
```
$ cat venv/var/lib/sage/scripts/sagemath_categories/spkg-
requirements.txt
sagemath_categories @ file:///Users/mkoeppe/s/sage/sage-
rebasing/worktree-algebraic-2018-spring/local/var/lib/sage/venv-python3.
11/var/lib/sage/wheels/sagemath_categories-10.2b6-cp311-cp311-
macosx_13_0_x86_64.whl
```
Previously, this information was only known during the execution of the
`spkg-install` script.

In a follow-up, we can address sagemath#30956 by delaying the wheel installation
to spkg-postinst.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36452
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 17, 2023
…-check`

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

We add `make` targets for testing wheel-building script packages that
have already been installed. For example:
```
$ make SAGE_WHEELS=yes sagemath_categories  # builds and stores (but
does not install) a wheel
$ make sagemath_categories-check            # tests the wheel using tox
```
The target `make pypi-wheels-check` calls all of them. We restructure
the Build & Test CI so that all tests are run after build has been
completed. Later, as a follow up of this and sagemath#36446, we can store the
result of Build as a container image and then parallelize the various
tests on top of it.

This is implemented on top of an improvement of how we record
information on our wheels.
```
$ cat venv/var/lib/sage/scripts/sagemath_categories/spkg-
requirements.txt
sagemath_categories @ file:///Users/mkoeppe/s/sage/sage-
rebasing/worktree-algebraic-2018-spring/local/var/lib/sage/venv-python3.
11/var/lib/sage/wheels/sagemath_categories-10.2b6-cp311-cp311-
macosx_13_0_x86_64.whl
```
Previously, this information was only known during the execution of the
`spkg-install` script.

In a follow-up, we can address sagemath#30956 by delaying the wheel installation
to spkg-postinst.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36452
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@tobiasdiez
Copy link
Contributor

How do you plan to handle the fact that some of the workflows currently have different triggers? In particular, the Linux work flow only being run when there is a label? Similarly, I think it will be hard to trigger the publish doc workflow directly after the docbuild has been finished.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 19, 2023
…-check`

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

We add `make` targets for testing wheel-building script packages that
have already been installed. For example:
```
$ make SAGE_WHEELS=yes sagemath_categories  # builds and stores (but
does not install) a wheel
$ make sagemath_categories-check            # tests the wheel using tox
```
The target `make pypi-wheels-check` calls all of them. We restructure
the Build & Test CI so that all tests are run after build has been
completed. Later, as a follow up of this and sagemath#36446, we can store the
result of Build as a container image and then parallelize the various
tests on top of it.

This is implemented on top of an improvement of how we record
information on our wheels.
```
$ cat venv/var/lib/sage/scripts/sagemath_categories/spkg-
requirements.txt
sagemath_categories @ file:///Users/mkoeppe/s/sage/sage-
rebasing/worktree-algebraic-2018-spring/local/var/lib/sage/venv-python3.
11/var/lib/sage/wheels/sagemath_categories-10.2b6-cp311-cp311-
macosx_13_0_x86_64.whl
```
Previously, this information was only known during the execution of the
`spkg-install` script.

In a follow-up, we can address sagemath#30956 by delaying the wheel installation
to spkg-postinst.

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36452
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 19, 2023

Thanks, these are relevant points to consider.

Current status (mainly for future reference):

  • Pushing the image to a registry works great for pushes but, as we know, cannot work for PRs because there aren't (and likely should not be) permissions.
  • Saving the Docker image as an artifact is too slow, much slower than pushing the image to a registry (in which case existing layers are reused). Pushing a multi-gigabyte image via upload-artifact takes very long, so not a suitable approach.
  • One can of course easily run a local registry (e.g. via the services keyword), but it will only be accessible by a job, not by all jobs in a workflow, so nothing gained.
  • Saving the changes as an incremental tarfile (https://www.gnu.org/software/tar/manual/html_node/Incremental-Dumps.html), which I was using successfully in the multi-stage Cygwin workflow, unfortunately also does not work well because of the type of file system used within Docker containers.
  • Approaches such as https://github.com/aidanhs/dlgrab are possible, but I won't pursue this direction.

Next approach that I'll try:

  • Docker layer caching via https://docs.docker.com/build/cache/backends/gha/ -- after building the image incrementally, the jobs for docbuild, ptest, etc. will just again rebuild the image incrementally from the same source image using the same steps; when the cache can be retrieved, this will be very fast; otherwise, it just falls back to building the image incrementally (= current status).

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 21, 2023

I'm closing this and will do the docker layer caching in a separate PR; which will keep the workflows separate.

Edit: But first #36498 / #36508.

@mkoeppe mkoeppe closed this Oct 21, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 18, 2024
…arate jobs for pyright, build, modularized tests, long tests

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
Running the containers explicitly, instead of using the declarative
`container:` feature of GH Actions gives us more control:
- we can create more space on the host if necessary; we just scraped by
an out of space condition in sagemath#36473 /
sagemath#36469 (comment)
- we can run some operations outside of the container but in the same
job; this will make the separate "Get CI fixes" jobs unnecessary,
addressing the cosmetic concerns from
sagemath#36338 (comment),
sagemath#36349
- it enables caching between the various workflows (as first discussed
in sagemath#36446).

We split out static type checking with pyright into its separate
workflow:
- **pyright.yml**: As static checking does not need  a build of the Sage
library, for PRs that do not make any changes to packages, there's
nothing to build, and the workflow gives a fast turnaround just after 10
minutes. For applying the CI fixes from blocker tickets, this workflow
uses the technique favored in sagemath#36349.

The workflow **build.yml** first launches a job:
- **test-new:** It builds incrementally (using a tox-generated
`Dockerfile` and https://github.com/marketplace/actions/build-and-push-
docker-images) and does the quick incremental test. This completes
within 10 to 20 minutes when there's no change.

After this is completed, more jobs are launched:
- **test-mod:** It again builds incrementally and tests a modularized
distribution. Later (with more from sagemath#35095), more jobs will be added to
this matrix job for other distributions.
- **test-long:** It again builds incrementally and runs the long test.

The workflows **doc-build.yml** and **doc-build-pdf.yml** again build
incrementally and then build the documentation. The diffing code for the
HTML documentation now runs in the host, not the container, which
simplifies things. (To show that diffing still works, we include a small
change to the Sage library.)

Splitting the workflow jobs implements @kwankyu's  suggestion from:
- sagemath#35652 (comment)
(Fixes sagemath#35814)

The steps "again builds incrementally" actually use the GH Actions
cache, https://docs.docker.com/build/ci/github-actions/cache/#cache-
backend-api. When nothing is cached and the 3 workflows are launched in
parallel, they may each build the same thing. But when there's
congestion that leads to the workflows to be run serially, the 2nd and
3rd workflow will be able to use the cache from the 1st workflow. This
elasticity may be helpful in reducing congestion.

There is a rather small per-project limit of 10 GB for this cache, so
we'll have to see how effectively caching works in practice. See
https://github.com/sagemath/sage/actions/caches


<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36938 (merged here)
- Depends on sagemath#36951 (merged here)
- Depends on sagemath#37351 (merged here)
- Depends on sagemath#37750 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36498
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
…arate jobs for pyright, build, modularized tests, long tests

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
Running the containers explicitly, instead of using the declarative
`container:` feature of GH Actions gives us more control:
- we can create more space on the host if necessary; we just scraped by
an out of space condition in sagemath#36473 /
sagemath#36469 (comment)
- we can run some operations outside of the container but in the same
job; this will make the separate "Get CI fixes" jobs unnecessary,
addressing the cosmetic concerns from
sagemath#36338 (comment),
sagemath#36349
- it enables caching between the various workflows (as first discussed
in sagemath#36446).

We split out static type checking with pyright into its separate
workflow:
- **pyright.yml**: As static checking does not need  a build of the Sage
library, for PRs that do not make any changes to packages, there's
nothing to build, and the workflow gives a fast turnaround just after 10
minutes. For applying the CI fixes from blocker tickets, this workflow
uses the technique favored in sagemath#36349.

The workflow **build.yml** first launches a job:
- **test-new:** It builds incrementally (using a tox-generated
`Dockerfile` and https://github.com/marketplace/actions/build-and-push-
docker-images) and does the quick incremental test. This completes
within 10 to 20 minutes when there's no change.

After this is completed, more jobs are launched:
- **test-mod:** It again builds incrementally and tests a modularized
distribution. Later (with more from sagemath#35095), more jobs will be added to
this matrix job for other distributions.
- **test-long:** It again builds incrementally and runs the long test.

The workflows **doc-build.yml** and **doc-build-pdf.yml** again build
incrementally and then build the documentation. The diffing code for the
HTML documentation now runs in the host, not the container, which
simplifies things. (To show that diffing still works, we include a small
change to the Sage library.)

Splitting the workflow jobs implements @kwankyu's  suggestion from:
- sagemath#35652 (comment)
(Fixes sagemath#35814)

The steps "again builds incrementally" actually use the GH Actions
cache, https://docs.docker.com/build/ci/github-actions/cache/#cache-
backend-api. When nothing is cached and the 3 workflows are launched in
parallel, they may each build the same thing. But when there's
congestion that leads to the workflows to be run serially, the 2nd and
3rd workflow will be able to use the cache from the 1st workflow. This
elasticity may be helpful in reducing congestion.

There is a rather small per-project limit of 10 GB for this cache, so
we'll have to see how effectively caching works in practice. See
https://github.com/sagemath/sage/actions/caches


<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36938 (merged here)
- Depends on sagemath#36951 (merged here)
- Depends on sagemath#37351 (merged here)
- Depends on sagemath#37750 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36498
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
…arate jobs for pyright, build, modularized tests, long tests

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
Running the containers explicitly, instead of using the declarative
`container:` feature of GH Actions gives us more control:
- we can create more space on the host if necessary; we just scraped by
an out of space condition in sagemath#36473 /
sagemath#36469 (comment)
- we can run some operations outside of the container but in the same
job; this will make the separate "Get CI fixes" jobs unnecessary,
addressing the cosmetic concerns from
sagemath#36338 (comment),
sagemath#36349
- it enables caching between the various workflows (as first discussed
in sagemath#36446).

We split out static type checking with pyright into its separate
workflow:
- **pyright.yml**: As static checking does not need  a build of the Sage
library, for PRs that do not make any changes to packages, there's
nothing to build, and the workflow gives a fast turnaround just after 10
minutes. For applying the CI fixes from blocker tickets, this workflow
uses the technique favored in sagemath#36349.

The workflow **build.yml** first launches a job:
- **test-new:** It builds incrementally (using a tox-generated
`Dockerfile` and https://github.com/marketplace/actions/build-and-push-
docker-images) and does the quick incremental test. This completes
within 10 to 20 minutes when there's no change.

After this is completed, more jobs are launched:
- **test-mod:** It again builds incrementally and tests a modularized
distribution. Later (with more from sagemath#35095), more jobs will be added to
this matrix job for other distributions.
- **test-long:** It again builds incrementally and runs the long test.

The workflows **doc-build.yml** and **doc-build-pdf.yml** again build
incrementally and then build the documentation. The diffing code for the
HTML documentation now runs in the host, not the container, which
simplifies things. (To show that diffing still works, we include a small
change to the Sage library.)

Splitting the workflow jobs implements @kwankyu's  suggestion from:
- sagemath#35652 (comment)
(Fixes sagemath#35814)

The steps "again builds incrementally" actually use the GH Actions
cache, https://docs.docker.com/build/ci/github-actions/cache/#cache-
backend-api. When nothing is cached and the 3 workflows are launched in
parallel, they may each build the same thing. But when there's
congestion that leads to the workflows to be run serially, the 2nd and
3rd workflow will be able to use the cache from the 1st workflow. This
elasticity may be helpful in reducing congestion.

There is a rather small per-project limit of 10 GB for this cache, so
we'll have to see how effectively caching works in practice. See
https://github.com/sagemath/sage/actions/caches


<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
- Depends on sagemath#36938 (merged here)
- Depends on sagemath#36951 (merged here)
- Depends on sagemath#37351 (merged here)
- Depends on sagemath#37750 (merged here)
- Depends on sagemath#37277 (merged here)

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36498
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
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.

None yet

3 participants