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

nightly build health checks and failing notebooks #53

Open
brian-rose opened this issue Jun 30, 2022 · 5 comments
Open

nightly build health checks and failing notebooks #53

brian-rose opened this issue Jun 30, 2022 · 5 comments
Labels
infrastructure Infrastructure related issue

Comments

@brian-rose
Copy link
Member

Currently we set allow_errors: True in our jupyter book _config.yml. This means that if a cell in a notebook throws an error, the build continues and reports success.

So our "health checks" as shown in the nightly build badges aren't really measuring whether the notebooks run.

I wonder if we should toggle the allow_errors setting just for the nightly build, while keeping it for other builds. (At least during the development phase it's very useful to be able to see the rendered book with the errors included)

@brian-rose brian-rose added the infrastructure Infrastructure related issue label Jun 30, 2022
@kmpaul
Copy link
Contributor

kmpaul commented Jun 30, 2022

Personally, I think a True/False toggle is not enough. There may be times in notebooks where, for demonstration purposes, you want to generate an error. So, there should be a way of checking for expected failures. To enable this, I think we need a different mechanism. For that, I think we want the Jupyter Book config to set allow_errors: True, and then we should test the results of each notebook cell against a specific rule. One approach might be nbmake, which you can read a simple blog about here.

@dcamron
Copy link
Contributor

dcamron commented Aug 22, 2022

Takeaway from today's IWG: encourage allow_errors: False to get an idea of overall cookbook health and test its interaction with per-cell raises-exception tags to make sure we can still support @ktyle's mentioned use-case here.

@brian-rose
Copy link
Member Author

Takeaway from today's IWG: encourage allow_errors: False to get an idea of overall cookbook health and test its interaction with per-cell raises-exception tags to make sure we can still support @ktyle's mentioned use-case here.

Yes, this won't catch every instance that we might ideally want to flag (e.g. a cell executes but produces nonsensical results because of bad data), but it will be a big step forward in terms of finding errors through our regular health checks.

@brian-rose
Copy link
Member Author

Looks like I misspoke at yesterday's IWG. We still have allow_errors: True in the template config file.

Some existing cookbooks remove that line and thus get the default allow_errors: False. One example is the CMIP6 Cookbook.

Action items here to close this issue:

  • remove allow_errors: True from the template and existing cookbooks
  • add some documentation to Cookbook contributor's guide about setting the raises-exception tag.

@clyne
Copy link
Contributor

clyne commented Apr 17, 2023

We converged on solution to not set allow_errors, true, and instead rely on cell-level checking.

This should be called out more explicitly in our notebook instructions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Infrastructure related issue
Projects
Status: No status
Development

No branches or pull requests

5 participants