Skip to content

Revisions for current intake-esm #12

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 2 commits into from
Dec 8, 2022
Merged

Revisions for current intake-esm #12

merged 2 commits into from
Dec 8, 2022

Conversation

ktyle
Copy link
Contributor

@ktyle ktyle commented Dec 7, 2022

This PR updates the notebooks/foundations/enhanced-catalog notebook to resolve various errors related to changes in the underlying intake and intake-esm libraries. Also took an opportunity to refresh the content a bit, based on current examples in https://intake-esm.readthedocs.io/en/stable/index.html .

@ktyle ktyle requested review from brian-rose and r-ford December 7, 2022 22:18
@github-actions
Copy link

github-actions bot commented Dec 7, 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: 4e58baf
✅ Deployment Preview URL: In Progress

@ktyle
Copy link
Contributor Author

ktyle commented Dec 7, 2022

The build is failing but that is due to as-yet unresolved issues with the example-workflows/key-figures notebook.

@brian-rose
Copy link
Member

@ktyle thanks for opening this! I'll take a look at the notebook tonight.

It seems that your PR commits the output of the enhanced-catalog.ipynb notebook. That's handy for now so I can easily look at it, but is it violating the spirit of our push for reproducible notebooks? Should we always strip output before committing and let the CI system do the execution (as we enforce for Foundations)?

I'm not sure we ever came up with a clear policy about this for the Cookbooks.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 7, 2022

Yes, I should have cleared the output before committing. Although I think we should have an exception for #3 ...

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 pushed a new commit that cleared the output.

Before doing this, I verified that the notebook runs cleanly on my own system. I also see that it ran cleanly on our CI system.

I really like the modifications to this notebook! Everything reads well and is clearer than the original.

I suggest we merge this update, and then open a new PR to deal with #3

@brian-rose
Copy link
Member

I think that if we merge this, the update will not get published because of the build failure related to #3.

A short-term solution, as discussed in #3, is to commit a copy of the key-figures notebook that contains locally-rendered output.

But we will also have to change the execution settings in the config file to a format that will not try to execute that notebook.

e.g. change execute_notebooks: binder to execute_notebooks: auto
(which will not try to execute a notebook that does not have missing output)

@ktyle
Copy link
Contributor Author

ktyle commented Dec 8, 2022

hmmm I wonder if there is a more granular way in jupyterbook to "opt out" certain notebooks from being executed.

But in this case, since there are just two notebooks in the cookbook, setting the flag to "auto" seems reasonable. I propose we merge this PR, and then create a new PR that contains the fully-executed key-figures notebook and also switches the build flag to auto.

@ktyle
Copy link
Contributor Author

ktyle commented Dec 8, 2022

(and having fully read your comment, @brian-rose , we're on the same page so I will go ahead and merge this and then embark on a PR that deals with #3 (tomorrow morning!).

@ktyle ktyle merged commit 18a0c47 into ProjectPythia:main Dec 8, 2022
@ktyle ktyle deleted the dec22 branch December 8, 2022 03:03
@brian-rose
Copy link
Member

@ktyle I think that auto is the granular way, in the sense that JupyterBook will skip execution of a notebook if and only if it doesn't have any missing output.

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