migrate from nose to pytest

StrikerRUS commented 3 years ago

https://docs.pytest.org/en/6.2.x/goodpractices.html#tests-as-part-of-application-code

mblondel commented 3 years ago

Thanks! I'm guessing #158 should be merged before this one?

StrikerRUS commented 3 years ago

Given that CI is currently broken for master, I don't think there is any particular preffered order of PRs. Synergy of all of them should lead to green CI checks and actual Python versions.

mblondel commented 3 years ago

Thanks for doing all this! I merged the other PRs. Hopefully, your next commit should trigger a rerun of travis and appveyor and we'll see if everything is green again.

CC @vene

StrikerRUS commented 3 years ago

Hey, Travis is already all-green in master! 🎉

Ran 132 tests in 4.923s

OK

However, Appveyor is misleadingly green with

Ran 0 tests in 0.000s
OK

Refer to https://github.com/scikit-learn-contrib/lightning/pull/158#issuecomment-832264957.

mblondel commented 3 years ago

Thanks! I can't merge this branch yet as it has merge conflicts.

StrikerRUS commented 3 years ago

Current state of this PR:

Travis is all-green and quite happy to migrate to pytest:

======================= 132 passed, 12 warnings in 5.30s =======================

master is also all-green with nose.

All jobs of Appveyor fail with multiple instances of the following error:

lightning\impl\randomkit\__init__.py:1: in <module>
    from .random_fast import RandomState
E   ModuleNotFoundError: No module named 'lightning.impl.randomkit.random_fast'

I googled a lot because have never worked with cython before and it seems that we hit internal pytest bug: https://stackoverflow.com/a/49068163. Tried several workarounds but neither of them worked. And I see only two possible ways to run tests with pytest for this project:

rewrite setup part with suggestions from the linked workaround above (no any guarantee this will help)
drop custom randomkit implementation and use RandomState from numpy: https://github.com/numpy/numpy/issues/7861#issuecomment-497974032, https://numpy.org/devdocs/reference/random/extending.html#cython.

Not sure that any of suggested points is worth to go with.

Appveyor current master with nose:

Python 3.6: success.
Python 3.7 32bit: success.

Python 3.7 64bit: fail with

FAIL: lightning.impl.tests.test_fista.test_fista_regression_simplex
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python37-x64\lib\site-packages\nose\case.py", line 198, in runTest
    self.test(*self.arg)
  File "C:\Python37-x64\lib\site-packages\lightning\impl\tests\test_fista.py", line 137, in test_fista_regression_simplex
    assert np.all(reg.coef_ >= 0)
AssertionError

Python 3.8-3.9: fail to collect any tests:
```
Ran 0 tests in 0.000s
OK
```

StrikerRUS commented 3 years ago

I will really appreciate any advices of possible further direction based on the report above.

mblondel commented 3 years ago

Using RandomState from numpy would be slow I think, as it would force to go through the Python interpreter.

Maybe #123 would help here? @vene, any clue?

Regarding the fista failure, could you try assert np.all(reg.coef_ >= -1e-9) instead?

vene commented 3 years ago

That file was removed recently in another PR because we thought it was supposed to be autogenerated at build time via Cythonize, right? That seems like it's not triggering on Windows?

StrikerRUS commented 3 years ago

That file was removed recently in another PR because we thought it was supposed to be autogenerated at build time via Cythonize, right? That seems like it's not triggering on Windows?

I used RDP to connect to Appveyor runners. With normal Python from CMD RandomState can be imported without any issues. Also, I can't find any difference between Python 3.7 and Python 3.8 installationы in terms of files in the installation directory (for the question why nose imports 0 tests in Python 3.8+ but runs OK with Python 3.7). In addition, I don't see any difference with my local installation of precompiled 0.6.0 version of lightning (released before commit that removed .cpp file) from the conda-forge channel. So I don't think that that file is the reason.

StrikerRUS commented 3 years ago

Regarding the fista failure, could you try assert np.all(reg.coef_ >= -1e-9) instead?

This works! I checked - we can go even further to -1e-12. PR created: #167.

StrikerRUS commented 3 years ago

Got it work without any codebase changes! 🎉

Travis is all-green now with

======================= 132 passed, 11 warnings in 5.40s =======================

Appveyor is all-green now with

======================= 131 passed, 1 warning in 6.30s ========================

The thing was in using python -m pytest ... instead of just pytest .... pytest was trying to import everything from the local repo folder instead of expected site-packages folder with installed package despite of usage --pyargs flag.

pytest --pyargs mypkg pytest will discover where mypkg is installed and collect tests from there. https://docs.pytest.org/en/6.2.x/goodpractices.html#tests-as-part-of-application-code

As a curiosity, I believe that python -m pytest running using the local code where py.test did not was due to Python's default behavior of adding the directory that it's called in to sys.path. https://stackoverflow.com/a/39519231

mblondel commented 3 years ago

Awesome! Thanks a lot for the hard work!

Without your work, lightning would become obsolete. I'm really glad you stepped up!

StrikerRUS commented 3 years ago

Many thanks for the lightning fast feedback and advice!

Very glad to help with great project!

scikit-learn-contrib / lightning

migrate from nose to pytest #163

Current state of this PR: