Fix/ignore all remaining test failures when run under conda

[tobiasdiez]

Add a skip_conda flag that skips the test when executed in an active conda environment. This is used to disable the remaining handful of tests that are still failing under linux in combination with #35593 (refs #35597).

📝 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

• Fix conda install instructions after setuptools update #36411 (as it fixes some of the tests)
• Fix mamba install in conda ci workflow #36468 (as it fixes the conda setup)

[mkoeppe]

I haven't looked at all the details, but many of the changes just disable the brial (polybori) tests.
So it would be cleaner to just disable the feature sage.rings.polynomial.pbori in the conda CI.

This can be done, for example, by passing the switch --hide sage.rings.polynomial.pbori to the doctester.

[tobiasdiez]

It's only the groebner basis method that fails, and only sometimes / for certain inputs.

[mkoeppe]

I know. We have seen these failures on other platforms too. It's not conda-specific.

[tobiasdiez]

What work is needed here? Even if pbori is demoted to experimental, these tests would fail in a local conda install (with optional features enabled) creating unnecessary noise.

[mkoeppe]

What work is needed here? Even if pbori is demoted to experimental, these tests would fail in a local conda install (with optional features enabled) creating unnecessary noise.

Yes, when broken packages are used, they show up as doctest failures unless they are marked as "known bug".

But that these doctests are failing is not conda-specific, so it is not appropriate to mark them as conda-specific "skips". We are supporting lots of platforms and ways to install Sage.

[tobiasdiez]

So I should mark these pbori lines as "known bug" instead?

[mkoeppe]

Yes, please, either known bug or not tested. You can add a reason/reference in parentheses.

This will solve the problem also for homebrew, where we have been seeing these failures for a long time.

[tobiasdiez]

Seems like the conda distribution of brial is working since yesterday with conda-forge/brial-feedstock#33. So I've removed the skips for this again.

[tobiasdiez]

Can we please get this in to make the conda workflow pass?

[mkoeppe]

I don't think this approach (skip_conda) is suitable for merging.

Note that we do not have skip_gentoo, skip_arch, skip_debian, ... directives -- for a good reason.

If there's something wrong with a particular doctest, that's a bug that needs to be analyzed and fixed properly instead of hiding it.

[mkoeppe]

I'll remove the "blocker" label because the changes here (including codestyle fixes for ordering of imports??) are too unfocused for a CI fix. More restraint is needed when preparing an automatically applied CI fix.

[mkoeppe]

We may think that known bug is for definite failures that occur on all platforms

I think it's a design flaw that these are effectively never tested.

[vbraun]

Instead of parens I'd suggest colon to limit the scope of the magic comment, this is an already-existing Python linter convention

x = 1+2  # noqa: E226

so why not

sage: foo()    # known bug: conda

[tobiasdiez]

Instead of skip_conda which is an optional tag specific to conda and which skips the test entirely, how about known_failure tag complemented with conda in parentheses.

Thanks for this constructive proposal. I had a similar idea yesterday after realizing that the remaining failures are most likely not conda specific anymore. It's just that the conda tests got more attention recently and are run more often. In particular there were no regular checks on any macos system before, and almost all of the random failures are macos specific.

So I'm in favor of replacing the "skip_conda" annotation by a "known_failure" annotation that skips the test on all platforms (or "known_failure: macos/linux" if you prefer). This could also be reused for the few other platform-specific failing tests that are currently annotated as "random". Should I go ahead and implement it?

But known_failure tag will not actually skip the test unlike your "skip_conda". Instead Matthias' baseline-stats code may monitor the test results and output useful report and decide to exit with error code 1 or 0.

I'm not sure if I completely follow here. Wouldn't this require to change the semantics of the "baseline-stats" (failing tests in the files listed there should still result in an error code unless they are annotated with "known_failure")? This is not possible as the baseline is also used to hide a lot of errors in the modularization setting.

You cannot write much into the supplemental comment, unless you want to clutter the source file with long comments only targeted to developers. The audience of the entire tag is basically the users. On the other hand, the baseline-stats file can contain longer comments about possible failures of the file that contains the failing doctests.

In my opinion, the comments should indicate enough information that users see that there are known problems with a given function. Everything more (like links to github action runs) should just go into an issue. Something like known_failure: random TimeoutError on MacOS #3333 seems short enough. So it seems overly complicated to have the baseline just to add a bit more details.

[mkoeppe]

Tobias, stop the nonsense with the labels.

[orlitzky]

This one stands out to me because I wrote random_cone(). I don't think this example should time out. All we have to do here is generate a subspace (or even half-space) of dimension 5 or more, which is pretty easy to do by generating a bunch of random vectors and adding them to the list of generators one at a time. You have to be very unlucky for it to not be "instantaneous."

[tobiasdiez]

See eg https://github.com/sagemath/sage/actions/runs/6827937663/job/18571052628#step:11:6663

[mkoeppe]

Instead of parens I'd suggest colon to limit the scope of the magic comment, this is an already-existing Python linter convention

Note that the syntax with parentheses is already in use in the handling of the # optional, # needs tags (both parsing and unparsing).

[kwankyu]

So I'm in favor of replacing the "skip_conda" annotation by a

"known_failure" annotation that skips the test on all platforms (or "known_failure: macos/linux" if you prefer). This could also be reused for the few other platform-specific failing tests that are currently annotated as "random". Should I go ahead and implement it? Thanks. Others seem to prefer using “known bug” for this purpose. We may spend some time for bikeshedding since we would use this term forever. Also Volker suggested colon for separating supplemental comment but parentheses have been used widely for other tags. With colon, it is hard to determine where the content ends. But known_failure tag will not actually skip the test unlike your

"skip_conda". Instead Matthias' baseline-stats code may monitor the test results and output useful report and decide to exit with error code 1 or 0. I'm not sure if I completely follow here. Wouldn't this require to change the semantics of the "baseline-stats" (failing tests in the files listed there should still result in an error code unless they are annotated with "known_failure")? This is not possible as the baseline is also used to hide a lot of errors in the modularization setting.

Yes, perhaps. Details should be thought out and discussed. The baseline-stats may behave differently depending on the tag and the content of the supplemental comment.

…

[tobiasdiez]

I've opened #36960 now for this.

[tobiasdiez]

This is no longer needed.