deepcharles / ruptures

ruptures: change point detection in Python
BSD 2-Clause "Simplified" License
1.56k stars 161 forks source link

ci: add pytest-cov to the pypi gh action #225

Closed deepcharles closed 2 years ago

deepcharles commented 2 years ago

The GH action that releases to PyPI run pytest ruptures/tests (see here) which requires pytest-cov (specified here).

To run the action, pytest-cov must be installed within cibuildwheel. Otherwise it runs into the following error.

codecov[bot] commented 2 years ago

Codecov Report

Merging #225 (b2c9240) into master (9685818) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #225   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files          40       40           
  Lines         978      978           
=======================================
  Hits          966      966           
  Misses         12       12           
Flag Coverage Δ
unittests 98.77% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9685818...b2c9240. Read the comment docs.

deepcharles commented 2 years ago

The GH action that pushes to PyPI remains stuck at the same step everytime: when testing the wheel built for cp38-manylinux_i686. There is no error message (except that the job was cancelled for taking too much time). Despite the verbose flag for pytest, there is no print, meaning that the pytest command is never launched in the first place.

wdyt @oboulant ?

oboulant commented 2 years ago

The GH action that pushes to PyPI remains stuck at the same step everytime: when testing the wheel built for cp38-manylinux_i686. There is no error message (except that the job was cancelled for taking too much time). Despite the verbose flag for pytest, there is no print, meaning that the pytest command is never launched in the first place.

wdyt @oboulant ?

I will have a look next week ! As for the verbose flag, I am not sure the GH action and cibuildwheel make the link with the [tool.pytest.ini_options] in pyproject.toml. The fact that you had to add pytest-cov to CIBW_TEST_REQUIRES: pytest pytest-cov makes me think that it does not make the link with all the package machinery. Have you tried to explicitly add the verbose flab in the GH action ? CIBW_TEST_COMMAND: pytest -vv {project}/tests

deepcharles commented 2 years ago

As for the verbose flag, I am not sure the GH action and cibuildwheel make the link with the [tool.pytest.ini_options] in pyproject.toml. The fact that you had to add pytest-cov to CIBW_TEST_REQUIRES: pytest pytest-cov makes me think that it does not make the link with all the package machinery.

When cibuildwheel executes pytest /project/tests, it runs the coverage, meaning that it reads [tool.pytest.ini_options]. Also, in the last run, the verbose flag was taken into account (see for instance L1428) and pyproject.tompl is listed as the config file (see L1424).

oboulant commented 2 years ago

Actually, I can see the dependency on pyproject.toml but I do not see the verbose option sh -c 'pytest /project/tests' here : https://github.com/deepcharles/ruptures/runs/4794143608?check_suite_focus=true#step:5:6056

oboulant commented 2 years ago

Have you tried to bump CIBW_BUILD_VERBOSITY: 1 ?

deepcharles commented 2 years ago

I do not see the verbose option sh -c 'pytest /project/tests' here : https://github.com/deepcharles/ruptures/runs/4794143608?check_suite_focus=true#step:5:6056

On this particular wheel, nothing is printed meaning that pytest might not be launched at all.

On other wheels (other python versions), the verbose flag does work (see for instance the cp37-manylinux_i686 wheel https://github.com/deepcharles/ruptures/runs/4794143608?check_suite_focus=true#step:5:5199)

deepcharles commented 2 years ago

Have you tried to bump CIBW_BUILD_VERBOSITY: 1 ?

I'll try it now.

deepcharles commented 2 years ago

I did not get anymore information and it remains stuck at the same wheel.

oboulant commented 2 years ago

Let's summarise !

It seems like it is not a pytest problem due to some conflict on how coverage is called with pyproject.toml and what is actually performed within cibuildwheel containers.

The problem is really contained to cibuildwheel. It appears that it stopped working recently for some 32 bits setups. We are not the only ones concerned since scikit-learn had the same issue : https://github.com/scikit-learn/scikit-learn/pull/22122 What we did is then to explicitly exclude the build and test of wheels for cp38-manylinux_i686 and cp39-manylinux_i686. Build and test for cp36-manylinux_i686 and cp37-manylinux_i686 still works so we kept them. Worth noticing that the building per se of the wheels worked, it is just that the tests would not run. I made the choice (to be challenged) that we would not want to push some untested wheels. Another option would be to build the wheels when possible and not to test them with the explicit CIBW_TEST_SKIP for the concerned os x python version.

We also had an issue with the new musllinux supported by cibuildwheel, see https://github.com/pypa/cibuildwheel/releases/tag/v2.2.0 I made the choice (to be challenged) that we would deal with the support of musllinux in the near future and not within the current PR.

Another choice that we could make within this PR (to be challenged) is to add support for python v3.10 now that is has been out for some time now. I planned on trying to build the wheels for python v3.10.

@deepcharles WDYT on those 3 options ?

I will also clean the PR when everything is done on the wheel front (might have some verbosity flags hanging there and I commented for now the upload to pypi step of the GHA so we do not pollute it with rcX versions just for debugging).

oboulant commented 2 years ago

Update : I added python v3.10 both

@deepcharles

deepcharles commented 2 years ago

I made the choice (to be challenged) that we would not want to push some untested wheels.

I am OK with this.

I made the choice (to be challenged) that we would deal with the support of musllinux in the near future and not within the current PR.

OK

deepcharles commented 2 years ago

For this particular PR, I would keep only the bare minimum (adding pytest-cov to the cibuildwheel setup) and have two other PRs for skipping the "bad" wheels and adding support for python310. That way, we can keep track of the changes in the changelog.