[MRG + 1] [TST] (half-cosmetic) use less nose.tools import to simplify future transition to py.test by code-of-kpp · Pull Request #7384 · scikit-learn/scikit-learn

code-of-kpp · 2016-09-10T02:16:41Z

Reference Issue

Contributing my experiments with ideas from #7319 issue

What does this implement/fix? Explain your changes.

Replaces most occasions of import nose and from nose.tools import with from sklearn.utils.testing import

Any other comments?

For now we can't get rid of nosetests completely because numpy.testing is built on top of them and there is a lot of numpy.testing imports.
But with that and with strong recommendation to not import anything from nose we will be able to replace it as soon as numpy will.

Also, with this PR setting $NOSETESTS to pytest should just work without any significant code changes.

code-of-kpp · 2016-09-11T20:59:46Z

There is no new functionality here and changes are very simple, so I really believe it should me merged ASAP. The sooner it is merged, the less work would be required to cleanup those imports from other PRs

jnothman · 2016-09-11T23:12:18Z

My concern is the load on the core dev team to review, reflect on whether
there's anything essential missing from the features being release, and get
it out the door. This is really low priority in comparison. Remind in a
week.

On 12 September 2016 at 06:59, Konstantin Podshumok <
notifications@github.com> wrote:

There is no new functionality here and changes are very simple, so I
really believe it should me merged ASAP. The sooner it is merged, the less
work would be required to cleanup those imports from other PRs

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7384 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz6-b86cO8gT1rkriEvxnWBggnHxV5ks5qpGvEgaJpZM4J5nFT
.

amueller · 2016-09-12T16:43:03Z

I think @mblondel did something similar at some point and got some push-back. but then it was cosmetic, now it's actually quite useful to hide the source so we can move.

code-of-kpp · 2016-09-12T18:43:46Z

May be I should extend this PR to full "Move to py.test" branch? Currently it is more or less cosmetic. But as I said above every new merged PR can introduce new nose imports, keeping it in actual state may not wroth it

mblondel · 2016-09-13T06:19:59Z

I think @mblondel did something similar at some point and got some push-back

You have a good memory :)

rth · 2016-10-03T18:11:31Z

Also, with this PR setting $NOSETESTS to pytest should just work without any significant code changes.

This would be quite useful. Could you rebase please? Also maybe add a [MRG] tag to the title if you happy for this to be merged. Even if new PR introduce new nose imports, this is still an improvement over the current state.

amueller

Yeah I think this would be great to have.

amueller · 2016-10-03T20:20:39Z

sklearn/utils/testing.py

-    # Nose < 1.0.0
+    SkipTest = unittest.case.SkipTest
+except AttributeError:
+    # Python <= 2.6, we stil need nose here


we dropped support for this.

jnothman · 2016-10-04T12:54:59Z

I think this would be good to merge ASAP. Please address the fact that we no longer support Python 2.6 (at master)

amueller · 2016-10-05T19:36:31Z

Also please rebase.

…eveloping test suites/runners

…string_values and one missed ImportError that should be replaced with AttributeError

although failed test will look a little bit ugly

lesteve · 2016-10-06T06:31:01Z

Thanks for rebasing ! Out of curiosity, is this PR supposed to remove all nose imports? I still see a few of them:

$ git grep -P 'from nose|import nose'
sklearn/utils/testing.py:from nose.tools import raises
sklearn/utils/testing.py:from nose import with_setup
sklearn/utils/testing.py:    from nose import SkipTest
sklearn/utils/tests/test_testing.py:    from nose.tools import assert_less
sklearn/utils/tests/test_testing.py:    from nose.tools import assert_greater

I understand that raises, and with_setup are still needed, SkipTest as well only for Python 2.6. Having said that, the two last ones can be removed, right ?

Given the probability of conflicts with other changes, I would be fine leaving the nose imports for Python 2.6 and do the Python 2.6 support removal in a separate PR.

code-of-kpp · 2016-10-06T06:35:42Z

Currently:

$ grep -n -E '^\s*[^#]+*nose' `find . -name '*.py'`
./sklearn/utils/tests/test_testing.py:22:    from nose.tools import assert_less
./sklearn/utils/tests/test_testing.py:36:    from nose.tools import assert_greater
./sklearn/utils/testing.py:51:from nose.tools import raises
./sklearn/utils/testing.py:52:from nose import with_setup
./sklearn/utils/testing.py:85:    from nose import SkipTest
./sklearn/externals/joblib/testing.py:28:    from nose.tools import assert_raises_regex
./sklearn/externals/joblib/testing.py:32:        from nose.tools import assert_raises_regexp as assert_raises_regex

imports in test_testing are py2.6-compatible to be removed, the same with SkipTest in testing.py:85.
I don't know what to do with externals.joblib.testing

with_setup can be replaced in most places with setup_module or with pytest.fixture
raises can be replaced directly with pytest.raises when time comes.

Please note, that numpy uses nose so all imports of numpy.testing really imply nose.tools.

One thing I noticed during my work on this PR is active usage of from numpy.testing import assert_equal where people really just mean usual assert_equal or asset_array_equal. I think usage of this function may be confusing and should be avoided in the future, for example consider:

>>> from nose import tools
>>> from numpy import testing
>>> 
>>> testing.assert_equal([], ())  # passes
>>> tools.assert_equal([], ())
Traceback (most recent call last)\
...
AssertionError: [] != ()

Should we document guidelines about nose and numpy.testing? Where to put these docs?

Can I remove py2.6 from .travis.yml or should I wait for master? After that I can remove py2.6-related commits without breaking CI.

GaelVaroquaux · 2016-10-06T06:37:42Z

I don't know what to do with externals.joblib.testing

Open an issue in the joblib repo. If the devs are in favor of moving to py.test, make a PR there.

lesteve · 2016-10-06T06:39:49Z

Open an issue in the joblib repo. If the devs are in favor of moving to py.test, make a PR there.

Yeah do that, I'd be more than favourable of using joblib for a test bed of moving to py.test. For completeness, sklearn/externals/joblib is always an unmodified release of joblib so it shouldn't be changed within scikit-learn.

lesteve · 2016-10-06T06:46:00Z

Can I remove py2.6 from .travis.yml or should I wait for master? After that I can remove py2.6-related commits without breaking CI.

My personal preference would be to do the Python 2.6 support removal in a separate PR. There is quite a few things to remove not related to nose or testing (the OrderedDict backport jumps to mind) and single-purpose PR are far easier to merge.

I think we should keep the .travis.yml Python 2.6 build but replace Python 2.6 by Python 2.7 in order to test old versions of dependencies.

code-of-kpp · 2016-10-06T07:31:46Z

Having said that, the two last ones can be removed, right ?

Those are imports are used to test if some sklearn's and noses's assert_* functions do the same.
With py2.6 there are some other nose imports and I believe these tests make sense.

Even more, until we have a nose dependency there is a sense in extending them - after all, I introduced some new (untested) code...

lesteve

LGTM, as I said above I am fine merging as is and removing the Python 2.6 support in a separate PR.

amueller · 2016-10-07T16:47:10Z

Thanks. Let's see where this'll bite us first ;)

…y future transition to py.test (scikit-learn#7384) * use less nose.tools import to simplify future transition to activly developing test suites/runners * assert_equal -> assert_array_equal in test_feature_hasher_pairs_with_string_values and one missed ImportError that should be replaced with AttributeError * test for py2.6 compat with except AttributeError * fix importing of SkipTest * force using nose in python2.6 for now * there was no assert_dict_equal in py2.6. but we can use assert_equal although failed test will look a little bit ugly * remove nose imports from doc/datasets # Conflicts: # sklearn/neighbors/tests/test_dist_metrics.py # sklearn/utils/tests/test_fixes.py

…y future transition to py.test (scikit-learn#7384) * use less nose.tools import to simplify future transition to activly developing test suites/runners * assert_equal -> assert_array_equal in test_feature_hasher_pairs_with_string_values and one missed ImportError that should be replaced with AttributeError * test for py2.6 compat with except AttributeError * fix importing of SkipTest * force using nose in python2.6 for now * there was no assert_dict_equal in py2.6. but we can use assert_equal although failed test will look a little bit ugly * remove nose imports from doc/datasets

amueller reviewed Oct 3, 2016

View reviewed changes

kv-radar added 7 commits October 6, 2016 08:36

use less nose.tools import to simplify future transition to activly d…

198a771

…eveloping test suites/runners

assert_equal -> assert_array_equal in test_feature_hasher_pairs_with_…

2ed34cf

…string_values and one missed ImportError that should be replaced with AttributeError

test for py2.6 compat with except AttributeError

751ee32

fix importing of SkipTest

78437a0

force using nose in python2.6 for now

3495556

there was no assert_dict_equal in py2.6. but we can use assert_equal

0c85f66

although failed test will look a little bit ugly

remove nose imports from doc/datasets

675b900

code-of-kpp force-pushed the lessnoseimports branch from 6dba4c1 to 675b900 Compare October 6, 2016 05:52

code-of-kpp changed the title ~~use less nose.tools import to simplify future transition to py.test~~ [MRG] [TST] (half-cosmetic) use less nose.tools import to simplify future transition to py.test Oct 6, 2016

code-of-kpp mentioned this pull request Oct 6, 2016

Move to py.test? joblib/joblib#411

Closed

lesteve approved these changes Oct 7, 2016

View reviewed changes

lesteve changed the title ~~[MRG] [TST] (half-cosmetic) use less nose.tools import to simplify future transition to py.test~~ [MRG] + 1 [TST] (half-cosmetic) use less nose.tools import to simplify future transition to py.test Oct 7, 2016

lesteve changed the title ~~[MRG] + 1 [TST] (half-cosmetic) use less nose.tools import to simplify future transition to py.test~~ [MRG + 1] [TST] (half-cosmetic) use less nose.tools import to simplify future transition to py.test Oct 7, 2016

amueller merged commit 9b2aac9 into scikit-learn:master Oct 7, 2016

Uh oh!

Conversation

code-of-kpp commented Sep 10, 2016

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

code-of-kpp commented Sep 11, 2016

Uh oh!

jnothman commented Sep 11, 2016

Uh oh!

amueller commented Sep 12, 2016

Uh oh!

code-of-kpp commented Sep 12, 2016

Uh oh!

mblondel commented Sep 13, 2016

Uh oh!

rth commented Oct 3, 2016

Uh oh!

amueller left a comment

Choose a reason for hiding this comment

Uh oh!

amueller Oct 3, 2016

Choose a reason for hiding this comment

Uh oh!

jnothman commented Oct 4, 2016

Uh oh!

amueller commented Oct 5, 2016

Uh oh!

lesteve commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

code-of-kpp commented Oct 6, 2016

Uh oh!

GaelVaroquaux commented Oct 6, 2016 via email

Uh oh!

lesteve commented Oct 6, 2016

Uh oh!

lesteve commented Oct 6, 2016

Uh oh!

code-of-kpp commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

amueller commented Oct 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lesteve commented Oct 6, 2016 •

edited

Loading

code-of-kpp commented Oct 6, 2016 •

edited

Loading