8000 Move to pytest? · Issue #7319 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Move to pytest? #7319

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.

Sign up for GitHub

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
Closed
amueller opened this issue Aug 31, 2016 · 42 comments
Closed

Move to pytest? #7319

amueller opened this issue Aug 31, 2016 · 42 comments

Comments

@amueller
Copy link
Member

nosetests is no longer maintained and contains deprecated code (inspect.getargspec). It might not keep working with future versions of python. Should we create a plan to move to py.tests?
@nelson-liu
Copy link
Contributor

+1 i think this would be a good change.

@GaelVaroquaux
Copy link
Member
GaelVaroquaux commented Sep 1, 2016 via email

@kdexd
Copy link
kdexd commented Sep 4, 2016 •
edited
Loading

Well I agree, nose has some deprecated code now and shifting to pytest can be a really good choice.
I recently completed GSoC 2016 with TARDIS. Throughout my project I have worked a lot with pytest, and read a lot of its docs - I can take up this issue to solve on a regular basis.
While I have used a little of scikit, I am planning to get comfortable with the codebase for sometime, and then move ahead with a series of PRs. Would you accept my PRs a few weeks down the line ?
( Because I see you are choosing it for next GSoC )

@amueller
Copy link
Member Author
amueller commented Sep 6, 2016

@karandesai-96 we haven't settled on anything yet. GSOC is one option. I'm not sure you should start with PRs on this yet, but there will certainly be an opportunity to work on this.
Maybe @bmcfee has some advice on what issues we might run into.

@bmcfee
Copy link
Contributor
bmcfee commented Sep 6, 2016

I wouldn't expect any major problems here: it's easy to get nose tests running in py.test. The harder part will be making the best use of py.test (proper sharing of fixtures, etc).

@code-of-kpp
Copy link

Just replaced nosetests command in Makefile with py.test. Test system is working this way.

=================================== 402 failed, 6400 passed, 19 skipped, 1 pytest-warnings in 355.58 seconds ===================================

most errors come from sklearn/neighbors/tests/test_kde.py

@amueller
Copy link
Member Author
amueller commented Sep 6, 2016

We need to remove all nose imports, though.

@kdexd
Copy link
kdexd commented Sep 6, 2016

I have an experience with fixtures, hooks and custom conftest.py. Other than that I don't think there's much of a problem in undergoing the change. If we require a massive porting - I'm all in ! 😃 If I were a maintainer of this repo, I would make a new branch named nose-to-pytest and ask the contributor to make PRs to that branch. When CI passes completely, I'd merge that into master. I'd be happy to hear more on this.

@amueller
Copy link
Member Author
amueller commented Sep 6, 2016

@karandesai-96 a bit of a problem with this parallel approach is that master is moving relatively fast. It might be the best option though? We need to be very careful not to drop any fixes that were applied to master since forking, though.

@code-of-kpp
Copy link

Most of such imports are
from nose.tools import assert_raises, assert_true, assert_false, assert_equal and SkipTest
that should be fairy easy to re-do/import from another place

The only thing I'm not familiar with is:

sklearn/datasets/tests/test_base.py:@nose.tools.with_setup(setup_load_files, teardown_load_files)

@amueller
Copy link
Member Author
amueller commented Sep 6, 2016

those are fixtures.

@code-of-kpp
Copy link

You mean with_setup?

There are only two files with it:
sklearn/datasets/tests/test_base.py
sklearn/datasets/tests/test_mldata.py

3 occurrences in each

@code-of-kpp
Copy link

Probably. we can just group those tests in classes and do this
http://doc.pytest.org/en/latest/xunit_setup.html#class-level-setup-teardown

@code-of-kpp code-of-kpp mentioned this issue Sep 10, 2016
Merged
@code-of-kpp code-of-kpp mentioned this issue Oct 6, 2016
Closed
@nelson-liu nelson-liu mentioned this issue Oct 6, 2016
Merged
@amueller
Copy link
Member Author

so the great py.test magic that gives helpful error messages is broken by the helpful assert helpers... at least partially.

import numpy as np
from sklearn.utils.testing import assert_true


def test_greater():
    X = np.random.normal((10, 10))
    assert_true(np.all(X < 0))

produces a non-helpful error message (under nosetests or py.test), but

def test_greater():
    X = np.random.normal((10, 10))
    assert np.all(X < 0)

provides a helpful error message under pytest. So I guess as part of the transition we should change all assert_true with assert, and maybe also assert_false?

@nelson-liu
Copy link
Contributor
nelson-liu commented Oct 25, 2016 •
edited
Loading

So I guess as part of the transition we should change all assert_true with assert, and maybe also assert_false

This seems reasonable. Accordingly, would there be any use for sklearn.utils.testing.assert_true and sklearn.utils.testing.assert_false after the move?

@lesteve lesteve mentioned this issue Jan 30, 2017
Merged
@kdexd
Copy link
kdexd commented Feb 1, 2017 •
edited
Loading

Hi again, everyone! I undertook the nose to pytest migration in joblib ( joblib/joblib#411 ) and completed it within nearly a month, having @lesteve as my main reviewer. I saw that he is in the reviewers panel in scikit-learn as well. It has been approximately a month now, I had a little vacation from Python, visited the C++ land.. I wish to get back to contributing here on this issue. Quite a while since the last active discussion happened on this thread. What's the status here ?

/ping @lesteve

@jnothman
Copy link
Member
jnothman commented Feb 1, 2017 via email

@lesteve
Copy link
Member
lesteve commented Feb 1, 2017

What are you proposing?

Shameless plug: first step is #8246.

I have some further thoughts on this that I need to organize a bit. I'll post here when I get around to it.

@jnothman
Copy link
Member
jnothman commented Feb 1, 2017 via email •
edited
Loading

@kdexd
Copy link
kdexd commented Feb 2, 2017

@jnothman I will be happy to take this up as a GSoC project, scikit-learn is one of the frequently used libraries in my routine. Joblib migration was about 2000 lines 8000 and it took nearly a month, this is bigger so talking about time, I'll do the math now. The workflow will be similar so I can do it faster due to practice.

About work related to check_estimators, I will be confidently able to tell that after being involved more with the community and looking around the code more - positively around the time when I start preparing the first draft of my proposal.

@jnothman
Copy link
Member
jnothman commented Feb 2, 2017

Great! Because there aren't many projects we'd consider running this year, but I think this, and you, would be a worthwhile use of strained resources.

@NelleV
Copy link
Member
NelleV commented Feb 2, 2017

FYI, it took less than 10 hours of work on skimage. If done properly, I don't think it should more than this on sklearn, specially considering part of the work has already been done. I really don't think this is near enough for a GSOC.

Note that numpy.testing has some hidden dependencies on nose (assert_raises for instance).

@lesteve
Copy link
Member
lesteve commented Feb 2, 2017

FYI, it took less than 10 hours of work on skimage

Out of interest, do you have a script that make most of the tedious conversion automatic? I have heard of https://github.com/pytest-dev/unittest2pytest but I have not given it a go.

@NelleV
Copy link
Member
NelleV commented Feb 2, 2017

I used git grep + vim macros mostly (and a lot of travis running time…).

On sklearn, all the imports are "localized", so I suspect the bulk is going to be the ~1000 assert raises. Sklearn also doesn't use any fancy nose plugins, which is going to simplify the migration.

@kdexd
Copy link
kdexd commented Feb 2, 2017

@NelleV I think probably you mean something like this work: joblib/joblib#433 I completely agree with you, this much work will hardly take a day or two now. Maybe a little more time if I take review cycles into account as well.

Actually I was imagining a little more addition here, pytest provides a bunch of custom fixtures, and it has a different way to add testcases ( through a @parametrize decorator ). Apart from this there can be a bunch of other changes as well, most of which were taken in separate PRs in joblib (You might like to have an overview through PR titles mentioned here joblib/joblib#411 ). Generally it helps in linearizing the flow and shortening the test bodies, while rarely we chose not to do so due to some reasons of joblib (mostly due to optional numpy dependency). I ended up reducing ~600 lines of existing code due to that.

@NelleV
Copy link
Member
NelleV commented Feb 2, 2017

I don't take review cycle in account in my estimates.

@lesteve
Copy link
Member
lesteve commented Feb 2, 2017 •
edited
Loading

Here is my thoughts on the pytest move as promised:

Towards pytest:

  • get the tests to run fine with pytest with the minimal modifications. Already done in my PR (waiting for feedback): [MRG] Make tests runnable with pytest without error #8246
  • Add some pytest conf in setup.cfg to disable annoying warnings about yield and run doctests
  • replace assert_* helpers by bare asserts. Some assert_* helpers should probably not be changed at this point, e.g. assert_raises_regex

Transition period:

  • During this phase you can run the tests with either nosetests or pytest. I would be enclined to wait until 0.19 release to drop nose. For example it is useful to be able to test the same test with
    the same tool and two different scikit-learn versions (0.18 and 0.19 for nose for example)
  • During this phase we could have a single build using pytest in Travis. Whether it is in allowed_failures mode is up for debate (I am enclined to say it should not be allowed to fail).

Dropping nose:

  • once nose compatibility is relaxed, maybe rewrite some of the tests, although I am not sure there is that much to to gain here. Also there is the problem of creating conflicts with ongoing
    PRs. What sticks to mind is removing the yield tests and replacing them with parametrize.

@HolgerPeters HolgerPeters mentioned this issue Feb 8, 2017
Closed
@HolgerPeters
Copy link
Contributor

I started migrating scikit-learn to pytest before I saw this issue, and got a little carried away while doing all the changes. Implementing #8321 I came to the following conclusions

  • pytest and nose are different enough, that maintaining a code-base that supports both test runners is imho too much of an effort.
  • assert_raises is the only critical numpy testing functions that needs fixing
  • assert_true and others are probably best fixed, once pytest is the default test runner on master. A PR that swaps test-runners shouldn't be too long lived because there is a danger of merge conflicts.
  • the yield-style tests that trigger a deprecation warning are probably the most time-consuming issue with regards to a pytest-migration. I already replaced many with pytest.mark.parametrize in [WIP] Pytest test suite completely independent of nose #8321 and the others could also be replaced.

@lesteve
Copy link
Member
lesteve commented Feb 9, 2017

I am more than willing to hear others' opinion on this, but here is mine:

pytest and nose are different enough, that maintaining a code-base that supports both test runners is imho too much of an effort.

Current tests run almost without error with pytest. Only #8246 needs to be merged AFAIK. Do you have any specific problem in mind?

assert_raises is the only critical numpy testing functions that needs fixing

I could find only two occurences of numpy.testing.assert_raises and they can be replace by sklearn.testing.assert_raises, so I do not think this is a problem.

A PR that swaps test-runners shouldn't be too long lived because there is a danger of merge conflicts.

Yeah but such a PR is not that trivial to review, this is why I was proposing to break down into chunks in #7319 (comment).

@kdexd
9E88 Copy link
kdexd commented Feb 9, 2017

I agree with @lesteve. This migration should be broken down into chunks as the review cycle might get long and PRs here will have a longer life, which are bound to make merge conflicts.

@HolgerPeters
Copy link
Contributor

Current tests run almost without error with pytest. Only #8246 needs to be merged AFAIK. Do you have any specific problem in mind?

It took me a while to realize it. You are using the pytest test runner, with nose being installed in the same environment. And that is why #8246 only has very few changes. In contrast, I started with #8321 by not installing nose and making the whole test suite run on pytest-only. Then yes, with nose present it seems a more incremental approach of removing nose step-by-step would be doable.

Anyway, it still might be interesting for you to have a look at #8321 as it has migrated the whole test suite to a state that is not using nose anymore. What would be left to do is to properly migrate the CI jobs, which would be diligent but routine kind of work.

@lesteve
Copy link
Member
lesteve commented Feb 9, 2017

Then yes, with nose present it seems a more incremental approach of removing nose step-by-step would be doable.

Incremental is key here IMHO.

jcrist added a commit to jcrist/dask-searchcv that referenced this issue Feb 23, 2017
@jcrist
Remove nose dependency for tests
5c67299 
Scikit-learn currently uses `nose` for tests, while dask projects use
`py.test` (and I like py.test better). Scikit-learn plans to move to
py.test in the near-ish future because `nose` is no longer maintained
(see issue scikit-learn/scikit-learn#7319). As
such, this swaps out the current test/mock stuff that depends on `nose`
to use `py.test` instead.
@jcrist jcrist mentioned this issue Feb 23, 2017
Merged
jcrist added a commit to dask/dask-searchcv that referenced this issue Feb 23, 2017
@jcrist
Remove nose dependency for tests (#23)
0e5eae4 
Scikit-learn currently uses `nose` for tests, while dask projects use
`py.test` (and I like py.test better). Scikit-learn plans to move to
py.test in the near-ish future because `nose` is no longer maintained
(see issue scikit-learn/scikit-learn#7319). As
such, this swaps out the current test/mock stuff that depends on `nose`
to use `py.test` instead.
@lesteve lesteve mentioned this issue Jul 26, 2017
Closed
@massich massich mentioned this issue Jul 26, 2017
Closed
@jnothman
Copy link
Member

I think we should move to pytest as the default for CI before the next release, but we will need to have a small test suite for ensuring estimator_checks still all work with nose.

@glemaitre
Copy link
Member

estimator_checks still all work with nose

Do you still want to support nose? Is there any feature which are not in pytest?

We start to move the test in imbalanced learn and we saw that the yield will not be supported in the future by pytest. I think that it could be good to give you some feedback once we migrated since:

  • we can tell you, as a user of the estimator check what we would need
  • we will find out how to parametrize the tests to get away from the yield
  • @jorisvandenbossche mention that you can call a test with a specific parametrization to execute a single estimator for instance, which could be really useful. I usually have problem with the common test because I am executing everything each time (maybe there is a nosetest feature that I don't know)

@jnothman
Copy link
Member
jnothman commented Aug 17, 2017 via email

@jorisvandenbossche
Copy link
Member

I think it should be possible to keep the current check_.. functions as is, but parameterize them with pytest internally instead of the yielding functions. This way (at least initially) they can be kept 'amphibious' to also still work with nose.

Small toy example:

import pytest

from sklearn.base import BaseEstimator
from sklearn.linear_model import LinearRegression, Lasso, Ridge


def check_estimator_aspect(estimator):
    """ dummy example of existing check_... function """
    # ...
    assert isinstance(estimator(), BaseEstimator)

estimators = [LinearRegression, Lasso, Ridge]

# run this check function for a list of estimators by parametrizing them
test_estimator_aspect = pytest.mark.parametrize('estimator', estimators)(check_estimator_aspect)

# run this check function using yield (simplified version of how it is done now, but deprecated in pytest)
def test_estimator_aspect_orig():
    for estimator in estimators:
        yield check_estimator_aspect, estimator

This way external project can import the check_... functions but do their own parametrization (with their own estimator objects) allowing those projects to also migrate to pytest, can the current _yield_..checks functions be kept but deprecated (to not break external projects directly that now use those yield checks), and can sklearn internally already use the pytest parametrization way.

Note: I only tested it with the above toy example, so not sure how it would work out in practice with all complexities.

If you run the above with pytest, you get:

joris@joris-XPS-13-9350:~/scipy$ pytest test_pytest.py -v
============= test session starts ===============
collected 6 items 

test_pytest.py::test_estimator_aspect[LinearRegression] PASSED
test_pytest.py::test_estimator_aspect[Lasso] PASSED
test_pytest.py::test_estimator_aspect[Ridge] PASSED
test_pytest.py::test_estimator_aspect_orig::[0] PASSED
test_pytest.py::test_estimator_aspect_orig::[1] PASSED
test_pytest.py::test_estimator_aspect_orig::[2] PASSED

=============== warnings summary ===================
/home/joris/scipy/test_pytest.py
  yield tests are deprecated, and scheduled to be removed in pytest 4.0
  yield tests are deprecated, and scheduled to be removed in pytest 4.0
  yield tests are deprecated, and scheduled to be removed in pytest 4.0

=============== 6 passed, 3 warnings in 0.42 seconds ======================

So the parametrized ones get automatically a nicer name, which you can use to run a single test (so not necessarily a single check for all estimators, but really a single test)

@lesteve
Copy link
Member
lesteve commented Aug 21, 2017

well we need the estimator checks to be amphibious for a deprecation period in my opinion.

Can I ask why? Is it just that you want to keep the estimator_checks.check_* functions that may be used by downstream packages? Is there a reason we still want to run the estimator checks with nose?

@jorisvandenbossche
Copy link
Member

Can't speak for @jnothman, but I think that is indeed the reason (downstream packages use those check functions to test compliance, and they can still be using nose).
But also, even if we don't want to keep the estimator_checks.check_* compatible with nose, we still need a way for downstream packages to run the checks.

@jnothman
Copy link
Member
jnothman commented Aug 21, 2017 via email

@jorisvandenbossche
Copy link
Member

With my comment above (#7319 (comment)), I give one possible way how this could be possible I think (without needing to duplicate the check_estimators just to keep backwards compatibility, but keeping them working as is for downstream CI's)

@lesteve lesteve changed the title Move to py.tests? Move to pytest? Oct 2, 2017
@amueller
Copy link
Member Author

done in #9840.

@jnothman jnothman mentioned this issue Feb 28, 2018
Closed
@rth rth mentioned this issue May 3, 2018
Merged
@rth
Copy link
Member
rth commented May 3, 2018

I think it should be possible to keep the current check_.. functions as is, but parameterize them with pytest internally instead of the yielding functions

Something fairly similar is proposed in #11063

@rth rth mentioned this issue May 7, 2018
Merged
3 tasks
@jeromedockes jeromedockes mentioned this issue Jun 20, 2019
Closed
@kchawla-pi kchawla-pi mentioned this issue Dec 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0