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

Skip doctests if optional dependencies are not installed #8258

Merged

Conversation

GenevieveBuckley
Copy link
Contributor

This PR skips more doctests if the optional dependencies they require are not found in the environment.

This PR is similar to #8256. There, a missing optional pyarrow dependency caused errors in pytest collection so that none of the tests would run. Here, pytest will run but the doctests fail for those where the optional dependencies are not installed.

Following the development installation instructions for conda vs pip gets you two slightly different environments, which is most obvious when you look at what optional dependencies have been included. Our conda approach for a development installation gives you basically everything, but the corresponding pip installation do not automatically come with every optional dependency. I think that's ok, but it means we occasionally have to tidy up situations like this.

  • Closes #xxxx
  • Tests added / passed
  • Passes pre-commit run --all-files

@GenevieveBuckley
Copy link
Contributor Author

I found that if cachey is not installed, the doctests fail in two places. At first I tried this:

try:
    import cachey  # noqa: F401
except ImportError:
    collect_ignore.append("dask/cache.py")
    collect_ignore.append(
        "dask/diagnostics/profile.py::dask.diagnostics.profile.CacheProfiler"
    )

...but found that you can't skip doctests for a single class, you have to skip the entire file (unless you use a tool like pytest-doctestplus, which can skip individual class doctests).

I wanted to keep the other doctests in profile.py and not skip them, so it seemed easier to just install cachey with pip install dask[test]. If you prefer, we can do somethin different. There are three possible ways to handle it:

  1. Install cachey with pip install dask[test] (this is the option I've chosen)
  2. Use pytest-doctestplus to have finer-grained control over which doctests are skipped or not.
  3. Just skip all the doctests in the file dask/diagnostics/profile.py. This file contains the Profiler, ResourceProfiler, and CacheProfiler classes in it, which to me seems too important to just skip all of them. I'm against this.

@jsignell
Copy link
Member

Another option is to just always skip these cachey doctests by adding # doctest: +SKIP to the ends of the lines. That is my personal preference.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for skipping doctests if an optional dependency isn't installed. In fact, we might consider running doctests during our normal test CI builds

py.test dask --runslow $XTRATESTARGS

Currently we run doctests in a separate CI build here

doctest:
runs-on: "ubuntu-latest"
steps:
- name: Checkout source
uses: actions/checkout@v2
- name: Setup Conda Environment
uses: conda-incubator/setup-miniconda@v2
with:
miniforge-variant: Mambaforge
miniforge-version: latest
use-mamba: true
channel-priority: strict
python-version: "3.8"
environment-file: continuous_integration/environment-latest.yaml
activate-environment: test-environment
auto-activate-base: false
- name: Install
shell: bash -l {0}
run: source continuous_integration/scripts/install.sh
- name: Run tests
shell: bash -l {0}
run: pytest -v --doctest-modules --ignore-glob='*/test_*.py' dask

Running doctests anytime we run the test suite, in particular in our mindeps builds, would ensure that we have # doctest: +SKIP in all the appropriate places

@GenevieveBuckley
Copy link
Contributor Author

Another option is to just always skip these cachey doctests by adding # doctest: +SKIP to the ends of the lines. That is my personal preference.

Won't that skip them all of the time then - as in, they'll never run?

+1 for skipping doctests if an optional dependency isn't installed.

@jrbourbeau and @jsignell - it seems like you two have conflicting suggestions on which approach to take. I'm happy to do either, if you can come to an agreement.

@GenevieveBuckley
Copy link
Contributor Author

In fact, we might consider running doctests during our normal test CI builds

What I care most about with this is that if someone follows the developer contribution guide, then the environment should work and the test suite passes every time. Works fine, every time (which is not currently the case, there are odd hiccups with both the dask and distributed developer setup guides here's another example of a hiccup)

@jsignell
Copy link
Member

Won't that skip them all of the time then - as in, they'll never run?

Yeah that is what I meant. I think that is the philosophy that we have about doctests. If they are a pain in some way we should feel free to skip them. Unit tests should be the real tests and the purpose of running doctests is just to ensure that they don't drift out of date.

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like you two have conflicting suggestions on which approach to take. I'm happy to do either, if you can come to an agreement.

Good point. After re-reading my comment I clearly had a mental lapse -- I don't think it's possible to conditionally skip doctests based on if a package is installed like it is in unit tests with pytest.importorskip 🤦 Let's go with @jsignell's suggestion

@GenevieveBuckley
Copy link
Contributor Author

Ok, done. We are now more aggressively skipping doctests that depend on cachey (so those doctests should never run now).

It seems there were already a few # doctest: +SKIP statements, we just needed to be careful to have those on every line.

@GenevieveBuckley
Copy link
Contributor Author

I think if you're happy with it, this is probably ready to merge now.

@jsignell jsignell merged commit 6dbf095 into dask:main Oct 21, 2021
@GenevieveBuckley GenevieveBuckley deleted the skip-docstrings-deps-not-installed branch October 21, 2021 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants