Skip to content

Pre-ran key-figures.ipynb; change JB execution to auto #13

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

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

ktyle
Copy link
Contributor

@ktyle ktyle commented Dec 8, 2022

Addresses #3 . Let's see if everything builds properly ... the fully-run key-figures notebook should not get re-run since Jupyter Book execution is set to auto.

@ktyle ktyle requested review from brian-rose and r-ford December 8, 2022 15:46
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

👋 Thanks for opening this PR! The Cookbook will be automatically built with GitHub Actions. To see the status of your deployment, click below.
🔍 Git commit SHA: d3921bb
✅ Deployment Preview URL: https://ProjectPythia.github.io/cesm-lens-aws-cookbook/_preview/13

@ktyle
Copy link
Contributor Author

ktyle commented Dec 8, 2022

Yay it ran! But I will need to re-run locally, since I had interrupted one of the (long) Dask compute cells after mistakenly re-running it. I also will add some Markdown warning users who want to run it on their own that the notebook will take a long time to run.

Copy link
Member

@brian-rose brian-rose left a comment

Choose a reason for hiding this comment

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

Everything looks great in the preview!

@brian-rose
Copy link
Member

Yay it ran! But I will need to re-run locally, since I had interrupted one of the (long) Dask compute cells after mistakenly re-running it. I also will add some Markdown warning users who want to run it on their own that the notebook will take a long time to run.

Whoops didn't see this. Makes sense to add the warning to users!

I also think we should leave an open issue in the repo re: the aspirational goal of having the whole Cookbook executed during regular health checks.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 8, 2022

looks like NCAR just changed some URL's! New one for LENS CESM looks to be https://www.cesm.ucar.edu/community-projects/lens

Copy link
Member

@r-ford r-ford left a comment

Choose a reason for hiding this comment

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

Besides the one interrupted cell, looks good!

@brian-rose
Copy link
Member

Is that interrupted cell supposed to be there? I'm a bit confused about that.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 9, 2022

Thought I had pushed the version that had the interrupted cell resolved. I will try again ...

@ktyle
Copy link
Contributor Author

ktyle commented Dec 9, 2022

My guess is that this may be an unintended misbehavior in Jupyterbook with the execute=auto flag. Since the previous commit had output in all the cells, even though the newer commit is different, Jupyterbook is not updating the HTML file.

@brian-rose
Copy link
Member

My guess is that this may be an unintended misbehavior in Jupyterbook with the execute=auto flag. Since the previous commit had output in all the cells, even though the newer commit is different, Jupyterbook is not updating the HTML file.

hmm you may be right.

There's another possible strategy: We can specific particular files to be excluded from execution at build time:
https://jupyterbook.org/en/stable/content/execute.html#exclude-files-from-execution

Maybe if we set execute_notebooks: force but add the beefy notebook to the exclude list, we will finally get the desired behavior.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 9, 2022

Could be ... wonder if that will either:

  1. update the previously-rendered HTML
  2. Not update the previously-rendered HTML (same misbehavior as with auto), or
  3. Remove the previously-rendered HTML leaving nothing in its place

I think I'll try building the book locally to see what happens before pushing. Otherwise, seems the only option would be to do a push where the notebook is temporarily removed, then add it back and push again.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 9, 2022

It all seemed to work this time:

  • As far as I can tell, setting execute = auto doesn't stop jupyterbook from re-executing the notebooks ... even if all the cells have been run beforehand.
  • Setting execute = force while specifically excluding a notebook looks to do the trick.
  • The previewer is not being refreshed (why?), and so we are still seeing an earlier-rendered version of the key-figures notebook with an execution error in one of the cells. However, you can see in https://github.com/ktyle/cesm-lens-aws-cookbook/blob/dec22/notebooks/example-workflows/key-figures.ipynb that everything looks good.

I think/hope/pray if we merge, the JB-rendered version will look good as well.

Copy link
Member

@brian-rose brian-rose left a comment

Choose a reason for hiding this comment

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

@ktyle I do see the updated preview, and it looks great!

I think this is good to go. Thanks for your dogged efforts on this.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 9, 2022

Oh yeah I see it now on the preview as well! I was too impatient I guess. I will merge! 😺

@ktyle ktyle merged commit 64e450b into ProjectPythia:main Dec 9, 2022
github-actions bot pushed a commit that referenced this pull request Dec 9, 2022
@brian-rose
Copy link
Member

Oh yeah I see it now on the preview as well! I was too impatient I guess. I will merge! 😺

Correction, thanks for your catted efforts on this.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 9, 2022

😹

@ktyle ktyle deleted the dec22 branch May 24, 2024 18:50
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.

3 participants