Skip tests for optional/extra dependencies when not installed #1113

hectormz · 2020-03-09T01:22:34Z

Description

When running all tests locally ImportErrors are thrown if optional/extra dependencies are not installed. By skipping these tests when not installed, users can still run the entire test suite if desired. The entire test suite should still be run via CI.

This behavior is largely achieved by changing

import xx

to

xx = pytest.importorskip(xx)

which will try to import module xx, and will skip all tests in file if the module is not installed.

Individual tests may also be skipped using the decorator:

@pytest.mark.skipif(importlib.util.find_spec("xx") is None, reason="xx tests only required for CI")

The last remaining issue is numba, which is used in 20+ tests, used in a variety of files. One option is to make numba an entry in requirements-dev.txt instead of requirements-optional.txt, or add the skip decorators to each of the 20+ tests.

This PR addresses Issue #1112 .

Checklist

Follows official PR format
New features are properly documented (with an example if appropriate)?
Code style correct (follows pylint and black guidelines)
Changes are listed in changelog

OriolAbril

Very general and minor comments.

To get pylint off you back you can put the emcee = pytest.importorskip("emcee") like lines below the imports, or set the disable wrong import error at the beginning of the file to avoid the check throughout all the file.

I'd also change the message in the skipifs to "xx not installed" or "test requires xx which is not installed"

Regarding numba, maybe we could move all numba tests to a specific file? Numba is an optional requirement for users (like bokeh) whereas the dependencies in requirements dev are not.

hectormz · 2020-03-09T18:53:51Z

Hi @OriolAbril thanks!

I'll fix the pylint issues, and move the imports down.

I'll update the skipif messages to: "test requires xx which is not installed"

I'll look into moving all the numba files to their own file(s). If it looks a little more complicated organizing them in a clear way, I might move that to a new Issue/PR. Here are the failing numba tests currently:

FAILED tests/base_tests/test_diagnostics.py::test_numba_bfmi - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_rhat[rank] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_rhat[split] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_rhat[folded] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_rhat[z_scale] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_rhat[identity] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_mcse[mean] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_mcse[sd] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_numba_mcse[quantile] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_ks_summary_numba - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_geweke_numba - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_mcse_error_numba[True-1] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_mcse_error_numba[True-20] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_mcse_error_numba[False-1] - ValueError: Numba is not installed
FAILED tests/base_tests/test_diagnostics.py::test_mcse_error_numba[False-20] - ValueError: Numba is not installed
FAILED tests/base_tests/test_stats.py::test_summary_include_circ[True] - ValueError: Numba is not installed
FAILED tests/base_tests/test_stats.py::test_summary_include_circ[False] - ValueError: Numba is not installed
FAILED tests/base_tests/test_stats.py::test_numba_stats - ValueError: Numba is not installed
FAILED tests/base_tests/test_utils.py::test_utils_fixture - ModuleNotFoundError: No module named 'numba'
FAILED tests/base_tests/test_utils.py::test_conditional_jit_numba_decorator_keyword - ValueError: too many values to unpack (expected 2)
FAILED tests/base_tests/test_utils.py::test_numba_utils - ValueError: Numba is not installed
FAILED tests/base_tests/test_utils.py::test_numba_var[0-0] - ValueError: Numba is not installed
FAILED tests/base_tests/test_utils.py::test_numba_var[0-1] - ValueError: Numba is not installed
FAILED tests/base_tests/test_utils.py::test_numba_var[1-0] - ValueError: Numba is not installed
FAILED tests/base_tests/test_utils.py::test_numba_var[1-1] - ValueError: Numba is not installed

The easiest/laziest solution might be to create new files with numba tests:

test_utils_numba.py
test_stats_numba.py
test_diagnostis_numba.py

This would preserve the clarity of the original goal that numba is helping achieve.

hectormz · 2020-03-10T02:49:22Z

Hi @OriolAbril I ended up moving the numba tests as I proposed. I tested locally with and without numba to verify passed or skipped tests

canyon289 · 2020-03-11T02:14:44Z

arviz/tests/helpers.py

-        import stan as pystan  # pylint: disable=import-error
-    return int(pystan.__version__[0])
+        try:
+            import stan as pystan  # pylint: disable=import-error


Curious, why is this extra block of logic needed?

@canyon289 In the event that neither pystan or stan are installed, there would be no imported pystan, which would then have no version attribute. I believe that this function is called via test_data_pystan.py, and will throw an error just while collecting the tests. Even though this file would eventually be skipped because neither libraries are installed via:

pytestmark = pytest.mark.skipif( ~pystan_installed, reason="test requires stan/pystan which is not installed" )

This was my way to handle potentially two exceptions, and also indicate that there is no form of stan installed by returning None. (I have neither stan/pystan package, so this was a solution that I could get to work for me)

Yeah, this sounds fine, maybe this could be import stan --> stan.__version__

Hi @ahartikainen is that just regarding line 580? Still try to import pystan, and if not present try to import stan, but no need to import stan as pystan?

canyon289 · 2020-03-11T02:17:54Z

This pr is great. It does mean that the maintainers need to be extra vigilant with CI and ensure that no tests are skipped before merging. Do you know if theres a way with pytest to fail ir any tests are skipped?

hectormz · 2020-03-11T04:04:29Z

Valid point @canyon289 ! There may be an option that is builtin to pytest (that I haven't found yet), but I came across pytest-error-for-skips :

Pytest plugin to treat skipped tests as test failures.

This is nice if you want to ensure that your CI tests really run all tests and don't skip tests because of missing dependencies.

Simply execute your tests via pytest --error-for-skips ... and all skipped tests become test failures.

But I must imagine that there might be something completely integrated into pytest

hectormz · 2020-03-11T04:20:26Z

2016 pytest issue on the matter (#1364)

ahartikainen · 2020-03-11T04:32:44Z

Could we have env variable which is also tested with the import func?

canyon289 · 2020-03-11T14:48:06Z

Could we have env variable which is also tested with the import func?

Curious what this would do

OriolAbril · 2020-03-11T14:51:48Z

I have to point out that some tests are skipped in CI, some features require the latest version of a given library which means that test can be executed in "external_latest" but not in "external_especial"

hectormz · 2020-03-11T15:18:22Z

I'm guessing @ahartikainen is proposing to create an env variable on the CI machine like ARVIZ_CI_MACHINE. Instead of the importorskip calls at the top, we would do something like:

if os.environ.get('ARVIZ_CI_MACHINE')=='True':
    ARVIZ_CI_MACHINE = True
else:
    ARVIZ_CI_MACHINE = False

try:
    import pystan
    pystan_installed = True
except ImportError:
    pystan_installed = False

pytestmark = pytest.mark.skipif(
    ~pystan_installed & ~ARVIZ_CI_MACHINE, reason="test requires stan/pystan which is not installed"
)

hectormz · 2020-03-11T15:23:09Z

@OriolAbril so it would be a good idea to separate acceptable and unacceptable CI skips?

ahartikainen · 2020-03-11T15:26:00Z

Maybe we could monkeypatch pytest.importskip function?

OriolAbril · 2020-03-11T15:29:20Z

I have seen this example in pytest docs about importorskip usage: http://doc.pytest.org/en/latest/example/parametrize.html#indirect-parametrization-of-optional-implementations-imports, maybe we could base the monkeypatch on something similar?

If CI import and return module, if not in CI use importorskip?

hectormz · 2020-03-22T04:48:05Z

I've been looking at monkeypatching pytest.importskip (and _pytest.outcomes.importorskip) and haven't been able to do so. I'm not sure pytest can monkeypatch itself. I've done this in the context of conftest.py for all tests, or individual tests. I started with just trying to delete importorskip entirely:

@pytest.fixture(autouse=True)
def no_importorskip(monkeypatch):
    """Remove pytest.importorskip for all tests."""
    monkeypatch.delattr("pytest.importorskip")

I think if we want to go down this route, we can implement our own importorskip() (take original code and add ability to check for env variable, and stick it in arviz.tests.utils etc).

ahartikainen · 2020-03-22T08:08:10Z

yeah, that sounds like the correct option

hectormz · 2020-03-22T23:12:08Z

@ahartikainen let me know of this implementation. I also added tests for the custom importorskip to test local and CI experience

ahartikainen

Looks good. I added some minor comments / questions.

LGTM

arviz/tests/external_tests/test_data_pystan.py

ahartikainen · 2020-03-23T07:22:17Z

arviz/tests/helpers.py

-        import stan as pystan  # pylint: disable=import-error
-    return int(pystan.__version__[0])
+        try:
+            import stan as pystan  # pylint: disable=import-error


Yeah, this sounds fine, maybe this could be import stan --> stan.__version__

ahartikainen · 2020-03-23T07:27:37Z

arviz/tests/helpers.py

+            # of existing directories with the same name we're trying to
+            # import but without a __init__.py file
+            warnings.simplefilter("ignore")
+            __import__(modname)


is there any difference between these

__import__(modname) mod = sys.modules[modname]

vs

mod = importlib.import_module(modname)

I think they should be mostly similar for our uses. I used the former as I copied the existing code in pytest.importorksip.

But I can use importlib.import_module. It might clear up the failures in CI to import jax.random.PRNGKey and pyro.infer.Predictive.

I can't seem to easily install jaxlib on Windows, so I can't test this out locally.

hectormz · 2020-03-23T15:54:49Z

In addition to the points raised by @ahartikainen , I'm looking to see why these CI imports failed:

ModuleNotFoundError: No module named 'jax.random.PRNGKey'; 'jax.random' is not a package

ModuleNotFoundError: No module named 'pyro.infer.Predictive'

hectormz · 2020-03-23T17:37:06Z

@ahartikainen I don't think test_precompile_models() in helpers.py is being discovered and run with pytest arviz\tests (I searched through it in verbose log). It might be better to move it to the test_helpers.py file I created.

ahartikainen · 2020-03-23T19:05:40Z

@ahartikainen I don't think test_precompile_models() in helpers.py is being discovered and run with pytest arviz\tests (I searched through it in verbose log). It might be better to move it to the test_helpers.py file I created.

tbh, it is not a test, but a way to precompile models for CI, so at some point, we can start gathering performance data for our tests and they should be reproducible, in most cases.

I cheated with test_ so pytest would work with it.

So users should not run it (without manually calling it)

ahartikainen · 2020-03-23T19:11:27Z

In addition to the points raised by @ahartikainen , I'm looking to see why these CI imports failed:
ModuleNotFoundError: No module named 'jax.random.PRNGKey'; 'jax.random' is not a package

ModuleNotFoundError: No module named 'pyro.infer.Predictive'

edit.
I think we probably need to change the import for the classes

class = getattr(import_module('my_module'), 'my_class')

https://stackoverflow.com/questions/41678073/import-class-from-module-dynamically

hectormz · 2020-03-27T00:12:06Z

tbh, it is not a test, but a way to precompile models for CI, so at some point, we can start gathering performance data for our tests and they should be reproducible, in most cases.

I cheated with test_ so pytest would work with it.

So users should not run it (without manually calling it)

okay got it @ahartikainen

edit.
I think we probably need to change the import for the classes
class = getattr(import_module('my_module'), 'my_class')
https://stackoverflow.com/questions/41678073/import-class-from-module-dynamically

I'll give that shot!

… to fix errors

codecov · 2020-03-30T20:36:00Z

Codecov Report

Merging #1113 into master will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
+ Coverage   92.61%   92.68%   +0.06%     
==========================================
  Files          93       93              
  Lines        8991     9073      +82     
==========================================
+ Hits         8327     8409      +82     
  Misses        664      664

Impacted Files	Coverage Δ
arviz/data/io_cmdstanpy.py	`100.00% <0.00%> (ø)`
arviz/plots/backends/matplotlib/forestplot.py	`96.53% <0.00%> (+0.01%)`	⬆️
arviz/plots/plot_utils.py	`94.66% <0.00%> (+0.01%)`	⬆️
arviz/plots/backends/bokeh/forestplot.py	`92.88% <0.00%> (+0.02%)`	⬆️
arviz/stats/stats_utils.py	`91.66% <0.00%> (+0.02%)`	⬆️
arviz/data/io_pystan.py	`99.07% <0.00%> (+0.04%)`	⬆️
arviz/data/inference_data.py	`83.13% <0.00%> (+0.06%)`	⬆️
arviz/utils.py	`90.95% <0.00%> (+0.14%)`	⬆️
arviz/data/io_cmdstan.py	`94.84% <0.00%> (+0.14%)`	⬆️
arviz/data/io_pyro.py	`95.86% <0.00%> (+0.24%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f76da26...3edc64e. Read the comment docs.

ahartikainen · 2020-03-30T20:45:10Z

Is this now skipping pystan tests?

hectormz · 2020-03-30T20:58:55Z

Seems that way. I was going to investigate why. To sort this out, I setup a virtual machine to mimic the CI, but pystan tests ran locally.

ahartikainen · 2020-03-30T21:33:45Z

Is it trying to import once and then just fails?

ahartikainen · 2020-03-30T21:40:31Z

arviz/tests/external_tests/test_data_pystan.py

+    importlib.util.find_spec("stan") is not None
+)
+pytestmark = pytest.mark.skipif(
+    not pystan_installed & (not running_on_ci()),


not pystan_installed & (not running_on_ci())

not True & (not True) == True

(not True) & (not True) == False

Or you can use

not ( pystan_installed | running_on_ci() )

I think that logic is cleaner. But shouldn't the current logic work because it equates to False, so it shouldn't get skipped

It is currently true (missing parenthesis)

not True & (not True) == True

Ah yes okay. I updated that, will see if it does the trick. And I see that there are some built-in emcee skips looking for emcee3, so that should still be okay

hectormz · 2020-03-30T23:22:06Z

Thanks @ahartikainen for getting this through!

canyon289 · 2020-03-31T05:12:31Z

Looked it over. It looks like it works, there some sections I can't read but I don't want to hold this up :)

canyon289 · 2020-03-31T05:12:53Z

Also this is great work. Looks well implemented. thank you so much for persevering and getting it through

hectormz · 2020-03-31T06:00:35Z

@canyon289 there are sections you physically can't access in order to inspect, or some sections that don't make sense?

If there's something I could clarify or improve to avoid future headache, I'd be happy to update it now that it's working.

Happy to contribute 🙂

canyon289 · 2020-03-31T15:07:45Z

@hectormz It's likely less about your code and more about me not understanding the depth of the pytest internals. I didn't mean that as a criticism of your work!

hectormz · 2020-03-31T15:12:31Z

No offense taken!

OriolAbril reviewed Mar 9, 2020

View reviewed changes

hectormz force-pushed the optional-tests branch 2 times, most recently from 2f43704 to ea585c9 Compare March 9, 2020 19:15

canyon289 reviewed Mar 11, 2020

View reviewed changes

hectormz mentioned this pull request Mar 22, 2020

Skip tests for optional dependencies if they are not installed pyjanitor-devs/pyjanitor#115

Closed

hectormz force-pushed the optional-tests branch from 0efed7a to 0a55e59 Compare March 22, 2020 23:48

ahartikainen approved these changes Mar 23, 2020

View reviewed changes

hectormz added 3 commits March 29, 2020 13:44

Skip tests for optional/extra dependencies when not installed

124e473

Update changelog

6b05556

Fix pystan_version()

ad4a1e1

hectormz added 9 commits March 29, 2020 13:44

Ignore vscode config

25f4cbb

Properly test for local machine with monkeypatch deleting CI env

9b34ae7

Add back in some parts of pytest.importorskip to our importorskip…

30fb64e

… to fix errors

Clarify reason text when test skipped for lack of pystan/pystan3

8b4463d

Refactor helper function to test running on CI machine

dfbc2d3

Ensure individual tests for external requirements only skip locally

4981d78

Use importlib.import_module in importorskip

a02e781

Attempt to fix failing CI imports

173e359

Revert import method

b8ad150

hectormz force-pushed the optional-tests branch from 2bb6522 to b8ad150 Compare March 29, 2020 20:50

hectormz added 3 commits March 30, 2020 00:26

Correct skip logic

a480cd6

Breakup import statements

6c387dc

Correct pydocstyle issues

32bee02

ahartikainen reviewed Mar 30, 2020

View reviewed changes

Correct pystan skip logic

3edc64e

ahartikainen approved these changes Mar 30, 2020

View reviewed changes

ahartikainen merged commit 9909aab into arviz-devs:master Mar 31, 2020

OriolAbril mentioned this pull request Apr 1, 2020

Skip tests relying on absent optional/external libaries #1112

Closed

hectormz mentioned this pull request Jul 9, 2020

[TST] Only skip tests for non-installed packages on local machine pyjanitor-devs/pyjanitor#698

Merged

8 tasks

hectormz deleted the optional-tests branch May 21, 2021 04:22

Skip tests for optional/extra dependencies when not installed #1113

Skip tests for optional/extra dependencies when not installed #1113

Conversation

hectormz commented Mar 9, 2020 • edited Loading

Description

Checklist

OriolAbril left a comment • edited Loading

Choose a reason for hiding this comment

hectormz commented Mar 9, 2020

hectormz commented Mar 10, 2020

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

canyon289 commented Mar 11, 2020

hectormz commented Mar 11, 2020

hectormz commented Mar 11, 2020

ahartikainen commented Mar 11, 2020

canyon289 commented Mar 11, 2020

OriolAbril commented Mar 11, 2020

hectormz commented Mar 11, 2020

hectormz commented Mar 11, 2020

ahartikainen commented Mar 11, 2020

OriolAbril commented Mar 11, 2020

hectormz commented Mar 22, 2020 • edited Loading

ahartikainen commented Mar 22, 2020

hectormz commented Mar 22, 2020

ahartikainen 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

hectormz commented Mar 23, 2020

hectormz commented Mar 23, 2020

ahartikainen commented Mar 23, 2020

ahartikainen commented Mar 23, 2020 • edited Loading

hectormz commented Mar 27, 2020

codecov bot commented Mar 30, 2020 • edited Loading

Codecov Report

ahartikainen commented Mar 30, 2020

hectormz commented Mar 30, 2020

ahartikainen commented Mar 30, 2020

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

hectormz commented Mar 30, 2020

canyon289 commented Mar 31, 2020

canyon289 commented Mar 31, 2020

hectormz commented Mar 31, 2020

canyon289 commented Mar 31, 2020 • edited Loading

hectormz commented Mar 31, 2020

hectormz commented Mar 9, 2020 •

edited

Loading

OriolAbril left a comment •

edited

Loading

hectormz commented Mar 22, 2020 •

edited

Loading

ahartikainen commented Mar 23, 2020 •

edited

Loading

codecov bot commented Mar 30, 2020 •

edited

Loading

canyon289 commented Mar 31, 2020 •

edited

Loading