Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CI disable coverage on Windows to keep CI times reasonable #26052

Merged
merged 5 commits into from Apr 2, 2023
Merged

CI disable coverage on Windows to keep CI times reasonable #26052

merged 5 commits into from Apr 2, 2023

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Apr 2, 2023

Now that codecov has correctly collected coverage data on main after the merge of #26027, let see if we can speed-up the Windows CI by disabling coverage collection for this platform without degrading the test coverage statistics since very few code branches should depend on the windows platform in scikit-learn:

git grep sys.platform sklearn
sklearn/_build_utils/openmp_helpers.py:    if sys.platform == "win32":
sklearn/_build_utils/openmp_helpers.py:    elif sys.platform == "darwin" and "openmp" in os.getenv("CPPFLAGS", ""):
sklearn/conftest.py:    elif sys.platform.startswith("win32"):
sklearn/decomposition/tests/test_sparse_pca.py:    if sys.platform == "win32":  # fake parallelism for win32
sklearn/utils/_testing.py:        sys.platform == "darwin", reason="Possible multi-process bug with some BLAS"

@ogrisel
Copy link
Member Author

ogrisel commented Apr 2, 2023

Hum:

@glemaitre
Copy link
Contributor

there is still problems with invalid coverage reports and missing of an HEAD commit:

This happened only in the last 3 pull requests. This might be transient.

@ogrisel
Copy link
Member Author

ogrisel commented Apr 2, 2023

This happened only in the last 3 pull requests. This might be transient.

It might be a consequence of the merge of #26027 although I do not understand why (yet).

@ogrisel
Copy link
Member Author

ogrisel commented Apr 2, 2023

Ok I understand:

https://docs.codecov.com/docs/deprecated-uploader-migration-guide#python-uploader

The raw .coverage is not a text based format but a sqlite database that needs to be converted to XML with coverage xml prior to uploading. Let me do that.

@ogrisel
Copy link
Member Author

ogrisel commented Apr 2, 2023

It works:

Note that on the last run, the windows build was below 25 min and the overall coverage is still good (97.21%). So I think we should remove the coverage collection on the windows build to keep more in line with the others (I saw some runs above 45 min in the past for the windows job). The 32 bit linux run is now the slowest but I think we should keep it because it will be able to cover branches that no other build can cover and this will be important as a proxy for WASM platform support.

I will clean-up the debug prints.

@ogrisel ogrisel marked this pull request as ready for review April 2, 2023 14:05
@ogrisel ogrisel enabled auto-merge (squash) April 2, 2023 14:05
@ogrisel
Copy link
Member Author

ogrisel commented Apr 2, 2023

I will merge ASAP because coverage is broken on all PRs at the moment.

@ogrisel ogrisel disabled auto-merge April 2, 2023 14:58
@ogrisel ogrisel merged commit 1ac1073 into scikit-learn:main Apr 2, 2023
26 checks passed
@ogrisel ogrisel deleted the ci-disable-windows-coverage branch April 2, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Build / CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ogrisel @glemaitre