-
Notifications
You must be signed in to change notification settings - Fork 7
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
Ensure env is rebuilt when use cached env is set to false #37
Conversation
@r-ford - this should resolve the issue we were having with the environment not rebuilding |
I tried deploying this fix with this CMIP Cookbook PR ProjectPythia/cmip6-cookbook#43 and it is working as expected, fixing the failure to rebuild the env |
Great! I'll take another look tonight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change broke the build for the cookbooks that execute on GitHub Actions, e.g. last night's nightly build of the Cookbook Template
We'll need to revert back to what we had and seek a different fix for the Binder image problem.
@@ -113,7 +113,7 @@ jobs: | |||
if: | | |||
(inputs.use_cached_environment != 'true' | |||
|| steps.cache.outputs.cache-hit != 'true') | |||
&& steps.parse_config.outputs.execute_notebooks != 'binder' | |||
&& steps.parse_config.outputs.execute_notebooks == 'binder' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgrover1 I think this is not the right fix for the problem you were having.
This line was meant to run this workflow step only if executing the notebooks on GitHub Actions. In that case, we need to build an execution environment here.
For executing on binder, the environment.yml
file is ignored and we just build a minimal JupyterBook + binderbot environment (see this line)
If I understand correctly, the problem you were trying to solve is the case where we execute on binder, but there is a change in the environment.yml
file so a new image needs to be built on the binder. Is that correct? We'll need a different fix for that.
Currently the call to binderbot is hard-wired to the main
branch (this line) so we always use the image associated with current main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification here @brian-rose and sorry for merging without getting more feedback first!
This is also a great example of why we need #6! |
This fixes a potential bug in the existing book building code. This is preventing the cmip6-cookbook from rebuilding the image