Include notebook tutorials in documentation

[ernestum]

Description

While cleaning up the documentation, I found a nice way to integrate the tutorial notebooks with the sphinx documentation.

There is a number of redundant options we could also remove in this PR:

1. Since the notebooks are executed during the conversion to HTML, they are implicitly tested and we can consider removing the test_examples.py:test_run_tutorial_notebooks_notebooks test.
2. In the docs circleci job, we could remove the make doctest step since this is executed by the readthedocs job anyways.
3. I think the integration of the tutorials in the sidebar is optional. I am happy to remove it there when you think it is too cluttered. The tutorials are still discoverable via the algorithm articles.

Testing

Description of how you've tested your changes.

[codecov]

Codecov Report

Merging #565 (a364a0e) into master (32467bf) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
- Coverage   97.28%   97.23%   -0.06%     
==========================================
  Files          88       88              
  Lines        8030     8028       -2     
==========================================
- Hits         7812     7806       -6     
- Misses        218      222       +4     

Impacted Files	Coverage Δ
tests/test_examples.py	100.00% <100.00%> (ø)
src/imitation/envs/examples/model_envs.py	96.15% <0.00%> (-3.08%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[AdamGleave]

In principle seems good, nice to have everything in one place, though we should still link to the examples as these kinds of notebooks are better if people "follow along" rather than just passively reading it.

My only concern is that this may end up making building the docs too slow? Doesn't seem too bad right now: ReadTheDocs is just under 6 minutes, which is fine for CI. But what about for pre-commit hooks? We build docs right now in ci/code_checks.sh, and I don't want that to take more than 30s max as it'd make commits painfully slow. Perhaps we can cache or just exclude building these parts of the doc?

[ernestum]

Building docs becomes slower but test should pass faster if we decide to remove the notebook tests.

Good point with the building docs in the pre-commit hooks. I think there is a way to cache the results. I will have a look into this.

[ernestum]

I switched from nbsphinx to myst_nb which supports caching. So the notebooks are only executed in the first run and then only when their contents changes.

Also setting the environment variable NB_EXECUTION_MODE="off" will disable any execution of notebooks during the sphinx build.

[ernestum]

Where do you think we should place the reference to the examples (now called tutorials)? Options that I see:

1. Each notebook links to itself in github at the top
2. We link the folder (on github) on the First Steps page.
3. We add the a "open in colab" badge to the top of each notebook (details see here

What do you prefer?
I am reluctant towards 3. since then we need to ensure the notebooks actually run on colab, which we can not automatically test.
My favorite would be 2. since it does not involve changes to the notebooks.

[AdamGleave]

Where do you think we should place the reference to the examples (now called tutorials)? Options that I see:

1. Each notebook links to itself in github at the top
2. We link the folder (on github) on the First Steps page.
3. We add the a "open in colab" badge to the top of each notebook (details see here

What do you prefer? I am reluctant towards 3. since then we need to ensure the notebooks actually run on colab, which we can not automatically test. My favorite would be 2. since it does not involve changes to the notebooks.

I agree 3 is more effort than it's worth -- I doubt things will work with Colab out of the box.

2. "First steps" though seems a bit user unfriendly. It's an easy detail to miss.

Of these options my vote would be for 1). But better yet would be if we could make Sphinx automatically add a link? It already includes a (somewhat subtle) edit link, so perhaps we could mod that. Not worth much time though. executablebooks/MyST-NB#148 is a related issue but with no clear resolution.

[AdamGleave]

Change largely LGTM.

In addition to the notebook linking issue discussed above, one concern I have is that this change means we are no longer testing the notebooks on Windows/Mac as we are relying entirely on the RTD executor to test them.

Given this, I'm inclined to think it's worth keeping the test for them in test_examples.py. Skipping them in the doctest build makes sense though -- no reason to run the doctests from the example algorithms on both RTD and CircleCI. WDYT?

[ernestum]

In addition to the notebook linking issue discussed above, one concern I have is that this change means we are no longer testing the notebooks on Windows/Mac as we are relying entirely on the RTD executor to test them.

Good point. I added them back again but we skip the test when on linux.

Btw: we can ignore the coverage changes here. They are due to the coverage being measured on linux.

[AdamGleave]

LGTM, one minor suggestion.

We've lost some coverage in the test suite itself because skipping notebooks on Linux, and CodeCov is calculated from the Linux run only...

I agree no point in running the notebooks on both Linux CircleCI and RTD, so I'd be OK adding a # pragma: no cover here (with a comment explaining why.)

[ernestum]

What about the remaining indirect coverage change? Do we also add the pragma: no cover or do we just leave it?

[AdamGleave]

Is the only remaining indirect coverage change the plotting code in src/imitation/envs/examples/model_envs.py?

I'd be fine ignoring that, we're going to remove that file from imitation soon anyway in #541

We try pretty hard to maintain 100% coverage of the tests themselves to make sure we're not missing test code accidentally. But other code we can be much more lenient on. Especially stuff in the examples directory IIRC we actually exclude from the CodeCov calculation.