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

Initial notebook testing framework #480

Merged
merged 15 commits into from
Sep 20, 2021
Merged

Conversation

jklaise
Copy link
Contributor

@jklaise jklaise commented Sep 15, 2021

Testing how we can use either nbmake or jupytext for executing notebooks under pytest.

Working on two new Github Actions workflows:

  • test_all_notebooks - "batch" notebook execution, intended to run manually and at regular (e.g. weekly) intervals
  • test_changed_notebooks - select notebook execution, intended to be triggered by any changes to existing notebooks or new notebooks.

Current version uses nbmake with default options (so 300 second timeout which we may want to increase for certain notebooks or re-design the notebooks to take less time). There is also an example script in testing/test_notebooks.py if we wanted to use jupytext to execute notebooks ourselves. This could be more flexible in the future as it would allow slightly more configuration as well as applying static checkers onto scripts generated by jupytext if we wished and/or papermill to allow passing parameters to notebooks (e.g. this may be more useful for alibi-detect to execute both True and False paths for the load_pretrained_detector variable).

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jklaise jklaise marked this pull request as draft September 15, 2021 16:22
@jklaise jklaise marked this pull request as ready for review September 15, 2021 16:30
@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #480 (af82207) into master (90ff57e) will not change coverage.
The diff coverage is n/a.

❗ Current head af82207 differs from pull request most recent head d611589. Consider uploading reports for the commit d611589 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #480   +/-   ##
=======================================
  Coverage   82.32%   82.32%           
=======================================
  Files          76       76           
  Lines       10316    10316           
=======================================
  Hits         8493     8493           
  Misses       1823     1823           

@jklaise jklaise changed the title WIP: Initial notebook testing framework Initial notebook testing framework Sep 16, 2021
@jklaise jklaise requested a review from ascillitoe September 16, 2021 09:20
Copy link
Contributor

@ascillitoe ascillitoe left a comment

Choose a reason for hiding this comment

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

@jklaise this looks like a nice setup in terms of the github actions and general approach to testing i.e. weekly batch vs testing when changed etc.

Is the plan going forward to leave this here as a PR until we've figured out how to speed execution up with jupytext/papermill?

Also, purely out of curiosity, what sort of checks are you thinking in terms of static checks on jupytext output?

@jklaise
Copy link
Contributor Author

jklaise commented Sep 20, 2021

@ascillitoe my preference would be to merge this (for both libraries) and then start solving issues with the failing notebooks which may involve using other tools such as papermill. Without merging it's more difficult to check the whole notebook test suite as it would need to be run locally, whereas after merging we can trigger the workflow manually on CI.

Purely speculative, but one could run a reduced set of flake8 on the output of jupytext, when I did it for an example it flagged up a lot of irrelevant stuff (e.g. line length for Markdown cells), but also caught some relevant things (e.g. unused imports).

@ascillitoe
Copy link
Contributor

@jklaise, got you, makes total sense on both points 👍🏻

@jklaise jklaise merged commit 16d30ad into SeldonIO:master Sep 20, 2021
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.

2 participants