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.

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

Tweaks to develop.rst from trying to run tests #4772

Merged
merged 4 commits into from May 7, 2019

Conversation

@chrish42
Copy link
Contributor

@chrish42 chrish42 commented May 3, 2019

Trying to follow the instructions in develop.rst and run all the tests, I get a pytest error unless I also install graphviz, python-graphviz and ipython. Here are some tweaks to the docs for that:

  • Mention dependencies needed to get tests to run
  • Also tweak "supported Python versions" line (while I'm here)

Otherwise, I get errors like the following during test collection with pytest:

__________________________________________________________________ ERROR collecting dask/tests/test_dot.py ___________________________________________________________________
dask/tests/test_dot.py:18: in <module>
    from IPython.display import Image, SVG
E   ModuleNotFoundError: No module named 'IPython'
__________________________________________________________________ ERROR collecting dask/tests/test_dot.py ___________________________________________________________________
ImportError while importing test module '/Users/chrish/Code/dask/dask/tests/test_dot.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
dask/tests/test_dot.py:18: in <module>
    from IPython.display import Image, SVG
E   ModuleNotFoundError: No module named 'IPython'
Copy link
Member

@mrocklin mrocklin left a comment

Thanks for doing this @chrish42 ! A small request, but otherwise this looks good to me.

@@ -97,6 +97,10 @@ For development, Dask uses the following additional dependencies::

pip install pytest moto mock

And the following dependencies are required to get all tests to pass::

conda install graphviz python-graphviz ipython
Copy link
Member

@mrocklin mrocklin May 4, 2019

I think that this is the intent of the pip install line just above. Maybe roll those dependencies into here as well so that we don't ask users two do two things when they could do just one.

Copy link
Member

@mrocklin mrocklin May 4, 2019

Also, I think that python-graphviz brings in graphviz.

Alternatively, we might add pytest.importorskip lines in tests that require graphviz. I don't think that people should have to have tools like graphviz or ipython to run our tests successfully.

Copy link
Member

@jcrist jcrist May 6, 2019

Alternatively, we might add pytest.importorskip lines in tests that require graphviz.

This is my preference as well. I fixed a few of these recently, but there's still a few more it seems. The tests should pass fine with just:

# in a clean python environment
pip install -e .[complete]
pip install pytest

py.test dask

Any differences from this is a bug in our test suite.

Copy link
Contributor Author

@chrish42 chrish42 May 6, 2019

Cool, thanks. I'll revert this part of the patch, and figure out instead how to make ipython, etc. optional for the tests.

@chrish42
Copy link
Contributor Author

@chrish42 chrish42 commented May 6, 2019

Work in progress, for IPython & test_dot.py. I had to rework some of the tests. Let me know if that looks right or not. I'll do the other ones and revert the patch to develop.rst in the comint days.

I feel the tests will be back in this situation soon, though, unless there's something in the CI that tests that all the tests pass when only the minimal dev dependencies are installed. It is possible to add that to the CI pipeline?

Also, question. One of the tests that fails if bcolz is not installed is a doctest. Anyone knows how to skip those conditionally? Bonus point if there is an existing example already in the code. Otherwise, I'll search online for the answer.

@chrish42
Copy link
Contributor Author

@chrish42 chrish42 commented May 7, 2019

How about this? I've uninstalled graphiz and IPython, and all tests still pass.

@jcrist
Copy link
Member

@jcrist jcrist commented May 7, 2019

This looks good to me! Thanks for doing this @chrish42. If you're satisfied with the state of this PR and all tests pass, I'm happy to merge.

@chrish42
Copy link
Contributor Author

@chrish42 chrish42 commented May 7, 2019

@jcrist Looks good to me too, please merge. I wish there was a better solution for conditionally skipping doctests than skipping the whole file, but that's not for this pull request to solve.

The only minor point is that this is a bit of a losing battle until there's a CI step configured to run all tests with the minimal environment and fail the build if any test fails. Maybe someone from the core team wants to take this on? Otherwise, thanks!

@jcrist
Copy link
Member

@jcrist jcrist commented May 7, 2019

The only minor point is that this is a bit of a losing battle until there's a CI step configured to run all tests with the minimal environment and fail the build if any test fails. Maybe someone from the core team wants to take this on?

That's tough. Every build we add to our test suite makes the tests take more time. While I think the tests should be able to run with just the core dependencies, this issue has rarely come up in practice (most users don't care about running the tests, and most devs don't care about installing dependencies). That said, we may add a build to ensure this in the future.

Thanks for putting in the effort to get this fixed. Merging.

@jcrist jcrist merged commit 8f84c75 into dask:master May 7, 2019
2 checks passed
@chrish42 chrish42 deleted the dev-setup-doc-update branch May 8, 2019
jorge-pessoa pushed a commit to jorge-pessoa/dask that referenced this issue May 14, 2019
* Mention dependencies needed to get tests to run; tweak "supported Python versions" line

* Skip relevant tests from test_dot if IPython not installed

* Ignore doctest in dask/dask/dataframe/io/io.py, which needs bcolz

* Ignore more doctests; revert develop.rst edit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants