implement passthrough configuration

akhmerov commented 3 years ago

Closes #17, #24

So far this is an RFC. Does the overall idea seem reasonable, especially as far as removing of most defaults goes? I propose to remove those, so that the defaults can go to thebe repository directly.

TODO:

[ ] decide whether the approach is sound
[ ] decide what to do with kernel options
[ ] update the docs
[ ] update the tests

akhmerov commented 3 years ago

Kernel options configuration: decision needed

Right now we are reading the kernel name from the document metadata, like so:

https://github.com/executablebooks/sphinx-thebe/blob/44ae793880eba585120cfea0af87012b6abf21fc/sphinx_thebe/__init__.py#L70-L76

The approach seems sound, however schema contains extra information.

  // Options for requesting a kernel from the notebook server
  kernelOptions: {
    name: "python3",
    kernelName: "python3",
    path: "."
    // notebook server configuration; not needed with binder
    // serverSettings: <skipped>
  },

I propose to use meta["thebe-kernel"] as a passthrough dictionary with name, kernelName and path. An alternative would be to consider string a special case, which then refers to name (or should it be kernelName?), however I think a direct passthrough is more reliable.

choldgraf commented 3 years ago

Quick thought on document metadata - we want this to work with notebooks that are read into sphinx (and have their metadata brought into the page). So we don't want thebe-specific metadata fields at the document level (unless it's for some functionality that is only specific to thebe)

akhmerov commented 3 years ago

Quick thought on document metadata - we want this to work with notebooks that are read into sphinx (and have their metadata brought into the page).

Wouldn't it be reasonable for the conversion tool to prepare these notebooks for thebe? Note how thebe needs more information than what the kernel knows about, namely path.

choldgraf commented 3 years ago

What conversion tool are you talking about?

akhmerov commented 3 years ago

I see several scenarios:

A specific tool that generates doctrees from notebooks (myst-nb or nbsphinx) can write this metadata.
The user authoring the documents can manually provide the metadata.
Finally, another extension (e.g. jupyter-sphinx) can infer the metadata and provide it before sphinx-thebe reads it.

My point is mainly that thebe kernelOptions isn't the same as notebook metadata that other tools define, and some information may be lost if we try infer kernelOptions rather than provide another passthrough to thebe.

choldgraf commented 3 years ago

My feeling is that other tools shouldn't have to rework their own metadata just to work with thebe, if that metadata already exists. Instead we should try to infer it with thebe since it doesn't rely on us getting other packages to change their behavior - that seems harder to me. But maybe thebe-config can be a way to set it explicitly, and if this isn't set then we try to infer it from document metadata?

akhmerov commented 3 years ago

My feeling is that other tools shouldn't have to rework their own metadata just to work with thebe, if that metadata already exists.

I totally agree, but it wasn't actually clear to me whether this metadata already exists. Perhaps you can help me out here—below is what I found.

Overall we need to determine 3 configuration options:

 // Options for requesting a kernel from the notebook server
  kernelOptions: {
    name: "python3",
    kernelName: "python3",
    path: "."
    // ...
}

`kernelName`

Right now sphinx-thebe reads kernelName from the doc metadata, and I believe this is where myst-nb places it. Is that correct? Inferring the kernel name from the notebook metadata may face limitations down the line (https://github.com/executablebooks/thebe/issues/343), but it's certainly fine for now. If it was just this field, I'd be happy to leave this logic intact :heavy_check_mark:.

`name`

This one I don't actually understand. I see that thebe passes it to kernelManager from @jupyterlab/services, so it seems important, and different from kernelName. What is this actually? Can we somehow infer it? Right now it's left blank. Is it because it's unimportant and can be ignored?

`path`

This is the location where the kernel needs to be started. It seems important since the notebooks may use external assets, and I can imagine this path being document-dependent. For me this is the main uncertain part. Is it safe enough to assume it's the doctree original path? Do we know how to reach it on binder?

choldgraf commented 3 years ago

I think that the most important is kernelName and name is not strictly required (but don't quote me on this). e.g., there's thebe docs that use kernelName without name.

for path I actually do think we will need sphinx-specific information. We need to know the path to the Sphinx root, relative to the repository root (e.g., docs/). Then we can use the page metadata to get the path to the page from the Sphinx root, and then create our own path variable as <path-to-sphinx-root>/<path-to-page>.

So I think we can infer kernelName from the document metadata, and then create the path field using an optional configuration variable along w/ the path to the page. Does that make sense?

choldgraf commented 3 years ago

hey @akhmerov - is this something you're interested in seeing through to the finish line? Another issue popped up (#38 that would be solved by this as well)

akhmerov commented 3 years ago

I will come back to this soonish™ (going through the beginning-of-semester pileup).

joergbrech commented 1 year ago

I think that the most important is kernelName and name is not strictly required (but don't quote me on this). e.g., there's thebe docs that use kernelName without name.

I believe the thebe documentation is wrong here, see https://github.com/executablebooks/thebe/issues/349, https://github.com/executablebooks/sphinx-thebe/issues/59. Since thebe 0.6.0, the kernel is apparently spawned based on name.

executablebooks / sphinx-thebe