Disable broken and outdated CI #39467
Conversation
|
Documentation preview for this PR (built with commit b8639d7; changes) is ready! 🎉 |
|
There's a risk of we just forget to reenable it. Is there a good way to do this dynamically (i.e. check which tests the last release fail and skip these)? |
I'll open an issue where we can track which systems are failing, so we don't forget it. The issue tracker might actually be a more visible place in the first place as most people will not check the CI runs.
Not that I know. |
|
Great, thanks for taking care of this. |
we had a heated argument at some point whether 'minimal' configurations are meaningful beyond someone's curiosity - and they are certainly the longest ones (building gcc, python, and a bunch of equally mainstream packages from sources, that's just a contribution to global warming) |
|
Removing things in CI may break other things elsewhere. Especially the developer guide. In particular, https://doc-release--sagemath.netlify.app/html/en/developer/portability_testing. Did you check? The docker images created by CI linux are used by developers. The sage binder repo (https://github.com/sagemath/sage-binder-env) is using them (in particular the "minimal" image). The interactive sage doc relies on them. This PR will break the doc. Changes affecting developers widely should be discussed or at least posted on sage-devel. This PR falls in the category.
We do not remove code for temporary change. Do not invent a new protocol that code is removed to be disabled. Improving CI is good, but the present PR is also destructive. I object to this PR. |
kwankyu
added
disputed
|
@kwankyu What exactly is broken by this PR? |
|
And if you think this PR needs work, please label it so, not "disputed". |
dimpase
added
s: needs info
and removed
disputed
|
Technically you're right in that this merely remove part of CI that has been broken by some other change made months ago (thus doesn't "in addition" break anything), and reverting this is as simple as a I don't personally use the minimal configurations and have no strong opinion on whether they're actually useful to developers. Of course having more configurations lead to additional maintenance burden and spent computing power (such as the maintenance cost needed now to make the minimal configuration build again), the cost may not be justified by the usefulness to developers/users. Anyway, since the intention of this PR is to temporarily disable the run until the bug is fixed, can't we just comment it out? I mean, we're all volunteers, if anyone volunteers to fix the minimal configurations then of course that will supersede this pull request and everyone are happy [1]… but if nobody does, the feature unfortunately will remain broken. [1] maybe except those who prefer the minimal configurations to be deleted anyway, but I think the usual workflow is deleting a feature requires 1-year deprecation period and/or announcement of some sort. |
|
One doesn't need a deprecation period to remove broken code, and minimal configurations are
Why do we keep killing the project this way, just because few people find these minimal configs interesting or something? |
Did you see the section of the developer guide that I pointed out? The removed chunks of ci-linux are responsible to push various docker images. One of the image, ubuntu-jammy-standard, is used in the CI check workflows for PRs. This PR will collapse the whole CI checks. Other images, ubuntu-jammy-minimal-with-targets and ubuntu-jammy-minimal-with-system-packages, are used in sage-binder-env. This PR will collapse it and the interactive sage doc that is using it. |
|
Can we not escalate this, thanks. Anyway, I think a good compromise is
Do you think this is reasonable? (anyway I'm not a maintainer and would be happy with anything) p/s. @kwankyu , I don't really completely understand what's going on, but if the particular CI has been failing for months, how come the incremental things etc. still work (this is |
| # Make sure that all "standard" jobs can start simultaneously, | ||
| # so that runners are available by the time that "default" starts. | ||
| max_parallel: 50 | ||
|
|
There was a problem hiding this comment.
Why delete this? What is broken?
| fi; \\ | ||
| rm -rf /sage/src; \\ | ||
| mv src /sage/src; \\ | ||
| cd /sage && ./bootstrap && ./config.status; \\ |
There was a problem hiding this comment.
By the way what is this part supposed to do? As I read retrofit-worktree does
echo >&2 "usage: $0 WORKTREE_NAME WORKTREE_DIRECTORY"
echo >&2 "Ensures that the current working directory is a git repository,"
echo >&2 "then makes WORKTREE_DIRECTORY a git worktree named WORKTREE_NAME."
but I don't understand why. Your commit would make sense if you assume the retrofit-worktree always fail (but why would it?)
| max_parallel: 8 | ||
|
|
||
| minimal: | ||
| if: ${{ success() || failure() }} |
There was a problem hiding this comment.
Why delete this?
There are broken platforms. We may selectively turn them off until fixed.
There was a problem hiding this comment.
"gentoo-python3.10" is dead, as Gentoo doesn't ship python 3.10 any more (and 3.11 will soon be gone, too)
There was a problem hiding this comment.
Why delete this?
See PR description.
| maximal-pre: | ||
| if: ${{ success() || failure() }} | ||
| needs: [minimal] | ||
| uses: ./.github/workflows/docker.yml |
There was a problem hiding this comment.
Why delete this?
There are broken platforms. We may selectively turn them off until fixed.
There was a problem hiding this comment.
Why delete this?
This is not deleted, only renamed to maximal.
|
|
||
| optional: | ||
| if: ${{ success() || failure() }} | ||
| needs: [maximal-pre] |
There was a problem hiding this comment.
Why delete this?
We may turn this off until fixed.
There was a problem hiding this comment.
Why delete this?
We may turn this off until fixed.
You cannot turn single steps off as far as I know. And all of them are currently failing.
| targets_optional: '$(echo $(export PATH=build/bin:$PATH && sage-package list :optional: --has-file "spkg-install.in|spkg-install|requirements.txt" --no-file "huge|has_nonfree_dependencies" | grep -v sagemath_doc | grep -v ^_))' | ||
|
|
||
| experimental: | ||
| if: ${{ success() || failure() }} |
There was a problem hiding this comment.
Why delete this?
We may turn this off until fixed.
|
|
||
| needs: [dist] | ||
|
|
||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
Is this also failing? I cannot find a run.
There was a problem hiding this comment.
Yes, all of them are failing see eg https://github.com/sagemath/sage/actions/runs/12979684206/job/36196002577
No.
CI-linux runs portability tests on supported platforms. As you can see in https://github.com/sagemath/sage/actions/runs/12979684199 some jobs like "standard" fail for some platforms while some other jobs like "optional" fail for all patforms. We should selectively turn them off until fixed, not remove the jobs.
because the "default" job running tests on ubuntu-jammy-standard succeeds, its docker image is pushed, and the incremental things are using the docker image to test PRs incrementally.
Yes. |
|
|
|
Sorry. They are irrelevant questions. The point is that those docker images have been a part of our infrastructure. Removing the infrastructure should not be done without community-wide discussion. |
No, the point it that there is no need to use these particular images. Standard images would do just as well. Please don't tell me that we must keep this |
That's true, but I think that isn't in the scope of this pull request. If you want to migrate the generation of the docker images to a standard image you can open a separate one (which sounds like a good thing because the docker image is available more quickly, right?) For this pull request, @kwankyu 's argument is we should not take advantage of the failing runs to also remove the successful runs. Whether -minimal should be removed can be discussed later. |
Please don't tell me that I chose minimal images just out of my frivolity. For Binder, creating lightest image was my priority. For the scope of this PR: I agree that portability-test jobs that fails on all platforms should stop wasting energy until fixed. That could be achieved by adding some "if: false" conditions and commenting out and slight refactoring. Removing code is not a way to disable things temporarily. |
| "archlinux-latest", | ||
| "opensuse-15.5-gcc_11-python3.11", | ||
| "opensuse-tumbleweed-python3.10", | ||
| "opensuse-tumbleweed", | ||
| "conda-forge-python3.11", | ||
| "ubuntu-bionic-gcc_8-i386", | ||
| "debian-bullseye-i386", | ||
| ] | ||
| # 'tox -e update_docker_platforms' updates above | ||
| tox_packages_factors: |
There was a problem hiding this comment.
See the comment at L52. The main list is in tox.ini.
There was a problem hiding this comment.
I only changed the ci flows on purpose so that people trying to fix these other systems have an easy way to do this via tox/dev container. But I can completely remove them everywhere if you don't think that anyone will actively work on them.
please don't try to justify the need for default (!) minimal configurations this way. You could have looked at a more lightweight distro such as Gentoo. Not screwing users and developers over with them has a clear priority over a slightly bigger binder image. I am sick and tired of the constant pushback against bringing any normality to the project by removing this minimal nonsense. |
Thanks, I indeed forgot to change this. Fixed now.
The point of these docker images according to https://doc-release--sagemath.netlify.app/html/en/developer/portability_testing#using-our-pre-built-docker-images-published-on-ghcr-io is to "make[] it easy for developers to debug problems that showed up in the build logs for a given platform." You should use the official docker image and not these portability images for anything else. (Happy to review a PR that creates an official "minimal" docker images that is optimized for space; but note that "minimal" here refers to a completely different "minimal" than in the portability images. As Dima said, you probably want to choose a different base image than ubuntu in this case.) In either case, the old docker images are not deleted so everything should continue working; it's just that you don't get a new image.
I don't think anything on there is broken on there. Could you please be more specific what you mean? I'm adding a sentence now that clarifies that some images are no longer pushed due to various errors. |
Let's compare the ubuntu-jammy-minimal as an internal python variable prefixed with an underline. No one can tell me that I should not use the internal python variable if that fits my need. The internal python variable can be removed for their own need, and then, of course, I cannot complain. Back to the present PR: I object to removing "-minimal" images. I didn't agree with Dima about his arguments for removing them. I think it is a sound idea of Matthias that sage is tested for "minimal", "standard", "maximal" environment. (For the audience, "minimal" means the minimal set of system packages installed before building sage.) They are "stress" tests to keep sage robust in diverse user platforms. Moreover the "-minimal" images are useful for Binder. Digression: The testing machine (consisting of ci-linux, ci-macos, tox, etc.) was maintained by Matthias. He is temporarily inactive. Apparently no one understand and can maintain the machine like him. So the machine is crumbling. Then what is a reasonable action in this situation? Fix the broken parts as far as we can. Turn off malfunctioning parts. But do not trash the broken parts.
For example, the section mentions "-minimal" images. If there are actually no "-minimal" images available, then the documentation is "broken". |
Why don't you create a PR to remove the "minimal nonsense"? I would object, but if other developers agree, then you can achieve your welfare through normal procedure. |
I don't want to do this precisely because I am sick and tired of this, not the least of your insistence on solving technical issues by vote. Because it's not normal procedure, and I think everyone who participated in the recent, unfinished, discussion on Sage project governance models agrees with this. This thing trigger a PTSD for me. And these minimal configurations also don't work at all, or don't work well, it's very far from being a good idea--- they are mostly broken, leading to a broken CI. |
I don't think he's coming back, and if he comes back and starts on his tricks again, I and I am sure many others will leave.
Fine, this is not hard to fix, I am sure. |
So @dimpase , hypothetically if someone comes forward and fix the thing to a working state, would you be okay with merging it? (I believe yes, as it would be a reasonable answer in this case.) |
There are profound reasons for it being very hard, if not just impossible.
But the main point is that maintaining something that is only ever used in a few rather esoteric cases, if at all, is not worth it, and we don't have resources to do it properly (as we see...). These minimal configs are basically leftovers from a mostly unsuccessful software engineering experiment, why do we need to be saddled with them ? |
indeed… as much as I want to avoid heated discussions, it would be too much unpaid work if I were to try to fix the thing to defuse the issues here.
Evidently @kwankyu disagree that minimal test are useless, and this disagreement seems unsolvable, I think it would be more helpful for your case to show none of the working runs are removed instead.
I agree, but commenting out achieves the same effect; and also leaves indication for someone reading the code that the feature was there. I think this can be compared to marking a test case as side note, why is test-new failing? (is it just a temporary failure because of network condition or is it caused by this pull request?) |
Indeed, and as an active developer with by far the most current package updates on my name, I don't like people who do much less of this kind of work -- directly related to the unhealthy number of packages we vendor -- to tell me to keep up with it. My current job title is "research professor", in a CS department. I don't see how this sort of work is any kind of research, at a professorial level. Wearing my CS prof hood, I am saying that it's wrong from the CS/IT point of view to keep doing this kind of minimal configurations. |
|
I am very sorry that this PR triggered such an emotionally involved discussion. Perhaps it's best if everyone involved goes out for a walk, has a relaxed dinner with their partner or friends, or does something else to reset their mind. Then we can continue our discussion in a few days, hopefully with fresh eyes and more understanding for each other. See you then! |
|
@tobiasdiez - before I forgot, please remove "fedora-30" from the list of platforms to test on. Fedora 30 reached EOL 5 years ago, it's pretty much impossible to get new Sage to build on it, and there is no point in this test anyway |
Many of the CI runs after a new release are failing, for months now, see eg https://github.com/sagemath/sage/actions/runs/12979684199/job/36218126145. Some of these failures are genuine (eg a certain package cannot be built on a certain system) and some others are due to constraints of the build system (eg running out of free space). Since there is very little point in senselessly burning energy, all runs that were failing for the last releases are disabled. Once the underlying issues are fixed, they can be easily be re-enabled.
Moreover, the "minimal" runs where only a couple of systems packages are installed and most are build using sage are removed, keeping only the "maximal" where all available system packages are installed.
New test run: https://github.com/tobiasdiez/sage/actions/runs/13199372232/job/36847711005
📝 Checklist
⌛ Dependencies