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

TST: Add Test Execution For Notebook Execution #397

Closed
wants to merge 17 commits into from

Conversation

John-P
Copy link
Contributor

@John-P John-P commented Jul 5, 2022

This PR adds a set of simple unit tests that checks notebooks execute without error. It currently only checks the first two four notebooks (CPU only notebooks). nbconvert may need to be added to requirements (to set the version) if this fails on CI.

There are other ways to test notebooks including special packages. However, this does not introduce any dependencies, runs as if a normal notebook and also gives us an easy way to control which notebooks run (e.g. no GPU).

To Do / Done

  • Clean up file dumping. Some notebooks use a tmp directory to dump files, but others (patch extraction) just dump into the root. This ends up putting many files in with all the notebooks (.ipynb) when testing. Resolved by first copying the notebook to a tmp directory for the test.
  • Filter out apt-get and pip commands. Resolved by loading the notebook JSON and removing lines which match the regex ^!\s*(apt(-get)?|pip).*install. A better solution that could make these cells only execute on Colab directly in the notebook by simply only executing if the COLAB_GPU environment variable exists. Then there will no need to filter lines, and running locally will not execute apt. You can test this in colab vs locally with [[ -z "${COLAB_GPU}" ]] && echo "Not In COLAB" || echo "In COLAB".
  • Inject the tiatoolbox repo to the python path (sys.path) to import correctly on CI and locally during development.
  • Fix bug in coverage aggregation. There is a bug in aggregating multiprocessing test coverage after tests have run (coverage.exceptions.DataError: Can't combine arc data with line data). Supposedly fixed here:

Example Error Output With Partial stderr For Failing GPU Notebook

./tests/test_notebooks.py::TestNotebook::test_notebook[Patch Prediction] Failed: [undefined]Failed: Notebook failed to execute: examples/05-patch-prediction.ipynb Tail of notebook output:
	    209 if not hasattr(torch._C, '_cuda_getDeviceCount'):
	--> 210     raise AssertionError("Torch not compiled with CUDA enabled")
	    211 if _cudart is None:
	    212     raise AssertionError(
	    213         "libcudart functions unavailable. It looks like you have a broken build?")

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@John-P John-P requested a review from shaneahmed July 5, 2022 13:40
@John-P John-P added the dev tools Changes/Updates in Development tools label Jul 5, 2022
@John-P John-P self-assigned this Jul 5, 2022
@John-P John-P changed the title TST: Add Unit Tests For Notebook Execution TST: Add Test Execution For Notebook Execution Jul 7, 2022
@shaneahmed shaneahmed linked an issue Jul 8, 2022 that may be closed by this pull request
@John-P John-P marked this pull request as draft July 17, 2022 19:32
@John-P
Copy link
Contributor Author

John-P commented Jul 19, 2022

See #417 for a simpler test of notebook syntax correctness.

@John-P
Copy link
Contributor Author

John-P commented Jul 21, 2022

@shaneahmed Shall we delete this PR and branch if we are moving to static analysis of notebooks?

@shaneahmed shaneahmed closed this Jul 22, 2022
@shaneahmed shaneahmed deleted the test-notebook-exec branch July 22, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev tools Changes/Updates in Development tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing tests
2 participants