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

Revert to non-incremental docbuild #36469

Closed
wants to merge 7 commits into from

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Oct 16, 2023

The doc build workflow currently fails because it runs out of space (e.g https://github.com/sagemath/sage/actions/runs/6527955359/job/17723528313?pr=36466). The incremental build before is also not really working, as the sagelib is still built again. We remove the incremental build for now, until #36459, and use an older image for now.

📝 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

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 16, 2023

@mkoeppe ?

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 17, 2023

That the docbuild rebuilds sagelib from scratch after the incremental build is not a malfunction. It was requested in #35652, where the incremental building was added.

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 17, 2023

The out of space condition was likely triggered by changes in the GitHub runner images, not on our side.
Our technique to free disk space (before or after #36418) unfortunately does not apply to workflow jobs that use the container keyword.
We probably have to rewrite these jobs using an explicit use of docker instead.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 17, 2023

That the docbuild rebuilds sagelib from scratch after the incremental build is not a malfunction. It was requested in #35652, where the incremental building was added.

Do you mean this: #35652 (comment) ?

There I suggested to rebuild doc from scratch, not sagelib from scratch...

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 17, 2023

Do you mean this: #35652 (comment) ?

Yes

There I suggested to rebuild doc from scratch, not sagelib from scratch...

Re-reading it now, I think I misunderstood what you wanted.

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 17, 2023

I didn't know that you misunderstood. From

make doc-clean doc-uninstall sagelib-clean && git clean -fx src/sage && ./config.status && make doc-html

the part sagelib-clean && git clean -fx src/sage causes rebuilding sagelib from scratch?

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 17, 2023

Yes. I'm revising it in #36473

@tobiasdiez
Copy link
Contributor Author

The out of space condition was likely triggered by changes in the GitHub runner images, not on our side.

Right, it's still not working even with the old beta 1 image. Any idea how to fix it and what takes up so much space?

Closing this now as #36473 is a better solution.

@tobiasdiez tobiasdiez closed this Oct 17, 2023
@tobiasdiez tobiasdiez deleted the ci-fix-docs branch October 17, 2023 10:10
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 19, 2023
    
<!-- ^^^^^
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 -->

Reworking sagemath#35652 as discussed in
sagemath#36469 (comment)

Also applying the changes made in sagemath#35866 (cosmetic improvements to the
Actions logs of doc-build.yml) to doc-build-pdf.yml

<!-- 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.
- [ ] 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: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36473
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 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants