-
-
Notifications
You must be signed in to change notification settings - Fork 482
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: improve things around auto-apply of fixes #36349
Conversation
Ticket description should probably explain what you are trying to fix here. |
better now? |
Kindly rephrase with less aggression. |
It works in principle, although it for some reason things its on the master branch. I let you @mkoeppe fix this and change the other workflows accordingly. |
Please work on the ticket description. Bullet point 2 is unclear to me. What is broken, where can I see an example of it, why would any of the changes that you made fix it. |
Currently, it tries to apply fixes during the |
That the step "Store CI fixes" fails is already fixed in #36348. |
Explain why you think this thing should only applied to pull requests? |
Because the pushes to develop are the ground truth and should not depend on external factors (in particular not on PRs that are not yet merged). Moreover, the point of this feature is to lessen the pain of CI in PRs to fail without the hotfixes applied. |
OK, that's certainly a plausible point for pushes of release tags to sagemath/sage, and for pushes to develop and master of sagemath/sage. It's less clear to me for other pushes (to a branch, to other tags, to another repository). For example, I like to push tags to my fork to trigger the portability workflows -- those definitely benefit from applying the CI fixes; it's one fewer thing to do manually. So this may need more thought. I don't think immediate action is needed. |
Feel free to modify the workflows in your fork according to your needs. |
Have you tested it with 32 bit, as discussed? |
We don't use 32bit to test PRs. |
Oh, I see, you have introduced a condition on Edit, to elaborate: Now with some experience of using the auto-apply, I can say that we definitely want it to take effect generally, not just on PRs. In particular the (long-running) portability tests need this so that a trivial error does not block portability testing for the whole release. |
To address the cosmetic concern of repeated line in the Actions check list, I'd suggest to just merge build.yml, doc-build.yml, and doc-build-pdf.yml into one file. |
I'm not touching the portability tests... |
The "get_ci_fixes" is just visual noise nobody cares about, but it's always in your face. They have to run first (and are completed first), so they show up first in the checks list. Compare the view without this PR Also if you go to the "Build & Test" workflow you directly see at which point it broke and don't have to select the actual @kwankyu @dimpase could you please review this PR (if you agree that it adds value, of course). |
In case it was unclear at all (?!), I was referring to the change here that restricts the application of the CI fixes to a set of conditions, instead of running it unconditionally:
|
I am all for improving the cosmetic issue. Installing gh cli seems all right to me. But if it does matter regarding portability tests (i386 ?), I will leave the decision to @mkoeppe, as he is orchestrating ci pipelines. If the situation is like "yes gh cli for PR tests, but no gh cli for portability tests", I would rather live with the cosmetic issue instead of sacrificing consistency. |
What are you missing? It still runs on
Both the "Build & Test" and the portability workflows need gh-cli to merge the changes from blocker prs. Only the way to install it is slightly different (btw, why are we making our lives so hard and don't have git + ghcli in the docker images?) |
The Docker images test different configurations of system packages ... so as to make sure that Sage builds on different configurations of system packages on users' systems (... and deployments). In particular, the |
Yes |
if: runner.os == 'Linux' | ||
run: | | ||
apt update | ||
apt install -y git |
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.
No, one cannot assume that on Linux, apt
is the package manger.
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.
Well, it's working, so I call it good enough...
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.
When the workflow is run manually, the container can be selected by the user. As you know.
"That it's working" is because you didn't test it on a system that does not use apt.
@@ -19,7 +19,7 @@ else | |||
$GH pr checkout -b pr-$a $a | |||
git fetch --unshallow --all | |||
git checkout -q test_head | |||
if git merge --no-edit --squash -q pr-$a; then | |||
if git merge --no-edit --squash --allow-unrelated-histories -q pr-$a; then |
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 one has to use this flag, typically something's wrong already
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.
The checkout action results in a detached head for PRs. You can call that "wrong" but its by design.
With what reasoning? What advantages do you see? We actually had the situation that the conda workflow in some PRs was failing: maybe because of another blocker PR that got auto-merged, maybe not. We were not able to say why, because the conda workflow in the PR with the blocker label was running the same code as the one in the develop branch. So if you have an unmerged PR with blocker label, the workflows it runs are the same as the one in the develop branch, in other words one cannot see if some behavior is due to some changes in this PR or not. This is very bad in my opinion. I might also remind you that the idea of the feature was to help with broken PRs, not quickly fix workflows on the develop branch by virtually merging the PR. |
You have said that before, and it may have been your understanding of why I implemented #36338, but it was only part of the motivation. It's working as designed in #36338. |
Also mind responding to the actual main part of my comment? |
No, there's no mystery, the workflow logs clearly which blocker PRs are applied and for which the merge is failing. |
The problem was that some doctests in repl were failing, probably/maybe because of changes in that blocker PR. And there was no way to easily run the tests without any PRs applied, so it was not possible to easily see if these errors are coming from the blocker or because some conda package updated or whatever. Please give me one single situation where it is handy to apply the fixes in the build & test workflow also for pushes to develop. |
Documentation preview for this PR (built with commit 20fb5c3; changes) is ready! 🎉 |
…customizable by repository variable <!-- ^^^^^ 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 --> Instead of using `gh pr checkout`, we obtain the CI fixes via their patch URLs. - This is faster because typically we do not have to unshallow the repo to apply the patches (seconds instead of ~2 minutes) - Fewer surprises when applied to a PR based on an older release - Conjecturally uses fewer API queries, helping avoid sagemath#36685 When a repository variable `SAGE_CI_FIXES_FROM_REPOSITORIES` is set in a fork, it is used instead of the hardcoded sagemath/sage as the source(s) of the CI fixes; this gives better control in decentralized development. When set to "none", this also makes it possible to see the "ground truth", addressing a concern raised in sagemath#36349 (comment). See comments at the top of `.ci/merge-fixes.sh` for details. Also improving the display of details of the merged PRs, as requested by @tornaria in sagemath#36696 (comment) Example runs: - https://github.com/sagemath/sage/actions/runs/6854931832/job/186389018 59?pr=36686 (here it recovers gracefully from failed merges due to the 10.2.rc1/10.2.rc2 tagging confusions) - https://github.com/mkoeppe/sage/actions/runs/6855410957/job/1864038039 8#step:3:8 (I have set `SAGE_CI_FIXES_FROM_REPOSITORIES=none` in my fork -- for testing this PR) <!-- 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. - [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#36686 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe
…customizable by repository variable <!-- ^^^^^ 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 --> Instead of using `gh pr checkout`, we obtain the CI fixes via their patch URLs. - This is faster because typically we do not have to unshallow the repo to apply the patches (seconds instead of ~2 minutes) - Fewer surprises when applied to a PR based on an older release - Conjecturally uses fewer API queries, helping avoid sagemath#36685 When a repository variable `SAGE_CI_FIXES_FROM_REPOSITORIES` is set in a fork, it is used instead of the hardcoded sagemath/sage as the source(s) of the CI fixes; this gives better control in decentralized development. When set to "none", this also makes it possible to see the "ground truth", addressing a concern raised in sagemath#36349 (comment). See comments at the top of `.ci/merge-fixes.sh` for details. Also improving the display of details of the merged PRs, as requested by @tornaria in sagemath#36696 (comment) Example runs: - https://github.com/sagemath/sage/actions/runs/6854931832/job/186389018 59?pr=36686 (here it recovers gracefully from failed merges due to the 10.2.rc1/10.2.rc2 tagging confusions) - https://github.com/mkoeppe/sage/actions/runs/6855410957/job/1864038039 8#step:3:8 (I have set `SAGE_CI_FIXES_FROM_REPOSITORIES=none` in my fork -- for testing this PR) <!-- 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. - [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#36686 Reported by: Matthias Köppe Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe
…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
…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
…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
Fixes some of the mess that was introduced by merging #36338. In particular,
📝 Checklist
⌛ Dependencies