ENH: Fix GitHub actions notebook test workflow

[jhlegarreta]

Fix GitHub actions notebook test workflow

Fixes #31.

[netlify]

Deploy preview for carpentries-dmri ready!

Built with commit 74da157

https://deploy-preview-96--carpentries-dmri.netlify.app

[jhlegarreta]

We will not know if this works until we merge it. And hopefully we can know by tomorrow if we merge it before.

After what we have discussed so far about the CI tests and cross referencing PRs #41 and #46, this PR proposed to:

• Leave the preprocessing notebook out for now due to our concerns about its dependencies on FSL and ANTs.
• Run the solution files for the notebooks having exercises.
• Run the tests once a week.

As far as I see, our notebooks do not have fill-in-the-gaps like exercises as it was the case in previous versions (cross referencing #85 (comment)). However, I've seen that exercises are only stated in the Markdown files, being the code/*.ipynb and code/*/solutions/*.ipynb files exactly the same. Not sure about the rationale behind (see the related comment in #85 (comment)), but that is something to be addressed in a separate PR.

Adding the non-solutions files to the CI tests is left for a separate PR.

If at some point we switch to fill-in-the-gaps like exercises:

• The corresponding cells will need to be manually marked as not to be executed in the non-solutions file provided that it does not interfere with the notebook execution when teaching (see ENH: Install additional dependencies for GitHub Actions tasks #41 (comment)).
• The solutions file will be run anyway.

If the weekly tests pass after this PR is merged:

• We will need to add the Carpentries checks (see ENH: Run GitHub Actions build-test workflow on PRs #46 (comment)) in a separate PR.
• Regardless of whether we are under the time limit bill:
  • We may want to investigate the cache mechanism across the Python versions used (BUG: Github Actions Cache #88)
  • We may want to add the preprocessing notebook to the tests. If this turns out into a too long test time, we may perform it only monthly.

Triggering tests on PRs might be considered once all the above has hopefully been addressed and we are still under the time limit bill. Or we may think of triggering the tests only for the lightest notebooks on PRs, reduce the number of Python versions tested while keeping the weekly cron job for all notebooks.

So in summary, this PR is ready to be merged and the rest of the enhancements can come in separate PRs.

[josephmje]

Thanks for this @jhlegarreta. Could you try changing the cache key?
key: ${{ runner.os }}-pip-${{ matrix.python-version }}-${{ hashFiles('requirements.txt') }}

and restore keys to

restore-keys: |
    ${{ runner.os }}-pip-${{ matrix.python-version }}-
    ${{ runner.os }}-pip-

[jhlegarreta]

@josephmje Done in 74da157.

[jhlegarreta]

Also, I've seen the CI output from yesterday:
https://github.com/carpentries-incubator/SDC-BIDS-dMRI/runs/2262435829?check_suite_focus=true

We made progress (intro and DTI passing), but now the CSD fails due to a timeout. Cells have 30 minutes to do their job, which seem largely enough; especially for the one failing, which is the FRF display. That cell takes almost no time when running the notebook on the browser, but I've tested locally with the pytest command, and it shows the same behavior as in the CI. A few thoughts:

• I've done some debugging and the statements that trigger this behavior are the window.snapshot calls.
• Mark the relevant cells with # NBVAL_SKIP. Have tried locally and it works. Can be a distracting element: even if we aim for the solutions for now (including CSD and prob tracking eventually), at term we'll want to check the non-solution notebooks, so we'll end up by including it in the relevant cells. Also, it means that we should make statements related to the skipped cells (e.g. scene.rm(response_actor)) either dwell in the same cell or mark to be skipped as well, and splitting the cells that do some processing and visualization to be able to mark with # NBVAL_SKIP only those calling the visualization methods.
• I've tried locally with the --nbval flag instead of --nbval-lax (have put testing the CSD and followed by another notebook to a shell file and have called it): the second notebook gets tested regardless of the flag provided, so not sure why the CI build shows a different behavior.
• The execution does not follow because we are not allowing pytest to collect the notebooks by itself. We could try to ignore tests with the --ignore, e.g.:

pytest --nbval -v code/**/*.ipynb --ignore=code/preprocessing/**/*.ipynb

or

pytest --nbval-lax -v code/**/*.ipynb --ignore=code/preprocessing/**/*.ipynb

I've been unable to successfully exclude the preprocessing notebobok even.

We'd still need to mark the problematic cells with # NBVAL_SKIP to see the tests passing.

Also, that would collect the tests in alphabetic order (i.e. CSD before any other one for the current episodes). Although not a problem, does not follow the lesson workflow.

So we're left with the following possibility to see the tests passing:

• Refactor the cells having some visualization part using fury to isolate such parts and skip them by adding the # NBVAL_SKIP flag to the visualization cells.

[jhlegarreta]

So we're left with the following possibility to see the tests passing:

• Refactor the cells having some visualization part using fury to isolate such parts and skip them by adding the # NBVAL_SKIP flag to the visualization cells.

Cross-referencing #128.