doctest for .rst files

[awaelchli]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Follow up to #1484 in a series of doc improvements.
This PR adds doctesting for .rst files and converts existing code blocks to proper doctests.
Note:

Notes:

• For the tests to work I had to change imports like pl.LightningModule to because otherwise there was an import error because in docs the __LIGHTNING_SETUP__ variable is set.
• Even the doctests that don't test for outputs (like class defs) are useful because it still checks syntax. In fact, while converting them I found syntax errors that would otherwise not have been detected. So if someone copy pastes from docs, it is less likely they need to fix a typo we introduced.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[Borda]

pls add also testing RST files with pytest along with the existing testing because now it is tested only against one package's configuration but we would like to cover at least min and max package version coverage >> add it to Github action CI

[awaelchli]

this is just a draft and I'm experimenting. I don't know how to do all these things but I will figure it out.

P.S. you deleted all my content in the PR description. can it be undone? I have to type it again :(

[Borda]

P.S. you deleted all my content in the PR description. can it be undone? I have to type it again :(

I didn't, you can see the version history as next to the content title is edited by ...

[codecov]

Codecov Report

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

@@          Coverage Diff           @@
##           master   #1511   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          69      69           
  Lines        4151    4151           
======================================
+ Hits         3661    3670    +9     
+ Misses        490     481    -9     

[awaelchli]

@Borda The doctestplus you suggested seems not to work very well with sphinx. It is a library to run doctests on standalone rst files with pytest. However, since we are using rst files with sphinx, doctestplus is not optimal. First of all, sphinx doctests support things like this:

.. doctest::
    :skipif: torch.cuda.device_count() < 2
    >>> Trainer(gpus=2)

But this is not supported by doctestplus and it would be nice to run our doctests conditionally.
So, I suggest to use simply the sphinx built in doctest.
What do you think? Do you have any other suggestions?

[Borda]

the point is that the Read-The-docs is running the build on quite a simple machine so we do not want to take it too long... let's try it with sphinx only and see how it goes :]

[Borda]

@awaelchli can we turn it for review?

[Borda]

The doctestplus you suggested seems not to work very well with sphinx

what does it mean, some tests are skipped or failing?

[awaelchli]

No it means it is not designed to be used together with sphinx. It will ignore directives like

.. doctest::
    :skipif: torch.cuda.device_count() < 2
    >>> Trainer(gpus=2)

Sphinx has its own doctest extension.

[williamFalcon]

@awaelchli @Borda shall we merge soon? unless @awaelchli wants to keep rebasing lol... this will get hard to merge the longer it's open :)

[awaelchli]

@williamFalcon sure, I'll try to finish it today, thanks.

[mergify]

This pull request is now in conflict... :(

[awaelchli]

@Borda in the doctests I import loggers, should I add requirements-extra to the docs build or what should I do?

[Borda]

@Borda in the doctests I import loggers, should I add requirements-extra to the docs build or what should I do?

in this case, I think yeas you really need to call some methods... the mock here is not enough, right?

[awaelchli]

hmm, not sure how I could mock them.
It seems not possible to just simply install requirements-extra because of horovod.
Should I remove doctesting for loggers completely?

[Borda]

hmm, not sure how I could mock them.

I think it is not possible if we want to run the tests...

It seems not possible to just simply install requirements-extra because of horovod.

what is the problem with Horovod?

[awaelchli]

It seems CI can't install it if I do pip install -r requirements-extra.txt

[awaelchli]

@tgaddair Hi do you know how I can install or ignore horovod in the docs build CI? It is CPU only and I need to install the logger requiremetns in requirements-extra.txt but it also contains horovod.

[tgaddair]

@tgaddair Hi do you know how I can install or ignore horovod in the docs build CI? It is CPU only and I need to install the logger requiremetns in requirements-extra.txt but it also contains horovod.

Hey @awaelchli, Horovod should be able to install with or without GPU. I can take a quick look at the build output to see why it's failing.

[awaelchli]

Thanks, here it is:
https://circleci.com/gh/PyTorchLightning/pytorch-lightning/25540?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
Ignore the other failures, they are not due to horovod.

[tgaddair]

Thanks, here it is:
https://circleci.com/gh/PyTorchLightning/pytorch-lightning/25540?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
Ignore the other failures, they are not due to horovod.

Thanks. From the logs, it seems the problem is that neither MPI nor CMake are installed. One of those most be installed for Horovod to build. I suggest installing CMake as we do in install_deps:

sudo apt-get update && sudo apt-get install -y cmake

That should take care of the issue.

[awaelchli]

@tgaddair Thanks for the help, it worked.
@Borda the drone has a red cross but when I look at the output, all tests are passing, no mention of a failing test. could you have a quick look and check if you see something obvious I don't see?

[awaelchli]

Can't invest more time into trying to make the doctests run on drone. I have tried everything, there is just no output that would suggest any failures. So I suggest to drop it for drone and just keep it in the circle ci.

PR is now ready for review

[Borda]

Can't invest more time into trying to make the doctests run on drone. I have tried everything, there is just no output that would suggest any failures. So I suggest to drop it for drone and just keep it in the circle ci.

I am fine with having it now at least for CPU (CircleCI) and GPU we can add later...

[Borda]

I like his testing VERY MUCH ❤️

[awaelchli]

Looks like ReadTheDocs did not like what we modified in the conf.py. Some pages like Trainer and LightningModule are broken. I suspect it's the mocking of packages. Will track this down and submit a follow up PR.

[Borda]

@awaelchli could pls prepare fix as main repo branch and I will turn on building this branch so we will constantly know how it is going...