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

[patch] optionally pass kernel to papermill (headless notebook execution) #131

Merged
merged 7 commits into from
Sep 4, 2024

Conversation

mbruns91
Copy link
Contributor

@mbruns91 mbruns91 commented Sep 3, 2024

This a quick way to ensure that an existing kernel is used by papermill, ignoring whatever kernelspec is given as metadata in the .ipynb file.

One could make the kernel specification an optional input (with python3 being the default), but I think to effectively make use of that, a lot of adaptations were required and I don't really see the benefit.

@mbruns91 mbruns91 requested a review from liamhuber September 3, 2024 15:01
@mbruns91 mbruns91 linked an issue Sep 3, 2024 that may be closed by this pull request
Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

Brilliant. I like the change.

I am fine with this being the new default behaviour, but in order to guarantee that it is backwards compatible enough to qualify for just a minor bump, please

  • Make the kernel variable in the .sh script (can default to the currently hard-coded value)
  • Expose it as input in the notebooks action (kernel?)
  • Expose it as input in the push-pull workflow (notebooks-kernel?)

@mbruns91
Copy link
Contributor Author

mbruns91 commented Sep 4, 2024

Brilliant. I like the change.

I am fine with this being the new default behaviour, but in order to guarantee that it is backwards compatible enough to qualify for just a minor bump, please

  • Make the kernel variable in the .sh script (can default to the currently hard-coded value)
  • Expose it as input in the notebooks action (kernel?)
  • Expose it as input in the push-pull workflow (notebooks-kernel?)

Did that. Note that now in all 3 places (.support/build_notebooks.sh, build-notebooks/action.yml and .github/workflows/push-pull.yml) the kernel defaults to python3.

@mbruns91 mbruns91 changed the title specify kernel in '.support/build_notebooks.sh' optionally pass kernel to papermill (headless notebook execution) Sep 4, 2024
@mbruns91
Copy link
Contributor Author

mbruns91 commented Sep 4, 2024

BTW, I request here to merge into v3.3.0, which is maybe wrong if I understand correctly.
I should merge into main, and then a release v3.3.1 is generated from there, creating respective branch etc., right?

@mbruns91 mbruns91 changed the title optionally pass kernel to papermill (headless notebook execution) [minor] optionally pass kernel to papermill (headless notebook execution) Sep 4, 2024
@liamhuber liamhuber changed the base branch from v3.3.0 to main September 4, 2024 13:41
Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

Nice!

Yes, it should target main. The steps now are:

  • Run .support/update_actions_tag.sh main to re-target main
  • Merge to main
  • Checkout a new branch from main v3.3.1
    • (I'll update the title here to "patch" accordingly, IMO this is subtle enough to qualify)
  • Run .support/update_actions_tag.sh to target the branch
  • Push to remote (but don't open a PR!)
  • Go to releases and make a new release actions-3.3.1
    • Set the release to target the branch v3.3.1
    • Manually set the previous release as actions-3.3.0
    • Generate release notes and hit release as usual

This release cycle is a little annoying, but I haven't worked out anything better that ensures that versions always internally reference their own version.

@mbruns91 mbruns91 changed the title [minor] optionally pass kernel to papermill (headless notebook execution) [patch] optionally pass kernel to papermill (headless notebook execution) Sep 4, 2024
@mbruns91 mbruns91 merged commit 72012de into main Sep 4, 2024
@mbruns91 mbruns91 deleted the pass-kernelname branch September 4, 2024 15:59
@mbruns91
Copy link
Contributor Author

mbruns91 commented Sep 4, 2024

Alright, I hope I did it all correctly. Thanks for your instructions 👍

@liamhuber
Copy link
Member

Alright, I hope I did it all correctly. Thanks for your instructions 👍

@mbruns91 yeah, super! Nice deployment 🚀 The only thing that was off was your new release wasn't tagged as "latest", but that was a super easy edit on the release page.

TBH I'm still not 100% satisfied with the process -- e.g. the releases internally target the branch they are released from (which is mutable and thus a security risk) instead of the actual tag we create (which is immutable and therefore verifiable as safe), but it's just so darned tricky given that the actions internally target other actions in the same repo. So I'm open to suggestions for improvement. That's future stuff though, you executed the existing release flow great!

@liamhuber liamhuber mentioned this pull request Oct 2, 2024
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.

explicitly set the kernel to be used in '.support/build_notebooks.sh'
2 participants