[MNT] remove coverage reporting and pytest-cov from PR CI and setup.cfg

[fkiraly]

This PR removes generation of coverage reports, installation and use of pytest-cov from standard CI. Also removes the (unreliable) coverage badge from the README

Reasons:

• generation of coverage reports seems to make CI take substantially more time: [BUG] long test collection time and test timeouts #6344
• coverage reports are unreliable (spuriously low) due to incremental/differential testing: [MNT] ensuring correct coverage from incremental testing #5090
• in consequence, the README coverage badge showed randomly fluctuating coverage depending on which set of partial tests were executed. I think it makes sense to remove it until we have a way to address [MNT] ensuring correct coverage from incremental testing #5090.

[fkiraly]

Anecdotal, but looks like this leads to substantial runtime improvements.

Before:

After:

[yarnabrina]

@fkiraly I want to be catious for this. Can we test these:

1. what happens if instead of removing completely we only skip the xml and html reports? I believe a base one (.coverage) gets generated always, and these two run separately.
2. if it is removed completely (assuming only for CI runs in PR), how does coverage report appear in README (after merge to main)? If it shows missing or 0% or similar, that will be misleading. To test, may be you can try to edit the link of README in this branch without actually merging.

My caution is mainly for the reason that it's highly counter intuitive to me that coverage will affect timing by this much. It's more than 3-4 times in your screenshots, and if had that been the general effect of pytest-cov, it's expected to be detected by users quite ago. It's very popular and standard, so I am really wondering if we are missing something else (though I don't have any alternative ideas yet).

[fkiraly]

what happens if instead of removing completely we only skip the xml and html reports? I believe a base one (.coverage) gets generated always, and these two run separately.

According to the profiler, indeed these parts create the overhead.

How would you turn these off separately? Can you help? Would it be removing the --cov-report html etc, but not --cov?

how does coverage report appear in README

This PR also removes the badge from the readme, because it is misleading anyway, with or without this PR.

We should find a way to display genuine coverage in the readme (see #5090) - I would consider that a separate issue (namely, #5090), and it would then include adding the correct coverage display to the readme.

[fkiraly]

so I am really wondering if we are missing something else (though I don't have any alternative ideas yet).

so am I.
A wild guess is that we have some runaway import chains in the style of #6355, which is causing the long runtimes.

Or, perhaps cause/effect are hard to detect in general?

[yarnabrina]

How would you turn these off separately? Can you help? Would it be removing the --cov-report html etc, but not --cov?

Yes, that only. Let's see what happens.

This PR also removes the badge from the readme, because it is misleading anyway, with or without this PR.

I think if README shows 0% or etc. it may give potential users/contributors a negative impression that this framework is untested (e.g. I know I'll feel the same for a new tool).

[fkiraly]

ok - I've added it back in the test-nosoftdeps-full job now, and in the pyproject.toml, to see what happens

[yarnabrina]

Only 5 jobs got triggered, not a single testing job! How did it ran everything earlier?

[fkiraly]

Only 5 jobs got triggered, not a single testing job! How did it ran everything earlier?

I see - I think I understand why the difference.

Previously, pytest-cov was removed from pyproject.toml, and that triggered "test all". Now, we added it back, and pyproject.toml is no longer modified, so there is no trigger for testing anything anymore.

[yarnabrina]

https://github.com/sktime/sktime/actions/runs/8940323391

I triggered a manual test all workflow on this branch for debugging.

[fkiraly]

Thanks.

Sth is taking hours again, how do we find out with which estimator it gets stuck?

[yarnabrina]

I am not aware of a better solution than going into verbose mode.

By the way, have you seen the failures? It seems every single "other" run failed with this:

FAILED sktime/tests/tests/test_test_utils.py::test_run_test_for_class - AssertionError: assert 'True_run_always' in ['True_pyproject_change', 'True_changed_class', 'True_changed_tests']

[fkiraly]

By the way, have you seen the failures? It seems every single "other" run failed with this:

Thanks for pointing this out - this is a bug with a test I added to make sure we test the run_test_for_class utility.

The bug surfaces only in the test_all workflow which has a certain combination of conditions. Fix here:
#6383

[yarnabrina]

I checked other jobs, and so far no timeout failure. Only one module job failed and it's for forecasting:

FAILED sktime/forecasting/model_evaluation/tests/test_evaluate.py::test_evaluate_common_configs[backend8-scoring1-refit-1-10-fh5-ExpandingWindowSplitter] - OverflowError: Python int too large to convert to C long

Ref. https://github.com/sktime/sktime/actions/runs/8940323391/job/24558260007#step:3:6594

Any idea if it's sporadic? We'll probably know from random seed diagnostic.

(FYI @benHeid )

[fkiraly]

FAILED sktime/forecasting/model_evaluation/tests/test_evaluate.py::test_evaluate_common_configs[backend8-scoring1-refit-1-10-fh5-ExpandingWindowSplitter] - OverflowError: Python int too large to convert to C long

This is definitely a new one - have not seen this before.

However, there have been failures in test_evaluate_common_configs in ancient times, but these seem unrelated?
#1194

We'll probably know from random seed diagnostic.

Probably not, as that one does not add random seeds except in TestAllForecasters, the test_evaluate_common_configs lives elsewhere.

[Abhay-Lejith]

@fkiraly @yarnabrina , mentioning this here as it might be related.
I was not able to make breakpoints work in my python debugger in vscode up until now.
I've managed to fix it by essentially disabling pytest-cov ( or something of that effect ) by modifying my launch.json and adding the following field .
"env": {"PYTEST_ADDOPTS": "--no-cov"}
It appears that pytest-cov somehow interferes with the debugger. More info on it in the "Note section" here: https://code.visualstudio.com/docs/python/testing#_pytest-configuration-settings

[yarnabrina]

@fkiraly I think you use vs code and use the integrated debugging? Did you ever face the issue @Abhay-Lejith mentioned?

I definitely face a lot of issues in our current setup.cfg, so I have a local patch to ignore that file altogether, essentially doing what @Abhay-Lejith has done with VS Cide settings so never faced this issue myself. If this is indeed an issue, as the documentation seem to suggest, I expect this to be a very common thing, and am wondering why no one ever reported it before? 😕

[fkiraly]

@fkiraly I think you use vs code and use the integrated debugging? Did you ever face the issue @Abhay-Lejith mentioned?

Yes, the GUI integrated breakpoint debugging never worked, so I ended up adopting a more manual workflow and also ignoring the file in practice.

I applaud @Abhay-Lejith to having finally identified the reason.

I expect this to be a very common thing, and am wondering why no one ever reported it before?

Perhaps groupthink bias, i.e., everyone thinks it works for everyone else and they would be considered stupid if they raise it in public? Nothing further from the truth, but sometimes it is how the mind works.

Shall we remove the flags then, it seems to cause problems systematically?

[yarnabrina]

If we are disabling coverage everywhere, should we drop them from test dependencies too?

[fkiraly]

we should probably have some replacement plan in mind, e.g., where and when we run coverage. On the full test run?