[MRG+1] Remove nose from CIs and documentation #9840

lesteve · 2017-09-27T14:42:37Z

Apart from tackling comments that people may have, here are a few things I want to do before this can be merged:

quickly compare the number of tests run by pytest and nose to check that we were not missing tests in the transition
look at the codecov numbers in detail for the same reason

rth

Great that it's finally happening! I double checked there is indeed no references to nose left after this PR (excluding those wrapped into a try: .. ImportError: clause. LGTM if CI tests pass..

rth · 2017-09-27T16:07:25Z

doc/developers/advanced_installation.rst

-interact with tests that use ``multiprocessing``::
-
-    C:\Python34\python.exe -c "import nose; nose.main()" -v sklearn
+    $ pytest sklearn


Maybe add the -v option to be consistent with make test-code?

I am not sure what the recommendation should be. I rarely use -v myself and I would argue that it is only useful (if at all) if you only have a log file e.g. on our CIs, to make sure that a particular test was run or succeeded or not.

I see your point. Personally, I frequently use -sv to see, when there is something at the stdout, which unit tests it is associated with. But yes, without a log or the -s option, -v might not be that useful (particularly when there are lots and lots of tests as in scikit-learn)...

rth · 2017-09-27T16:07:49Z

doc/developers/advanced_installation.rst


-    nosetests -v sklearn/
+    pytest sklearn


Same comment as above

lesteve · 2017-09-27T16:44:44Z

Probably better to wait for 0.19.1 to be released before merging this one.

lesteve · 2017-09-27T16:52:34Z

The Travis failure on Python 2.7 conda distrib is one of this "What the ..." one. It happens for all the PRs and started happening only recently (maybe a few hours ago). For some reason (I have even the beginning of a clue why) in this build only, the detected C compiler is:

C compiler: /usr/lib/ccache/g++ -fno-strict-aliasing -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O3 -pipe -DNDEBUG -fwrapv -O3 -Wall -fPIC

-fstack-protector-strong is only supported for gcc >= 4.9 and Ubuntu uses gcc 4.8.4.

lesteve · 2017-09-27T17:13:51Z

I bet this is a new thing in the miniconda installer that adds some gcc related stuff .... although I am not 100 % sure.

I tried to download the installer from:
https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh

The python I get has been compiled with gcc 7.2.0. Although I can not reproduce the exact same problem I have a somewhat similar one where the detected C compiler has -fstack-protector-strong as arguments:

C compiler: x86_64-conda_cos6-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -fwrapv -O2 -Wall -Wstrict-prototypes -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -pipe -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -pipe -fPIC

cc @jnothman in case you are scratching your head over this as well.

I am just hoping this is a temporary problem that will not exist tomorrow morning (European time) ;-).

And yeah I forgot to say: x86_64-conda_cos6-linux-gnu-gcc does not exist so I can not build scikit-learn of course.

lesteve · 2017-09-27T17:18:57Z

Looking at https://repo.continuum.io/miniconda/ it looks like the latest miniconda installers have been uploaded around 2017-09-26 16:26:32.

lesteve · 2017-09-27T17:37:00Z

I pushed a commit to use the previous version of the miniconda installer, which I am hoping will fix the problem. This is probably what we should do in case the issue does not get solved quickly on the Anaconda Inc side.

lesteve · 2017-09-27T21:48:41Z

Seems like the issue has been reported at conda/conda#6030.

lesteve · 2017-09-27T22:16:59Z

The Miniconda-latest now points to the previous miniconda installer version and I pushed a temporary work-around in master (essentially skip conda update conda): 8de18e6.

jnothman · 2017-09-28T00:00:10Z

We will need a nose instance that runs check_estimator on a variety of estimators during a nose deprecation period, IMO.

lesteve · 2017-09-28T04:11:11Z

We will need a nose instance that runs check_estimator on a variety of estimators during a nose deprecation period, IMO.

Can you elaborate why, for example what are you worried about? I seem to remember you have mentioned this already in a separate issue but I did not get your point at the time.

jnothman · 2017-09-28T04:38:36Z

It's possible I'm mistaken. Other projects use scikit-learn's check_estimator. I thought we should check that there's nothing in there that makes it not work on nose. But I suppose at the end of the day it'll be some function with some assertions, so perhaps it's unnecessary.

…

On 28 September 2017 at 14:11, Loïc Estève ***@***.***> wrote: We will need a nose instance that runs check_estimator on a variety of estimators during a nose deprecation period, IMO. Can you elaborate why, for example what are you worried about? I seem to remember you have mentioned this already in a separate issue but I did not get your point at the time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6zo5BDJu04nwKFmApd4XqwDfcv8qks5smxxhgaJpZM4Pl4gu> .

lesteve · 2017-09-28T04:50:54Z

OK makes sense, thanks. I'll double check that indeed.

lesteve · 2017-10-02T07:23:27Z

I compared the output of nosetests -v sklearn to pytest -v sklearn and after some time figuring out all the tiny variations of one vs the other I am 100 % confident that we run the exact same tests (see my branch and the comparison for more details).

I found only one single case where a test was run by nose but not by pytest (test class with a constructor) which I will fix separately from this PR (see #9860).

lesteve · 2017-10-02T07:25:43Z

For completeness I will leave that PR alone for a bit, probably at least until we release 0.19.1.

jnothman · 2017-10-02T09:43:34Z

Good work :)

amueller · 2017-10-26T16:25:45Z

travis fails?

PackageNotFoundError: Package missing in current linux-64 channels: 
  - cython 0.26.1*

?!
probably a rendering issue?

lesteve · 2017-10-27T03:13:44Z

Yeah this was because I had a conda temporary work-around still in my branch. Now everything is green.

I think one of the main thing to discuss before a more thorough review is whether we want to ensure that check_estimator does not rely on pytest, which I think is what @jnothman means in #9840 (comment). I am hoping there is a way to do that without keeping a nose build but I have not looked at it in detail yet.

jnothman · 2017-10-28T09:56:19Z

well you certainly don't need a full nose build. but I can't see any way to assure it without running test_check_estimator in an environment without pytest

jnothman · 2017-11-02T13:24:22Z

Let's just add a build that does not install pytest and runs nosetests sklearn.utils.tests.test_estimator_checks

lesteve · 2017-11-03T16:05:56Z

How about this to check that pytest is not needed to run check_estimator? This makes sure that pytest is not imported when running check_estimator(LinearRegression()):

import subprocess
import sys


def test_pytest_not_required_for_check_estimator(tmpdir):
    to_execute = '\n'.join([
        'import sys',
        'from sklearn.utils.estimator_checks import check_estimator',
        'from sklearn.linear_model import LinearRegression',
        'check_estimator(LinearRegression())',
        'print(sys.modules["pytest"])'])
    path_to_execute = tmpdir.join('test.py')
    path_to_execute.write(to_execute)
    # Hack to make pandas not importable because pandas imports pytest if pytest
    # is installed
    tmpdir.join('pandas.py').write('raise ImportError()')

    stdout, stderr = subprocess.Popen(
        [sys.executable, path_to_execute.strpath],
        stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()

    assert stdout == b''
    assert b'KeyError' in stderr

Ideally going forward I think we would need to check that pytest is a soft dependency in the non test modules (i.e. basically I can import any module whose name is not test_*.py without pytest installed). I am not quite sure how to write that as a test. Related to numpy/numpy#9352 (comment).

jnothman · 2017-11-04T10:25:57Z

I suppose that's reasonable. How about just using assert pytest not in sys.modules then using check_call? I'd also test all major kinds of estimators to have stronger assurance. As I said, I'd rather run test_estimator_checks.

Pity about pandas... There's some risk that other libraries will do similar, but I suppose we'll find that out quickly?

jnothman

Otherwise LGTM

jnothman · 2017-11-13T07:39:05Z

sklearn/utils/tests/test_estimator_checks.py

+
+
+if __name__ == '__main__':
+    main_module = sys.modules['__main__']


Add a comment: "This module is run as a script to check that we have no dependency on pytest for estimator checks." Otherwise it could be removed by the ignorant and the test would still pass!

jnothman · 2017-11-13T07:41:00Z

build_tools/travis/test_script.sh

@@ -67,3 +59,17 @@ fi
 if [[ "$SKIP_TESTS" != "true" ]]; then
    run_tests
 fi
+
+if [[ "$CHECK_PYTEST_SOFT_DEPENDENCY" == "true" ]]; then
+    conda remove -y py pytest || echo "pytest not found by conda"


This error message will look weird without someone realising that we're trying to remove it. Silent is fine, IMO.

jnothman · 2017-11-13T07:41:33Z

.travis.yml

-           INSTALL_MKL="true" NUMPY_VERSION="1.13.1" SCIPY_VERSION="0.19.1"
-           PANDAS_VERSION="0.20.3" CYTHON_VERSION="0.26.1"
-           TEST_DOCSTRINGS="true"
+           CHECK_PYTEST_SOFT_DEPENDENCY="true"


We could also put this in its own worker to be sure it's sandboxed, but I guess it's okay as it is.

We could also put this in its own worker to be sure it's sandboxed, but I guess it's okay as it is.

I thought about too but I thought this is a bit of a waste to have another build and run tests only for 5 seconds.

…into so-long-nose

glemaitre · 2017-11-14T13:37:45Z

doc/developers/contributing.rst

-        $ pip install nose coverage
-        $ nosetests --with-coverage path/to/tests_for_package
+        $ pip install pytest pytest-cov
+        $ pytest --cov sklearn path/to/tests_for_package


Should we add an example to test a single test?

pytest path/to/tests_for_package::test_unit_test

I'd rather try to keep this PR minimal, essentially the doc should only be a nose -> pytest translation.

jorisvandenbossche · 2017-11-14T15:14:32Z

# Hack to make pandas not importable because pandas imports pytest if pytest
# is installed

That should be fixed in the latest release 0.21.0: pandas-dev/pandas#17710 (which doesn't necessarily help of course if you want to run the test against multiple versions of pandas)

lesteve · 2017-11-14T16:16:39Z

That should be fixed in the latest release 0.21.0: pandas-dev/pandas#17710 (which doesn't necessarily help of course if you want to run the test against multiple versions of pandas)

Nice to know, thanks! I ended up testing without pytest as suggested by @jnothman rather than looking in sys.modules. I think the former is less brittle.

amueller

lgtm

amueller · 2017-11-15T21:32:14Z

Makefile

@@ -29,19 +24,17 @@ inplace:
 	$(PYTHON) setup.py build_ext -i

 test-code: in
-	$(NOSETESTS) -s -v sklearn
+	$(PYTEST) --showlocals -v sklearn


this doesn't show warnings, right?

Not sure what you mean:

pytest logs by default show a lot less warnings that nosetests. A good thing in my opinion

for now, we disable pytest warnings in setup.cfg. This is mostly to get rid of the warnings about pytest not supporting tests using yield in pytest 4.0. This will be addressed in further PRs.

I wasn't able to turn on all the warnings we get with nosetests, and I think that's a bad thing. For example I couldn't get deprecation warnings (ours or numpy) to show. But I'm new to pytest

From https://docs.pytest.org/en/latest/warnings.html:

Starting from version 3.1, pytest now automatically catches warnings during test execution and displays them at the end of the session:

and

Pytest by default catches all warnings except for DeprecationWarning and PendingDeprecationWarning.

What I was not fully aware of this but it seems that --disable-pytest-warnings completely disables the warning summary. It was originally added to remove warnings about yield-based tests.

Reading more carefully https://docs.pytest.org/en/latest/warnings.html, it looks like you can use filterwarnings in setup.cfg to filter warnings a bit more carefully (only filter out yield-based tests deprecation and show other DeprecationWarnings). This is what we should probably do. Do you mind if I do that in a separate PR?

Looking closely at it, removing --disable-pytest-warnings from setup.cfg you get 10k lines of warnings in the warnings summary when running pytest sklearn. I am not really in favor of doing that by default:

it makes the pytest output a lot more noisy

you will not be able to see the Travis log directly inside Travis (you need to download the full log because it goes over the size limit). This is what is happening at the moment for example compare this pytest log to its nose equivalent log. In the nose log you get This log is too long to be displayed. Please reduce the verbosity of your build or download the raw log..

people interested to chase down warnings can run locally with the --disable-pytest-warnings commented out in their setup.cfg.

Side comment: actually we get less than 100 "yield tests are deprecated, and scheduled to be removed in pytest 4.0" warnings. So the original motivation for --disable-pytest-warnings does no longer exist.

I think we should make sure we don't use warnings instead of ignoring them. Many of them are important. Hiding them hides the problem.

Looking at the output of nose, I see that many of those are pretty recently introduced and probably point to real issues....

amueller · 2017-11-15T21:35:44Z

build_tools/travis/test_script.sh

+    if [[ "$COVERAGE" == "true" ]]; then
+        # Need to append the coverage to the existing .coverage generated by
+        # running the tests
+        CMD="coverage run --append"


I'm not sure what the point of this is. The union of the coverage of running the tests with pytest and running the estimator checks without pytest?

I'm not sure what the point of this is. The union of the coverage of running the tests with pytest and running the estimator checks without pytest?

Yes. This way you can see that run_tests_without_pytest is covered in test_estimator_checks.py. test_script.sh is simpler without this so I can definitely remove it. With hindsight I probably should not have done it, it took me way too much time to get to work and that was only so that codecov would be green ...

ok, fine by me. I'm good with merging either way.

lesteve · 2017-11-16T15:12:49Z

I compared the output of nosetests -v sklearn to pytest -v sklearn and after some time figuring out all the tiny variations of one vs the other I am 100 % confident that we run the exact same tests (see my branch and the comparison for more details).

I just reran the comparison to err on the side of caution and I am 100% confident that we run the exact same tests on nose and pytest.

Summing up all the reviews, I think there is at least 3.5 LGTM from @rth @jnothman @agramfort @amueller @glemaitre. Who will be bold enough to press the green button?

glemaitre · 2017-11-16T15:17:25Z

I am on my phone. Not sure that blackberry allowed for merging PR for security reason :D but otherwise I would...?

rth · 2017-11-16T19:21:47Z

Great that the pytests migration is happening. Thanks for working on it @lesteve !

jnothman · 2017-11-16T21:46:45Z

Well done, @lesteve! Thanks.

…

On 17 November 2017 at 06:21, Roman Yurchak ***@***.***> wrote: Great that the pytests migration is happening. Thanks for working on it @lesteve <https://github.com/lesteve> ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9840 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6_c7jsEHhYSGNqIiLDMgFyFIHjgHks5s3ItOgaJpZM4Pl4gu> .

amueller · 2017-11-16T22:23:22Z

Thanks @lesteve !

amueller · 2017-11-16T22:33:57Z

Is there an issue about the yields and fixing the test names?

glemaitre · 2017-11-16T23:59:30Z

The issue with yields will come after pytest 4 I think.

qinhanmin2014 · 2017-11-22T14:56:15Z

README.rst

-interact with tests that use ``multiprocessing``::
-
-    C:\Python34\python.exe -c "import nose; nose.main()" -v sklearn
+    pytest sklearn

 See the web page http://scikit-learn.org/stable/developers/advanced_installation.html#testing


Would it be better to change the web page to dev version? I am instructed to use pytest and I want to click in the link for more information, but are soon surrounded by nose :-)

Good catch I guess this would make a lot of sense in this case. Feel free to do a quick grep to spot similar problems with the nose to pytest move and open a PR.

@lesteve

We have changed to pytest but the stable doc is about nose. Already gain approval from @lesteve in #9840. Nothing to check so I push directly.

@lesteve

We have changed to pytest but the stable doc is about nose. Already gain approval from @lesteve in scikit-learn#9840. Nothing to check so I push directly.

Apparently this went missing in scikit-learn#9840

lesteve force-pushed the so-long-nose branch from 8430f32 to 99c9183 Compare September 27, 2017 14:47

rth mentioned this pull request Sep 27, 2017

[MRG] MAINT pytest test discovery #9839

Closed

rth reviewed Sep 27, 2017

View reviewed changes

lesteve added this to the 0.20 milestone Sep 27, 2017

lesteve force-pushed the so-long-nose branch from 4620fad to 487a4ba Compare September 27, 2017 17:34

lesteve mentioned this pull request Sep 27, 2017

[MRG+1] MAINT Use magic to list documentation versions #9841

Merged

lesteve force-pushed the so-long-nose branch from 037ef12 to 9a4a80f Compare September 27, 2017 21:48

lesteve force-pushed the so-long-nose branch from 9a4a80f to c86b5cf Compare September 28, 2017 04:14

lesteve force-pushed the so-long-nose branch 2 times, most recently from 05b98b7 to 66e95c8 Compare November 10, 2017 11:00

jnothman reviewed Nov 13, 2017

View reviewed changes

Tackle comments + some tweaks

eb02410

lesteve force-pushed the so-long-nose branch from 39089f6 to eb02410 Compare November 13, 2017 09:49

scikit-learn deleted a comment from codecov bot Nov 13, 2017

Merge branch 'master' of https://github.com/scikit-learn/scikit-learn …

2650c89

…into so-long-nose

glemaitre reviewed Nov 14, 2017

View reviewed changes

amueller approved these changes Nov 15, 2017

View reviewed changes

rth merged commit e7e05d8 into scikit-learn:master Nov 16, 2017

This was referenced Nov 16, 2017

Enable warnings on travis (and make sure they are reasonable) #10158

Open

Move to pytest? #7319

Closed

lesteve deleted the so-long-nose branch November 17, 2017 07:25

lesteve mentioned this pull request Nov 17, 2017

[MRG+2] Remove nose-specific _named_check #10160

Merged

jorisvandenbossche mentioned this pull request Nov 22, 2017

CLN: remove nose fixtures in docs #10187

Merged

qinhanmin2014 reviewed Nov 22, 2017

View reviewed changes

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG+1] Remove nose from CIs and documentation (scikit-learn#9840)

35c731a

jnothman added a commit to jnothman/scikit-learn that referenced this pull request Jan 14, 2018

CI Reinstate docstring testing

ba6a551

Apparently this went missing in scikit-learn#9840

jnothman mentioned this pull request Jan 14, 2018

[MRG] CI Reinstate docstring testing #10473

Merged

thomasjpfan mentioned this pull request Apr 26, 2019

[MRG+2] Faster Gradient Boosting Decision Trees with binned features #12807

Merged

trevorstephens mentioned this pull request May 26, 2019

Scikit-learn 0.21.1 strange failure using nosetests with import train_test_split #13943

Closed



		if __name__ == '__main__':
		main_module = sys.modules['__main__']

[MRG+1] Remove nose from CIs and documentation #9840

[MRG+1] Remove nose from CIs and documentation #9840

Conversation

lesteve commented Sep 27, 2017

rth left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Sep 27, 2017

lesteve commented Sep 27, 2017 • edited Loading

lesteve commented Sep 27, 2017 • edited Loading

lesteve commented Sep 27, 2017

lesteve commented Sep 27, 2017

lesteve commented Sep 27, 2017

lesteve commented Sep 27, 2017 • edited Loading

jnothman commented Sep 28, 2017

lesteve commented Sep 28, 2017

jnothman commented Sep 28, 2017 via email

lesteve commented Sep 28, 2017

lesteve commented Oct 2, 2017 • edited Loading

lesteve commented Oct 2, 2017

jnothman commented Oct 2, 2017

amueller commented Oct 26, 2017

lesteve commented Oct 27, 2017

jnothman commented Oct 28, 2017 via email

jnothman commented Nov 2, 2017

lesteve commented Nov 3, 2017 • edited Loading

jnothman commented Nov 4, 2017

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Nov 14, 2017

lesteve commented Nov 14, 2017

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesteve commented Nov 16, 2017 • edited Loading

glemaitre commented Nov 16, 2017 via email

rth commented Nov 16, 2017

jnothman commented Nov 16, 2017 via email

amueller commented Nov 16, 2017

amueller commented Nov 16, 2017

glemaitre commented Nov 16, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rth left a comment •

edited

Loading

lesteve commented Sep 27, 2017 •

edited

Loading

lesteve commented Sep 27, 2017 •

edited

Loading

lesteve commented Sep 27, 2017 •

edited

Loading

lesteve commented Oct 2, 2017 •

edited

Loading

lesteve commented Nov 3, 2017 •

edited

Loading

lesteve Nov 16, 2017 •

edited

Loading

lesteve commented Nov 16, 2017 •

edited

Loading