CI conda: Ignore baseline test failures #36678

mkoeppe · 2023-11-08T21:50:09Z

This makes the CI conda pass by marking affected files as "known baseline failures", thus reducing the noise that this workflow makes on PRs (including #36666 (comment))

Less intrusive version of #36372.

In addition to the failure marked there, here we collect a number of more failures observed over the course of a week in https://github.com/sagemath/sage/actions/workflows/ci-conda.yml?query=is%3Acompleted

📝 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

tobiasdiez

There are only a couple of test failures in these files. These ignore patterns are way to coarse.

(removing blocker label for that reason to not hide possible issues, and because it's not working anyway in this context)

mkoeppe · 2023-11-09T01:38:40Z

because it's not working anyway in this context

It's working as designed. Green checkmarks. Failures marked as "failed in baseline"
https://github.com/sagemath/sage/actions/runs/6804955526/job/18503541137?pr=36678#step:11:10205

tobiasdiez · 2023-11-09T01:42:58Z

So if another new test in the same file fails, the conda ci is red?

mkoeppe · 2023-11-09T02:17:40Z

Perhaps you should clarify what you think the purpose of the ci-conda workflows is.
To my understanding they are being run so that the major breakage that we have seen in the past (from unmonitored package upgrades in conda-forge) does not go unnoticed.
I don't think it's really the job of this workflow to duplicate the test coverage done by build.yml

Marking these files as baseline failures is done so that on unrelated PRs, the red checkmarks on the conda workflows do not annoy developers.

mkoeppe · 2023-11-09T02:20:13Z

And obviously work is needed to actually diagnose, report, and fix these failures. The approach of #36372 ("skip_conda") hides these failures too well.

tobiasdiez · 2023-11-09T20:32:35Z

Perhaps you should clarify what you think the purpose of the ci-conda workflows is. To my understanding they are being run so that the major breakage that we have seen in the past (from unmonitored package upgrades in conda-forge) does not go unnoticed. I don't think it's really the job of this workflow to duplicate the test coverage done by build.yml

They have the same purpose as the Build & Test workflow for people using the conda environment. So yes, they should duplicate the test coverage.

(In the long term, once the conda runs are a bit more stable and there are no more conda-specific issues, we can remove the Build & Test workflow).

kwankyu · 2023-11-10T00:47:59Z

They have the same purpose as the Build & Test workflow for people using the conda environment. So yes, they should duplicate the test coverage.

B&T is mainly for checking sage library changes to help developers and reviewers. It is unlikely that some code change in the sage library works on a platform and fails on another. That is why B&T runs only on a specific decent platform and tests on all other platforms are done only for weekly release.

I do not use conda (linux neither). I regard conda as another platform. So it looks strange to me that currently 6 B&T workflows for conda platforms run for each PR. I think there is already too much duplication.

Perhaps conda is special, and separate B&T for conda is necessary for PRs (perhaps upgrading packages). Then why not just two B&T for conda (one for ubuntu and one for mac) for PRs and more tests for weekly release?

About #36372, I object to introducing conda-specific doctest tags into sage library. That is another cluttering of sage library, in addition to all doctest tags for modularization. We decided to live with modularization. But no platform-specific doctest tags, please.

mkoeppe · 2023-11-10T18:10:23Z

it looks strange to me that currently 6 B&T workflows for conda platforms run for each PR. I think there is already too much duplication.

FWIW, I approved the change to run these jobs on all PRs in #36373. Tobias has already implemented a mitigation (see #36373 (comment)) there on my request by making it "fail-fast" and setting a max-parallel 2.

But I think at least the macOS jobs need reconsideration / additional adjustment. Given that we only have 5 macOS runners, launching 3 jobs (each 1 hour) for every PR is clearly too much.

For example, at the time that 10.2.rc1 was released (over an 1 hour ago), at least 7 "ci-conda" workflows were already in the queue (https://github.com/sagemath/sage/actions/workflows/ci-conda.yml?query=is%3Aqueued), with at least 14 macOS jobs waiting for a runner. As a result, the CI-macos run of 10.2.rc1 (https://github.com/sagemath/sage/actions/runs/6827163283) as well as the cibuildwheel job for macOS (https://github.com/sagemath/sage/actions/runs/6827163257/job/18568938031) have not been able to start yet.

No work items

mkoeppe · 2023-11-11T22:20:14Z

I think at least the macOS jobs need reconsideration / additional adjustment. Given that we only have 5 macOS runners, launching 3 jobs (each 1 hour) for every PR is clearly too much.

Implemented in #36694

tobiasdiez · 2023-12-03T06:29:17Z

[...] If you know the exact test that fails [...]

... then you open an Issue to report it.

There is no need for adding a record of the exact failing test to the source code.

That example was for the situation where I have 15min extra time and want to use that time to fix a conda bug. With your solution here, I already may need 2 hours to just find out which test is actually failing.

Another example: #36372 (comment) it was very easy to recognize that another PR fixed a certain failure on conda because the info was there directly in the code. With the coarse ignore rules proposed in this PR here, that would have been not that easy. (That also brings up the question who is maintaining the exclusion list, and how?)

mkoeppe · 2023-12-03T06:40:49Z

I don't think this adds anything to the discussion here. I'll explain again:

Test failure is noticed
You record it in a GitHub Issue. There you include the necessary information so that it does not take 2 hours to reconstruct it.
One adds stuff to the exclusion list to eliminate noise in the tests. It's not for humans to read, and definitely not for humans to use 2 hours of guessing time to reconstruct the failure.
The exclusion list is maintained like everything else in Sage -- by PRs, not by specific people in charge.

kwankyu · 2023-12-03T07:07:55Z

Test failure is noticed

You record it in a GitHub Issue. There you include the necessary information so that it does not take 2 hours to reconstruct it.

One adds stuff to the exclusion list to eliminate noise in the tests.

and items in the exclusion list are commented with the the GitHub Issue numbers...

kwankyu

LGTM.

tobiasdiez · 2023-12-08T16:08:19Z

Matthias, can you please just stop adding "positive review" labels on your PRs. Thanks!

Labeling it as "r: invalid" and "s: needs review" as a motion to close this PR. There are clear short comings with the approach of this PR as described above.

mkoeppe · 2023-12-08T16:30:48Z

There are clear short comings with the approach of this PR as described above.

Nope, all your claims have been refuted.

This reverts commit 0a69c2a, reversing changes made to 2017233.

…nd 1 Linux job    as discussed in sagemath#36678 (comment), sagemath#36616 (comment) On pushes to a tag, all 3 Linux and 3 macOS jobs are still run, as demonstrated in https://github.com/mkoeppe/sage/actions/runs/6829619271   After merging, the branch protection rule https://github.com/sagemath/sa ge/settings/branch_protection_rules/33965567 will need to be updated. It controls what workflows show as "Required" in the PR Checks. ### 📝 Checklist     - [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   URL: sagemath#36694 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee, Tobias Diez

mkoeppe self-assigned this Nov 8, 2023

mkoeppe added the c: scripts label Nov 8, 2023

mkoeppe force-pushed the ci_conda_baseline branch from c799e46 to a6a2408 Compare November 8, 2023 22:52

mkoeppe added s: needs review p: blocker / 1 labels Nov 8, 2023

tobiasdiez previously requested changes Nov 9, 2023

View reviewed changes

tobiasdiez added s: needs work and removed s: needs review p: blocker / 1 labels Nov 9, 2023

mkoeppe added s: needs review and removed s: needs work labels Nov 9, 2023

mkoeppe added the p: blocker / 1 label Nov 9, 2023

mkoeppe added this to the sage-10.2 milestone Nov 9, 2023

mkoeppe requested a review from kwankyu November 9, 2023 18:06

tobiasdiez added s: needs work and removed s: needs review p: blocker / 1 labels Nov 9, 2023

mkoeppe added s: needs review and removed s: needs work labels Nov 9, 2023

mkoeppe force-pushed the ci_conda_baseline branch from a6a2408 to 0a5d4a3 Compare November 10, 2023 17:20

vbraun force-pushed the develop branch from 429555a to e349b00 Compare November 12, 2023 02:08

kwankyu approved these changes Dec 3, 2023

View reviewed changes

mkoeppe removed this from the sage-10.2 milestone Dec 3, 2023

mkoeppe requested a review from tornaria December 4, 2023 04:50

mkoeppe added s: positive review and removed s: needs review labels Dec 6, 2023

tobiasdiez added r: invalid s: needs review and removed s: positive review labels Dec 8, 2023

mkoeppe removed the r: invalid label Dec 8, 2023

kwankyu added s: positive review and removed s: needs review labels Dec 13, 2023

tobiasdiez removed the s: positive review label Dec 18, 2023

mkoeppe added the s: positive review label Dec 18, 2023

vbraun merged commit 0a69c2a into sagemath:develop Dec 19, 2023
33 checks passed

mkoeppe added this to the sage-10.3 milestone Dec 19, 2023

github-actions bot removed the s: positive review label Dec 19, 2023

tobiasdiez added a commit to tobiasdiez/sage that referenced this pull request Dec 19, 2023

Revert "sagemathgh-36678: CI conda: Ignore baseline test failures "

29901c4

This reverts commit 0a69c2a, reversing changes made to 2017233.

tobiasdiez mentioned this pull request Dec 19, 2023

Revert "gh-36678: CI conda: Ignore baseline test failures " #36923

Closed

5 tasks

mkoeppe deleted the ci_conda_baseline branch December 19, 2023 16:40

tobiasdiez mentioned this pull request Dec 23, 2023

CI Linux: Replace use of pkill #36726

Merged

5 tasks

mkoeppe added the disputed PR is waiting for community vote, see https://groups.google.com/g/sage-devel/c/IgBYUJl33SQ label Dec 23, 2023

This was referenced Dec 25, 2023

Introduce os-dependent known bugs annotation tobiasdiez/sage#7

Closed

Introduce os-dependent doctest tags # known bug: macos, # known bug: linux #36960

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CI conda: Ignore baseline test failures #36678

CI conda: Ignore baseline test failures #36678

mkoeppe commented Nov 8, 2023 •

edited

Loading

tobiasdiez left a comment •

edited

Loading

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023 •

edited

Loading

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

kwankyu commented Nov 10, 2023 •

edited

Loading

mkoeppe commented Nov 10, 2023 •

edited

Loading

mkoeppe commented Nov 11, 2023

tobiasdiez commented Dec 3, 2023

mkoeppe commented Dec 3, 2023 •

edited

Loading

kwankyu commented Dec 3, 2023

kwankyu left a comment

tobiasdiez commented Dec 8, 2023 •

edited

Loading

mkoeppe commented Dec 8, 2023 •

edited

Loading

CI conda: Ignore baseline test failures #36678

CI conda: Ignore baseline test failures #36678

Conversation

mkoeppe commented Nov 8, 2023 • edited Loading

📝 Checklist

⌛ Dependencies

tobiasdiez left a comment • edited Loading

Choose a reason for hiding this comment

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

mkoeppe commented Nov 9, 2023 • edited Loading

mkoeppe commented Nov 9, 2023

tobiasdiez commented Nov 9, 2023

kwankyu commented Nov 10, 2023 • edited Loading

mkoeppe commented Nov 10, 2023 • edited Loading

mkoeppe commented Nov 11, 2023

tobiasdiez commented Dec 3, 2023

mkoeppe commented Dec 3, 2023 • edited Loading

kwankyu commented Dec 3, 2023

kwankyu left a comment

Choose a reason for hiding this comment

tobiasdiez commented Dec 8, 2023 • edited Loading

mkoeppe commented Dec 8, 2023 • edited Loading

mkoeppe commented Nov 8, 2023 •

edited

Loading

tobiasdiez left a comment •

edited

Loading

mkoeppe commented Nov 9, 2023 •

edited

Loading

kwankyu commented Nov 10, 2023 •

edited

Loading

mkoeppe commented Nov 10, 2023 •

edited

Loading

mkoeppe commented Dec 3, 2023 •

edited

Loading

tobiasdiez commented Dec 8, 2023 •

edited

Loading

mkoeppe commented Dec 8, 2023 •

edited

Loading