Fallback to default kernel if none specified.

[aebrahim]

We are being overly aggressive by raising an error if a kernel cannot be detected in a notebook. In an environment with only one kernel installed, the notebook executes just fine without errors, and we should not prematurely raise an error.

Fixes #338

[aebrahim]

Just bumping this!

[Erik-Dan-Tran]

Bump

[aebrahim]

Sorry for the direct ping @Borda - this has been sitting for a month and this issue is blocking us. Any chance we can get a review?

[aebrahim]

Sorry to direct ping someone else - @rgbkrk

[rgbkrk]

Can you rebase? This branch is out of date for me to merge. I promise I'll come back 😬

[aebrahim]

@rgbkrk rebased!

[rgbkrk]

Alright so I'm looking at this with fresh eyes now. This is a major breaking change to the API. I would have to make a major release just for this.

What is stopping your system from setting the appropriate metadata in the notebook format?

[aebrahim]

We check in our notebooks to git, and they execute in many different environments, one of which is dagstermill in a "production" setup. There will always be skew for the specifics of the kernel name. For example, on developer machines, it could be arbitrary based on the venv name, and that will certainly not match the production environment.

We therefore strip the kernel metadata from the notebook when we check it in, so every environment can run it in whichever kernel they feel is appropriate. This also prevents lots of churn when multiple people with different settings edit a notebook, which causes endless merge conflicts within git. I don't think we are the only ones who have adopted this solution, because this pre-commit hook seems to be pretty widely used to me: https://github.com/srstevenson/nb-clean

Our production environment which runs papermill has only one kernel available, so falling back to the default kernel would be the behavior which makes the most sense for our use case. I think that the specific issue we face about kernel mismatched names is the same issue faced by the poster in #338

[aebrahim]

Also it's not my project so my opinion on semantic versioning is fairly meaningless, but for what it's worth from my understanding, I think this would be a minor version change, not a major one. Before this PR, papermill never ran notebooks that did not have a version specified, rejecting them as invalid. This PR adds support to execute them by using the default executor, if available. Because it does not break existing supported flows and only adds a new one, I think it would be a minor version change (e.g. for 2.8.0 at this moment in time)

[rgbkrk]

Thanks for that context. That helps. I really hate the mess that is arbitrary kernels per venv rather than having the notebook presume to use the current venv/project from the directory its in. I'm working on a notebook app that handles that in nteract/desktop but it doesn't solve every single place people run notebooks (VS Code, Jupyterlab, classic, etc.) I think I'll want to take your perspectives into account there too.

You've still got language info in the notebook metadata right? Just double checking as then we can rely on the default language kernel matching up with the stated language of the notebook.

[rgbkrk]

I'm still seeing merge conflicts with main. Did you rebase against origin/main after pulling it in?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.17%. Comparing base (cb2eb37) to head (e12aafd).
⚠️ Report is 36 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #860      +/-   ##
==========================================
- Coverage   91.54%   91.17%   -0.38%     
==========================================
  Files          17       17              
  Lines        1621     1632      +11     
==========================================
+ Hits         1484     1488       +4     
- Misses        137      144       +7     

Files with missing lines	Coverage Δ
papermill/utils.py	96.66% <100.00%> (+1.42%)	⬆️

... and 5 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2503090...e12aafd. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[aebrahim]

Rebased again!

[aebrahim]

Thank you for spending so much time thinking about our use case.

The pre-commit hook we use also strips language out. It's not the reason we started using it, but is a consequence of the tool we use. Because we're only running with one kernel available, falling back to the default language for that kernel is the preferred behavior, but I'm sure we could try to figure out how to modify that pre-commit hook if for some reason that was not possible.

[rgbkrk]

I'll have to ship this tomorrow or sometime this week after looking at everything else we've been up to as well.