Joblib: Move to py.test?

[podshumok]

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?

On the other hand py.test is quite compatible with nose and offers nice way to write less code.

See also:
https://github.com/scikit-learn/scikit-learn/issues/7319
https://github.com/scikit-learn/scikit-learn/pull/7384

[lesteve]

It would be great to move to py.test. Doing something like scikit-learn/scikit-learn#7384 would be a first step, so PR more than welcome!

A second step would be to use py.test fixtures where we use with_setup in our tests.

We are probably using numpy.testing in a few tests though so we won't be able to remove completely the dependency on nose since AFAIU numpy.testing relies on nose.

[aabadie]

Fixed by #442. Closing

[aabadie]

Fixed by #442. Closing

oops, this is not totally true actually. Reopening.

442 just prepares the code for a smooth switch to py.test when related projects (like numpy) won't relies on nose anymore.

[lesteve]

Great, thanks @karandesai-96 ! I have an error if I run the tests with py.test, is it just me?

py.test joblib/test/test_parallel.py -k test_backend_conte 

======================================================== test session starts =========================================================
platform linux -- Python 3.5.1, pytest-2.8.5, py-1.4.31, pluggy-0.3.1
rootdir: /home/le243287/dev/joblib, inifile: 
collected 69 items 

joblib/test/test_parallel.py ..FFF

============================================================== FAILURES ==============================================================
__________________________________________________ test_backend_context_manager[2] ___________________________________________________

backend_name = 'test_backend_0'

    def check_backend_context_manager(backend_name):
>       with parallel_backend(backend_name, n_jobs=3):

joblib/test/test_parallel.py:563: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/volatile/le243287/miniconda3/lib/python3.5/contextlib.py:59: in __enter__
    return next(self.gen)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

backend = 'test_backend_0', n_jobs = 3, backend_params = {}

    @contextmanager
    def parallel_backend(backend, n_jobs=-1, **backend_params):
        """Change the default backend used by Parallel inside a with block.

        If ``backend`` is a string it must match a previously registered
        implementation using the ``register_parallel_backend`` function.

        Alternatively backend can be passed directly as an instance.

        By default all available workers will be used (``n_jobs=-1``) unless the
        caller passes an explicit value for the ``n_jobs`` parameter.

        This is an alternative to passing a ``backend='backend_name'`` argument to
        the ``Parallel`` class constructor. It is particularly useful when calling
        into library code that uses joblib internally but does not expose the
        backend argument in its own API.

        >>> from operator import neg
        >>> with parallel_backend('threading'):
        ...     print(Parallel()(delayed(neg)(i + 1) for i in range(5)))
        ...
        [-1, -2, -3, -4, -5]

        Warning: this function is experimental and subject to change in a future
        version of joblib.

        .. versionadded:: 0.10

        """
        if isinstance(backend, _basestring):
>           backend = BACKENDS[backend](**backend_params)
E           KeyError: 'test_backend_0'

joblib/parallel.py:97: KeyError

It looks like BACKENDS is updated in test_backend_context_manager but not in check_backend_context_manager and I seem to remember it was actually a py.test feature but I can't find it at the moment ...

Also I think if we want the move to py.test seriously, the first thing to do once this failure is sorted out is to get rid of the nose with_setup and replace it by the py.test equivalent.

[kdexd]

@lesteve This error was really funny for me, it raised an error with pytest < 3.0.0 else it didn't. Strange that error depends on pytest version. Thanks for clarifications, I'll start with next phase of migration on 18th November, I've got semester exams at my university :smile:

[lesteve]

Hmmm still get the error with py.test 3.0.3 ...

[lesteve]

Maybe the warnings we get with 3.0.3 gives an hint that tests using yield are not fully supported:

/home/le243287/dev/joblib/joblib/test/test_parallel.py yield tests are deprecated, and scheduled to be removed in pytest 4.0

[kdexd]

Hi @aabadie @GaelVaroquaux @lesteve:
Okay so I've prepared a roadmap for migration to pytest:

• [x] Remove all nose imports from test files and bring them to one module (joblib.testing). This basically aggregates everything for us to change it in the future. We'll not violate _Don't Repeat yourselves_.
• [x] Pytest gives helpful messages in error logs by using assert keyword based statements. (Refer here). Besides these are same:

assert_true(a == b)
assert_equal(a, b)

... but using either of them is developer's discretion. Whereas assert keywords:

assert a == b         # ... look more pythonic

• [ ] Next step would be to make a conftest.py file and declare necessary fixtures for setups and teardowns.

• [x] Pytest is deprecating yield based tests after 3.0.0 due to certain disadvantages. So we'll use @pytest.mark.parametrize to test any method with multiple test cases.

Finally after the basic migration is done, I can add in more to improve the current test methods. On the way, I will have to do dirty hacks but I'll put a _TODO_ comment which will be taken up in subsequent phases of migration. I believe if I jump too far, I might confuse myself and end up breaking things.

First phase is done. I am submitting a Pull Request for second phase now.

[lesteve]

Thanks a lot for getting back to this @karandesai-96 ! I think the first thing to do would be to understand the failures mentioned in https://github.com/joblib/joblib/issues/411#issuecomment-258797680. Rewriting the failing tests without yield could be an acceptable work-around.

IMO, replacing the assert_ helpers should be done at the very end (probably quite tedious even if you could imagine writing a script to do it).

[kdexd]

@lesteve While executing a run with py.test command, the assert keyword statements do not raise native exceptions.AssertionError. pytest has its own _pytest.assertion.reinterpret.AssertionError, which makes a beautiful error log. Before you commented, I already replaced all methods with pythonic assert statements by some regex replacing. I didn't even write a script for it, Pycharm Professional IDE is very powerful for refactoring businesses. I left the yield statements untouched. Finally I did some proof reading and within half an hour everything was ready. Current tests works fine with nosetests joblib comment yet those three methods still fail with py.test.

I understood the problem and now I'm rewriting the three failing tests mentioned in #411 (comment). I hope it doesn't take more time.

[kdexd]

@lesteve We'll hit a checkpoint with my next PR, where I would deal with #411 (comment)

[kdexd]

So finally, the crucial parts of the migration can be said to be over! Time for the last phase, I'll start off with disk utility and function inspect tests.

[kdexd]

Next up: test_hashing.py and test_logger.py modules.

There's a point worth noting for test_hashing.py -

• There are currently a few yield based tests which also use a @with_numpy decorator. Some of them build numpy arrays for test cases in multiple steps.
• This cannot be done in arguments of @parametrize decorator itself as it will not work in absence of numpy.
• Covering this @parametrize decorator with a @skipif(not np) is a bit buggy ( https://github.com/pytest-dev/pytest/issues/1296 ).

• I referred the docs and concluded that the best way is to make a fixture for a particular test and provide the test case through it. This fixture will be decorated using @with_numpy to steer clear from errors. Other attributes (expected values for example) will have to be combined with @parametrize through something known as "indirect parametrization".

Other alternatives expected to fail:

• Pytest has not designed itself in a way that a fixture can be directly used in @parametrize marker as @parametrize works during the setup of whole test suite, while a fixture function gets executed "when first called". This ensures fast boot of test suite but a casualty (for us) - direct usage is restricted.

• Pytest allows fixtures to be parametrized directly, providing different setups different times, so that the functions using them get executed multiple times. But that has syntax @fixture(params=[testcases...]) which again leaves the numpy specific code uncovered.

For someone who is new to pytest, it can be tricky to understand - I assume you have been using nose for a while so I will be happy to clarify any part of what I said if it is ambiguous to understand. This will be clearer once I make a PR for test_hashing.py
CC: @lesteve @GaelVaroquaux

[lesteve]

This cannot be done in arguments of @parametrize decorator itself as it will not work in absence of numpy.

Covering this @parametrize decorator with a @skipif(not np) is a bit buggy ( pytest-dev/pytest#1296 ).

I don't really understand this. Can you open a PR (even if it is super WIP) to highlight the issue and we discuss the problem there to avoid polluting this issue too much? I looked at the pytest issue and I don't think we are such in a complicated setup.

[kdexd]

@lesteve Please refer here https://github.com/joblib/joblib/pull/454/files#r91969309

If this is not clear then I will move a demo PR.

[nicoddemus]

Hi, I understand that the self.assert* functions have been already converted by @karandesai-96 using PyCharm, but just wanted to mention that one can use unittest2pytest to automatically do that conversion in the future.

[lesteve]

just wanted to mention that one can use unittest2pytest to automatically do that conversion in the future.

Thanks @nicoddemus I'll make sure to try out this tool for the next project I move over to pytest.

[nicoddemus]

Sorry about the slightly out of topic comment, but since this is a thread about pytest and PyCharm was mentioned, perhaps those interested would like to vote on this issue as well: PY-10713 Special support for py.test code completion. 😁

[kdexd]

@nicoddemus I have been used pytest for a few projects now, although this is the first time I am undertaking a migration from nose to pytest. Thanks for this valuable information :smile:

[nicoddemus]

Absolutely. I landed here from pytest-dev/pytest#1296. 😁

[kdexd]

perhaps those interested would like to vote on this issue as well

Well, I just upvoted the issue !

[kdexd]

@lesteve I am having a look at test_memory.py and I can see that yield based tests have fitted in very deeply here. At some places I feel that instead of yield a group of simple assert statements would also be fine, as they basically test various aspects of a common test case. We need to discuss about refactor of several chunks in this module. I'll keep just this module to a hold for a while and move on to next.

[lesteve]

I quickly looked at test_memory.py. Here is what I think:

• yied which are not inside a for loop can be replaced by assert
• yield inside a for loop could be replaced by using parametrize. If that is not super straightforward to do, I would be in favour of replacing by assert. There is not much point wasting too much time on this.

[GaelVaroquaux]

• yied which are not inside a for loop can be replaced by assert
• yield inside a for loop could be replaced by using parametrize. If that is
not super straightforward to do, I would be in favour of replacing by
assert. There is not much point wasting too much time on this.

+1!

[kdexd]

@lesteve Thanks for a clear workaround !

[lesteve]

@karandesai-96 I think the priority is to remove all the yield from the tests (and once this is done remember to remove the disable-warnings flag from the setup.cfg). All the rest are cosmetic changes, although some of them are very nice they can be done later.

[kdexd]

I think the priority is to remove all the yield from the tests (and once this is done remember to remove the disable-warnings flag from the setup.cfg).

This has been undertaken in #461

[kdexd]

I initially adopted a module wise approach and converted complete modules to adopt the pytest flavor. So far I got along with test_{disk, func_inspect, logger, hashing, testing}.py.

But as @lesteve proposed, a topicwise approach for what's left of other modules, is better to take up. I'll follow up with general PRs which apply one change at a time on all modules. Tasks include:

1. Using tmpdir fixture instead of manually setting and deleting a temporary directory through the builtin tempfile module.

2. Replace all assert_raises and assert_raises_regex with pytest_assert_raises.

3. Use pytest.warns instead of warnings.check_warnings (less noisy logs on failures) ...

And any more general ones before I reach a stage where I could only make improvements on specifically one module (such as better parametrization / fixtures).

[lesteve]

Sounds like a good roadmap to me, enjoy the ride ;-) !

[nicoddemus]

Replace all assert_raises and assert_raises_regex with pytest_assert_raises.

Just curious, why not use pytest.raises directly?

[kdexd]

When this project used nose, nose was imported in almost all the test modules. So to drop nose and introduce pytest easily, we removed all nose imports and put necessary variables (such as with_setup / SkipTest of nose) in joblib.testing module which served as aliases. This still continues in pytest.

So joblib.testing has:

import pytest

pytest_assert_raises = pytest.raises

# old code, soon to be removed
assert_raises_regex = unittest.Testcase.assertRaisesRegex

in all other modules:

from joblib.testing import pytest_assert_raises

Since the transition is going on, we have different variable names as of now..

[kdexd]

@nicoddemus It is really helpful to have someone from the core pytest team looking over this issue thread, I'm optimistic about the fact that if we discuss about a design pattern of tests in near time, you'll have the best alternative :smile: Thanks for being here ! :sunflower:

[nicoddemus]

Since the transition is going on, we have different variable names as of now..

I see, makes perfectly sense, thanks.

It is really helpful to have someone from the core pytest team looking over this issue thread, I'm optimistic about the fact that if we discuss about a design pattern of tests in near time, you'll have the best alternative 😄 Thanks for being here !

Sure, though I'm mostly glancing at the comments, not reviewing the code. I will chime in when I can, but if you have any questions or comments please feel free to ask. 😁

[kdexd]

@lesteve Please have a look at this documentation:
http://doc.pytest.org/en/latest/recwarn.html

There are currently two ways of asserting warnings and both have same amount of functionalities: through a recwarn fixture and through a with block.

Using recwarn might make the flow of test like we used capsys - earlier we did a readouterr and did comparison of strings. With recwarn, we pop and assert message.

Using with blocks will be similar to raises blocks.

If I am correct about the fact that these two features have same functionalities ( @nicoddemus would know better ), then it depends on our personal preference. Which one would you prefer according to current amount of warning assertions ?

One good thing about pytest is that Deprecation and PendingDeprecation warnings are ignored by default unless we allow them explicitly. So a chunk of code becomes redundant hereafter.

[lesteve]

recwarn looks good for checking warnings.

I think we still need capsys for a few use cases. One is checking stdout. Also sometimes the warning happens in a subprocess as in two nested Parallels so I don't think it can be captured in the main process with recwarn.

[lesteve]

One good thing about pytest is that Deprecation and PendingDeprecation warnings are ignored by default unless we allow them explicitly. So a chunk of code becomes redundant hereafter.

Not sure what you mean by "a chunk of code becomes redundant".

[kdexd]

@lesteve I have seen this occuring a lot of times:

# This is a temporary workaround until we get rid of
# inspect.getargspec, see
# https://github.com/joblib/joblib/issues/247
warnings.simplefilter("ignore", DeprecationWarning)

we don't need this anymore :smile:

[kdexd]

I think we still need capsys for a few use cases.

@lesteve Sorry I did not mean to represent recwarn as a replacement of capsys in my comment. I was just saying that the flow of test (aesthetically) with recwarn would look much similar to tests using capsys.. :smile: So with blocks or recwarn is simply based on what seeming more pleasing to our eyes.

[lesteve]

we don't need this anymore :smile:

We have not needed this for a while, actually. Thanks for spotting this, I pushed a fix in master.

Edit: we use inspect.getargspec only on Python 2 and inspect.getfullargspec. inspect.getargspec only gives DeprecationWarning on Python 3.

[lesteve]

@karandesai-96 what else do you think remains besides using pytest.warns for warnings?

[kdexd]

@lesteve Nothing much. Simply tackling each module to parametrize the tests... I guess we'll be done by that. I won't be exercising parametrization unless it looks as clear as a for loop iterating test cases..

[kdexd]

With #465 merged, the last task left to do for completing the pytest migration is to revisit the left out modules and refactor certain tests / make test specific fixtures to make the tests simpler if they become..

All common amendments across all modules are done, as far as I can think of them.

[kdexd]

@lesteve I have look upon the remaining tests, the ones in test_pool.py - all of them use @with_numpy decorator and have significantly different test cases. So it is best not to parametrize them using the decorator - it will twist things up. So finally, this issue can be closed after the above three mentioned PRs are merged...

[kdexd]

@lesteve: Hi, I saw your PR on pytest - it is something you insisted to have in our code, are we going to take these remaining PRs after pytest 3.0.6 is out? Because as for now I think after the last three PRs there is nothing significant left which looks nose-ish..

[lesteve]

@karandesai-96, sorry I have not had time to look at the remaining PRs during the Christmas break but I will certainly do so in the upcoming days.

As you say, the joblib pytest move is nearing its completion and it was great to have you onboard to make it happen so smoothly! Also I think it was a very good preparation towards moving scikit-learn over to pytest. If you feel like helping on this front it would be really awesome!

[kdexd]

sorry I have not had time to look at the remaining PRs during the Christmas break but I will certainly do so in the upcoming days.

@lesteve Sure, no worries, I actually expected less activity around the last weekend due to the celebrations :smile:

As you say, the joblib pytest move is nearing its completion and it was great to have you onboard to make it happen so smoothly!

It was an enjoyable experience for me as well. I had worked with pytest but it was the first time when I undertook a migration.

Also I think it was a very good preparation towards moving scikit-learn over to pytest. If you feel like helping on this front it would be really awesome!

Yes indeed, I have kind of got hold of many things, how the roadmap of PRs should be and how not to let the PR diffs slip out of my hands ! I will be more than happy to take up the same in scikit-learn. I just checked out that you're a reviewer in the scikit-learn team as well. I commented on https://github.com/scikit-learn/scikit-learn/issues/7319 few months ago, when I thought this migration was not as complex as it turned out to be recently. I will ping there again soon. :smile:

[lesteve]

Closing now that all pytest related PRs have been merged.

[lesteve]

Again thanks a lot and great work @karandesai-96!

[ogrisel]

Thanks @karandesai-96 and @lesteve! This work is really appreciated.

[kdexd]

After continuous evaluation and feedback from @lesteve and design suggestions from @nicoddemus, along with motivational approvals from @GaelVaroquaux , here we are, finally closing this issue after a long discussion thread !

I have learnt a lot by nitpicks from @lesteve which would be really helpful to me while design of tests in general. I had familiarity with pytest but never had done a migration of a moderately big project. This workflow is fresh in my memories and I will be really happy to take up a bigger task - in scikit-learn. Cheers ! :smile:

[nicoddemus]

Excellent work to all involved! 🎆 😄

[GaelVaroquaux]

Well thanks a lot, Karan, you've done pretty awesome job!