Skip to content
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

py.test test discovery in the root directory #9699

Closed
rth opened this issue Sep 6, 2017 · 11 comments
Closed

py.test test discovery in the root directory #9699

rth opened this issue Sep 6, 2017 · 11 comments

Comments

@rth
Copy link
Member

rth commented Sep 6, 2017

Not sure if this is addressed somewhere, but with the current migration to pytest, it would be nice if pytest was able to correctly collect unit tests from the scikit-learn root directory. Currently,

cd scikit-learn
py.test -sv .

fails (partial traceback below) because it believes that benchmarks are unit tests a and consequently fails to import some packages.

py.test -sv sklearn/

works correctly. There should be a way to modify the test discovery in pytest settings somewhere I imagine..

cc @lesteve @massich

(partial traceback below)

py.test -sv .               
==================================================================== test session starts ====================================================================
platform linux -- Python 3.6.1, pytest-3.0.7, py-1.4.33, pluggy-0.4.0 -- /home/rth/.miniconda3/envs/sklearn-env/bin/python
cachedir: .cache
rootdir: /home/rth/src/scikit-learn, inifile: setup.cfg
plugins: xdist-1.16.0
collecting 0 itemsPartial import of sklearn during the build process.
collecting 0 items / 17 errorsinput data sparsity: 0.050000
true coef sparsity: 0.000100
test data sparsity: 0.027400
model sparsity: 0.000024
r^2 on test data (dense model) : 0.232730
r^2 on test data (sparse model) : 0.232730
collecting 0 items / 17 errors================================================================================
#    Text vectorizers benchmark
================================================================================

Using a subset of the 20 newsrgoups dataset (11314 documents).
This benchmarks runs in ~20 min ...
^C^C
========================================================================== ERRORS ===========================================================================
______________________________________________________________ ERROR collecting test_speed.py _______________________________________________________________
test_speed.py:17: in <module>
    ipython.magic("%timeit row_norms(Xcsr)")
E   AttributeError: 'NoneType' object has no attribute 'magic'
______________________________________________________________ ERROR collecting test_speed.py _______________________________________________________________
test_speed.py:17: in <module>
    ipython.magic("%timeit row_norms(Xcsr)")
E   AttributeError: 'NoneType' object has no attribute 'magic'
___________________________________________________ ERROR collecting benchmarks/bench_isolation_forest.py ___________________________________________________
benchmarks/bench_isolation_forest.py:10: in <module>
    import matplotlib.pyplot as plt
E   ModuleNotFoundError: No module named 'matplotlib'

[and so on]
@lesteve
Copy link
Member

lesteve commented Sep 6, 2017

Personally I just don't think this is worth the pain. For examples benchmarks can have additional dependencies like pandas, doc/conf.py has a dependency on sphinx, and there may be additional things that I am not aware of.

Maybe we could tweak pytest discovery in a way that make pytest . just work. My personal opinion would be just run the tests with pytest sklearn, forget about this, and use the few hours you just saved to work on something more impactful.

@rth
Copy link
Member Author

rth commented Sep 6, 2017 •
edited
Loading

Personally I just don't think this is worth the pain. For examples benchmarks can have additional dependencies like pandas, doc/conf.py has a dependency on sphinx, and there may be additional things that I am not aware of.

Definitely, that's why IMO they should be excluded from unit tests auto-discovered by py.test

Maybe we could tweak pytest discovery in a way that make pytest . just work. My personal opinion would be just run the tests with pytest sklearn, forget about this, and use the few hours you just saved to work on something more impactful.

I know. Just run into this for the 3rd time because I keep forgetting to prepend the sklearn argument. It's just that, same as when you download a Python project from github you assume, before reading the manual, that it can be installed with pip install . (if it doesn't have too exotic dependencies); you also assume that tests can be run with pytest . ( independently of the fact that it uses nose or pytest).

Just opening this issue in case someones runs into this often enough and becomes motivated to fix it )

@jnothman
Copy link
Member

jnothman commented Sep 7, 2017 via email

@rth
Copy link
Member Author

rth commented Sep 7, 2017

@jnothman No, but,

[tool:pytest]
addopts =
    [...]
    --ignore=benchmarks/
    --ignore=doc/
    --ignore=examples/

should be, I think. Thanks for checking this..

@jnothman
Copy link
Member

jnothman commented Sep 7, 2017 via email

@rth
Copy link
Member Author

rth commented Sep 7, 2017

Well the there is one additional issue is that when running pytest . with these changes as opposed to with pytest sklearn one test fails,

    def test_root_import_all_completeness():
        EXCEPTIONS = ('utils', 'tests', 'base', 'setup')
        for _, modname, _ in pkgutil.walk_packages(path=sklearn.__path__,
                                                   onerror=lambda _: None):
            if '.' in modname or modname.startswith('_') or modname in EXCEPTIONS:
                continue
>           assert_in(modname, sklearn.__all__)
E           AttributeError: module 'sklearn' has no attribute '__all__'

sklearn/tests/test_common.py:145: AttributeError

@jnothman
Copy link
Member

jnothman commented Sep 7, 2017 via email

@rth
Copy link
Member Author

rth commented Sep 26, 2017

Any idea why that happens??

Essentially, if one installs scikit-learn with, python setup.py develop (which is typical for development) then runs py.test . the sklearn.__all__ will not exist because the __SKLEARN_SETUP__ variable seems to be set in setup.py to True. And the sklearn/tests/test_common.py::test_root_import_all_completeness will therefore fail..

@lesteve How about I just skip this test if sklearn.__all__ does not exist? The test seems to be there to ensure that sklearn was installed correctly but I wonder if it's still relevant in development mode...

`

@jnothman
Copy link
Member

jnothman commented Sep 26, 2017 via email

@rth rth mentioned this issue Sep 27, 2017
Closed
@rth
Copy link
Member Author

rth commented Sep 27, 2017

OK, got it: __SKLEARN_SETUP__ is added to buitins in setup.py and pytest was parsing it during the test collection. Fixed in PR above. Looks like @lesteve was right, after all, about the time this would take )

@ogrisel
Copy link
Member

ogrisel commented Oct 2, 2019

This was fixed a while ago (#12011) without closing this issue.

@ogrisel ogrisel closed this as completed Oct 2, 2019
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

Successfully merging a pull request may close this issue.

[MRG] MAINT pytest test discovery rth/scikit-learn
4 participants
@jnothman @ogrisel @rth @lesteve