CI Add summary about failures and errors in most builds by lesteve · Pull Request #26847 · scikit-learn/scikit-learn

lesteve · 2023-07-17T15:20:52Z

I would find it very useful to have the default pytest summary for failures and errors. This allows to go to the end of the CI log and have at a quick glance an idea where things are broken and whether the same root cause is causing all the errors (although the limit is width is not that helpful sometimes oh well ...). Right now if I want to have an idea of which test are broken, I tend to look test_ in the CI log which is not very efficient given CI log verbosity.

Maybe I am biased because I have looked at broken scipy-dev builds recently, where there are 100+ errors currently. I think it would be useful generally speaking.

The -rN was added in #21554, I guess because -ra was too long which makes sense but having "errors + failures" feels like a better default.

I also renamed SHOW_SHORT_SUMMARY => SHOW_LONG_SUMMARY, since -ra is all except passed and is a lot more than the default summary "errors + failures"

github-actions · 2023-07-17T15:22:50Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: bec5151. Link to the linter CI: here}

betatim · 2023-07-17T15:51:04Z

Is there an easy way to create (locally) the output with and without this PR? I tried to read about -r and found "-r chars Show extra test summary info as specified by chars: (f)ailed, (E)rror, (s)kipped, (x)failed, (X)passed, (p)assed, (P)assed with output, (a)ll except passed (p/P), or (A)ll. (w)arnings are enabled by default (see --disable-warnings), 'N' can be used to reset the list. (default: 'fE')." - but I am not sure I understand what "reset the list" means, does it mean "equivalent to the default" (in this case fE)?

So in the end I was wondering if we can post the main and "this PR" versions, or link to them (the CI output is indeed massive).

ps. I find navigating the CI output pretty tedious. It involves a lot of scrolling and skipping over "visual noise", so if this improves that, I'd be a happy man :D

lesteve · 2023-07-17T17:22:24Z

AFAIU -rN means no summary (reset here seems to mean set it to the empty list).

You can also reset in this sense (a potential use case is if you get -ra argument from a command line tool you don't control and you want to first reset it to then add the warnings you want, since -r is only additive):

# This reset the -ra and only shows failures (f) and (E)
pytest test.py --disable-warnings -ra -rNfE

With this PR you will get one line per failure or error, something like this at the end of the pytest output:

======================== short test summary info =========================
FAILED test.py::test2 - RuntimeError: Oh it looks like something went wrong here and now I am...

You can try locally passing pytest -rfE this is what this PR is doing.

Currently with main you don't get any summary info in most builds except one see this build for example and scroll to the bottom to see -ra build showing all but passed.

ps. I find navigating the CI output pretty tedious. It involves a lot of scrolling and skipping over "visual noise", so if this improves that, I'd be a happy man :D

I would agree but this PR is not going to help too much with this. Also at the same time it is nice to get all the details we can get to do some post-mortem stuff where you don't manage to reproduce locally. It may be worth spending some time to look at the doc to see if there is some pytest option where you can get a better trade-off to what we have now.

lesteve · 2023-07-18T04:36:35Z

Here is an example build where I find useful to have the summary. I can quickly see that the 6 failures have the same root cause (np.find_common_type deprecated).
The output looks like this (with summary):

=========================== short test summary info ============================
FAILED cluster/tests/test_spectral.py::test_spectral_clustering_with_arpack_amg_solvers - DeprecationWarning: np.find_common_type is deprecated.  Please use `np.resu...
FAILED manifold/tests/test_spectral_embedding.py::test_spectral_embedding_amg_solver[float32] - DeprecationWarning: np.find_common_type is deprecated.  Please use `np.resu...
FAILED manifold/tests/test_spectral_embedding.py::test_spectral_embedding_amg_solver[float64] - DeprecationWarning: np.find_common_type is deprecated.  Please use `np.resu...
FAILED manifold/tests/test_spectral_embedding.py::test_spectral_embedding_amg_solver_failure[float32] - DeprecationWarning: np.find_common_type is deprecated.  Please use `np.resu...
FAILED manifold/tests/test_spectral_embedding.py::test_spectral_embedding_amg_solver_failure[float64] - DeprecationWarning: np.find_common_type is deprecated.  Please use `np.resu...
FAILED manifold/tests/test_spectral_embedding.py::test_spectral_eigen_tol_auto[amg] - DeprecationWarning: np.find_common_type is deprecated.  Please use `np.resu...
= 6 failed, 31301 passed, 968 skipped, 93 xfailed, 47 xpassed, 3204 warnings in 1049.08s (0:17:29) =

The same build without the summary (as in main). I have to scroll or search with some hacky pattern to figure out this is the same root cause.

The output looks like this (no summary):

= 6 failed, 31301 passed, 968 skipped, 93 xfailed, 47 xpassed, 3204 warnings in 1049.08s (0:17:29) =

Another build with 50+ failures and the same build without summary. Same thing you can quickly see there is a single root cause compared to scrolling to look at 50+ tracebacks, this is a huge time saver.

betatim · 2023-07-18T08:44:36Z

Thanks for the examples. Fine with me/I think this makes thinks easier to read and get an overview.

ogrisel · 2023-07-18T13:53:10Z

azure-pipelines.yml

        LOCK_FILE: './build_tools/azure/pylatest_conda_forge_mkl_linux-64_conda.lock'
        COVERAGE: 'true'
-        SHOW_SHORT_SUMMARY: 'true'
+        SHOW_LONG_SUMMARY: 'true'


@thomasjpfan do we really want to display skipped and xfailed/xpassed tests on this build? Why?

Personally I use the test results interactive report in the Azure pipelines web UI to get the list of skipped or xfailed tests.

I think I would be in favor of always hiding the summary of skipped/xfailed tests in CI logs.

I'm okay with removing this option all together.

ogrisel

LGTM. I think we could even always hide the skipped/xfailed tests on all CI configs as commented here: https://github.com/scikit-learn/scikit-learn/pull/26847/files#r1266811477

lesteve · 2023-07-19T15:02:50Z

LGTM. I think we could even always hide the skipped/xfailed tests on all CI configs as commented here: #26847 (files)

I did a little bit of archeology:

skipped tests were added to the pytest summary in TST: display reason of skipping by default with pytest #10946. It seems like the motivation behind the PR is

This can be especially useful to not be surprised why your doctests are skipping because you don't have numpy 1.14.

xfailed and xpassed tests were added to the pytest summary in ENH XFAIL in common tests with estimator tags (v3) #16502
MAINT Removes short summary for most builds #21554 actually used -ra in a single build which adds failed tests and errors to the pytest summary.

In the end, unless someone says that he finds it useful from time to look at skipped and xfailed tests in the CI, I would go for simplicity and remove the special build with the long summary.

lesteve · 2023-07-19T15:10:04Z

As mentioned by Olivier you can see skipped tests from Azure UI ("Tests" tab) if you select "Others", e.g. on this page

I don't think there is an easy way to search for skipped vs xfailed but oh well ...

thomasjpfan · 2023-07-22T01:23:01Z

I would go for simplicity and remove the special build with the long summary.

I am okay with this to simplify the CI.

lesteve · 2023-07-24T12:03:49Z

I am okay with this to simplify the CI.

Done, I believe this can be merged now

thomasjpfan

LGTM

…#26847)

CI Add summary about failures and errors in most builds

cc06f6f

github-actions bot added the Build / CI label Jul 17, 2023

ogrisel reviewed Jul 18, 2023

View reviewed changes

ogrisel approved these changes Jul 18, 2023

View reviewed changes

Remove SHOW_LONG_SUMMARY feature

bec5151

thomasjpfan approved these changes Jul 24, 2023

View reviewed changes

thomasjpfan merged commit d66a384 into scikit-learn:main Jul 24, 2023

punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023

CI Add summary about failures and errors in most builds (scikit-learn…

e07112d

…#26847)

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023

CI Add summary about failures and errors in most builds (scikit-learn…

6b79e66

…#26847)

jeremiedbb pushed a commit that referenced this pull request Sep 20, 2023

CI Add summary about failures and errors in most builds (#26847)

61747ff

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023

CI Add summary about failures and errors in most builds (scikit-learn…

9b4379a

…#26847)

Uh oh!

Conversation

lesteve commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

Uh oh!

betatim commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

betatim commented Jul 18, 2023

Uh oh!

ogrisel Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

thomasjpfan Jul 22, 2023

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

lesteve commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasjpfan commented Jul 22, 2023

Uh oh!

lesteve commented Jul 24, 2023

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lesteve commented Jul 17, 2023 •

edited

Loading

github-actions bot commented Jul 17, 2023 •

edited

Loading

betatim commented Jul 17, 2023 •

edited

Loading

lesteve commented Jul 17, 2023 •

edited

Loading

lesteve commented Jul 18, 2023 •

edited

Loading

lesteve commented Jul 19, 2023 •

edited

Loading

lesteve commented Jul 19, 2023 •

edited

Loading