executablebooks / sphinx-thebe

A Sphinx extension to convert static code into interactive code cells with Jupyter, Thebe, and Binder.
https://sphinx-thebe.readthedocs.io/en/latest/
MIT License
28 stars 15 forks source link

Add name to the kernelOptions #60

Closed joergbrech closed 1 year ago

joergbrech commented 1 year ago

Fixes #59.

See also https://github.com/executablebooks/thebe/issues/349.

Follow-up question: Can we have some kind of doctests verifying that all code blocks in the documentation don't error?

Edit from @choldgraf : i think this also closes #43 and closes #53

Edit: Closes https://github.com/executablebooks/jupyter-book/issues/1173

welcome[bot] commented 1 year ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.
Welcome to the EBP community! :tada:

joergbrech commented 1 year ago

Good catch - do you think we can deprecate kernelName or should we keep it?

I honestly don't know :shrug: I wasn't brave enough to deprecate it.

choldgraf commented 1 year ago

@stevejpurves would you be willing to provide guidance on whether we can remove/deprecate kernelName?

stevejpurves commented 1 year ago

I would keep it -- I believe that a kernel launch can fail if the (default) named kernel is not available on the server/service, and if for some reason a kernel named python3 is not there then that would block usage, even though the public binder may have that kernel available.

choldgraf commented 1 year ago

@stevejpurves so name and kernelName serve two different purposes then? Could you link to a doc (or provide briefly here) so that we can cross-link in the sphinx-thebe docs to help others understand when/how to use?

choldgraf commented 1 year ago

ah looking at the comment in the linked issue, it seems that name is the new correct option after thebe 0.6, and kernelName is an old / deprecated option:

Does that sound correct to you @stevejpurves ?

joergbrech commented 1 year ago

I am not entirely sure, but I believe this could fix https://github.com/executablebooks/jupyter-book/issues/1173 also (with a sphinx-thebe release and a bump in the jb dependencies).

choldgraf commented 1 year ago

Yep i think you are right. Let's cross link that issue in the PR as well so that it closes it. We can make a release quickly!

stevejpurves commented 1 year ago

@choldgraf I can't actually find any reference to the two existing side by side outside of thebe. AskernelName has no effect in the current thebe release, and as thebe is moving to not using the Juptyerlab sessions at all - it looks like deprecating kernelName would be ok here after all, and as a breaking change make it clearer for sphinx-thebe users to switch to /usename.

In the meantime, thebe 0.8.x needs an update to provide the backwards compatbility

choldgraf commented 1 year ago

Ah ok, thanks for this clarification - I'd then suggest:

joergbrech commented 1 year ago

make kernelName rename itself to name if given, and raise a warning.

@choldgraf I am afraid I don't understand enough about the interplay of thebe/sphinx/jupyter etc. What do you mean by this? Doesn't sphinx-thebe just parse thebe-kernel from the Markdown metadata and copy it to the thebe html config? Where would kernelName be given, so that it can be renamed?

choldgraf commented 1 year ago

@joergbrech ah I'm being silly - kernelName is chosen by sphinx-thebe, not the user. I think we can just deprecate the use of kernelName entirely by deleting that line. I added comments to make that clearer.

I think we should still open an issue about the soon-to-be-deprecated-again API. @stevejpurves would you be willing to explain in a new issue what we'll need to change?

stevejpurves commented 1 year ago

@choldgraf yes I will - how about you go ahead and release this change first, then I can sit down and test the jb|sphinx-thebe|thebe together and make sure I have a clear picture.

joergbrech commented 1 year ago

@choldgraf just a friendly ping. Are we ready to merge?

welcome[bot] commented 1 year ago

Congrats on your first merged pull request in this project! :tada: congrats
Thank you for contributing, we are very proud of you! :heart:

choldgraf commented 1 year ago

Yep - thanks for the ping. If any of you are interested in having merge rights on this repository, I am happy to provide them. I clearly do not have enough time to maintain this in addition to all the other stuff I have to do, and I don't want to block progress on myself!

joergbrech commented 1 year ago

Yep - thanks for the ping. If any of you are interested in having merge rights on this repository, I am happy to provide them. I clearly do not have enough time to maintain this in addition to all the other stuff I have to do, and I don't want to block progress on myself!

Thanks, but as this is just a small side project, I don't see myself contributing often. Also I know too little about the whole jupyter-sphinx-thebe ecosystem to be of much value. Once this is in jupyter book I will be happy for a while 🙂