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 build, doc-build: Run containers explicitly, separate jobs for pyright, build, modularized tests, long tests #36498

Merged
merged 171 commits into from
May 2, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Oct 21, 2023

Running the containers explicitly, instead of using the declarative container: feature of GH Actions gives us more control:

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 ci: improve things around auto-apply of fixes #36349.

The workflow build.yml first launches a job:

After this is completed, more jobs are launched:

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:

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

📝 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

@mkoeppe mkoeppe force-pushed the ci_build_explicit_docker_exec branch 16 times, most recently from 65f827e to 118bd40 Compare October 22, 2023 01:57
@mkoeppe mkoeppe self-assigned this Oct 22, 2023
@mkoeppe mkoeppe marked this pull request as ready for review October 22, 2023 05:05
@kwankyu
Copy link
Collaborator

kwankyu commented Oct 22, 2023

src/sage/schemes/toric/variety.py sneaked in?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 22, 2023

via #36443

@tobiasdiez
Copy link
Contributor

I don't get the point of this PR. It seems to mostly complicate the setup and essentially reimplements the container feature of github. For the out-of-space issue it would be nice to first know what's the route cause to be able to evaluate if steps to fixing it go into the correct direction. For potential speedups by caching the docker image, I'm not sure if this is really needed. Yes, from a conceptual viewpoint you only want to build sagelib once, and then reuse it - but well, it mostly takes 2-10 mins of incremental compilation anyway. Personally, I think we have more pressing issues with the ci setup and better not introduce more moving parts (atm at least).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 22, 2023

For the out-of-space issue it would be nice to first know what's the root cause to be able to evaluate if steps to fixing it go into the correct direction.

It's really not very complicated. GitHub runs the container from its very full runner image. We know that it does not have enough space for our large containers.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 22, 2023

It seems to [...] essentially reimplements the container feature of github.

That's right. The declarative container feature is convenient, but we have run into the two restrictions described in the PR description. So we just use docker directly; problem solved.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 22, 2023

Yes, from a conceptual viewpoint you only want to build sagelib once, and then reuse it - but well, it mostly takes 2-10 mins of incremental compilation anyway.

Actually, the compilation can take much longer when low level parts of sagelib are touched, and when certain packages are updated.

@tobiasdiez
Copy link
Contributor

For the out-of-space issue it would be nice to first know what's the root cause to be able to evaluate if steps to fixing it go into the correct direction.

It's really not very complicated. GitHub runs the container from its very full runner image. We know that it does not have enough space for our large containers.

Github provides 14gb of space (after its runner image), that should be sufficient (and was sufficient) to run a 7gb image in it and install sage. Also our image got bigger by a few houndred mbs.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 22, 2023

If you want to understand more details, I'd suggest that you experiment with the space parameters in https://github.com/sagemath/sage/blob/develop/.github/workflows/docker.yml#L148

@mkoeppe mkoeppe requested a review from dimpase October 22, 2023 20:11
@tobiasdiez
Copy link
Contributor

It's already quite easy to run commands outside the container. Here is an example for exactly your first use case: https://github.com/osquery/osquery/blob/a916e80a895f8abb9b749161d774d528d0466b31/.github/workflows/hosted_runners.yml#L257-L263 I don't think we need more flexibility.

@vbraun vbraun merged commit f633de0 into sagemath:develop May 2, 2024
35 of 40 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone May 2, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request May 4, 2024
    
Bump up the bumpable actions.
Should be complete in conjunction with sagemath#37474.

Note: `potiuk/get-workflow-origin` is not maintained anymore and throws
warnings.

- Depends on sagemath#36498 (merged here)
    
URL: sagemath#37818
Reported by: gmou3
Reviewer(s): Dima Pasechnik
@mkoeppe mkoeppe deleted the ci_build_explicit_docker_exec branch May 6, 2024 19:47
vbraun pushed a commit to vbraun/sage that referenced this pull request May 9, 2024
    
Bump up the bumpable actions.
Should be complete in conjunction with sagemath#37474.

Note: `potiuk/get-workflow-origin` is not maintained anymore and throws
warnings.

- Depends on sagemath#36498 (merged here)
    
URL: sagemath#37818
Reported by: gmou3
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request May 11, 2024
    
Bump up the bumpable actions.
Should be complete in conjunction with sagemath#37474.

Note: `potiuk/get-workflow-origin` is not maintained anymore and throws
warnings.

- Depends on sagemath#36498 (merged here)
    
URL: sagemath#37818
Reported by: gmou3
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
    
Bump up the bumpable actions.
Should be complete in conjunction with sagemath#37474.

Note: `potiuk/get-workflow-origin` is not maintained anymore and throws
warnings.

- Depends on sagemath#36498 (merged here)
    
URL: sagemath#37818
Reported by: gmou3
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this pull request May 12, 2024
    
Bump up the bumpable actions.
Should be complete in conjunction with sagemath#37474.

Note: `potiuk/get-workflow-origin` is not maintained anymore and throws
warnings.

- Depends on sagemath#36498 (merged here)
    
URL: sagemath#37818
Reported by: gmou3
Reviewer(s): Dima Pasechnik
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.

build.yml: Report failures (e.g. from incremental test) earlier
4 participants