-
Notifications
You must be signed in to change notification settings - Fork 15
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
implement passthrough configuration #25
base: main
Are you sure you want to change the base?
Conversation
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 looks pretty reasonable to me!
}}, | ||
predefinedOutput: true | ||
}} | ||
{ json.dumps(thebe_config) } |
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.
Is there any reason we'd wanna do any data validation or anything?
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.
I think validation is a great idea, however the best approach to implement validation would be jupyter-book/thebe#398.
Kernel options configuration: decision neededRight now we are reading the kernel name from the document metadata, like so: sphinx-thebe/sphinx_thebe/__init__.py Lines 70 to 76 in 44ae793
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 |
cm_language = kernel_name | ||
if "python" in cm_language: | ||
cm_language = "python" | ||
elif cm_language == "ir": | ||
cm_language = "r" |
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.
Codemirror language is in principle kernel-dependent, however I would like to remove this bit. I think that thebe should configure the codemirror language by talking to the kernel instead. @choldgraf do you agree?
Also currently jupyter-book/thebe#241 seems to render this irrelevant.
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.
Well the main challenge here is that kernels can have all kinds of wacky names (eg, conda-python-3) , while codemirror only uses "canonical" names for languages (eg, "python") so this logic will need to.be somewhere
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.
Once thebe is talking with the kernel and it's running, thebe can read from codemirror_mode
kernel metadata key, without any config. This would avoid flaky heuristics, and would therefore be a more reliable approach. Or am I missing something?
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.
that would require thebe to wait for the kernel to be ready and then update the DOM once that finishes, right?
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.
You're right, this is poor UX. I would still prefer to make the logic more reliable. In principle, notebook metadata should already have the codemirror mode specified directly. Any idea how we could get it?
An alternative approach is to assume that the necessary kernel is reachable locally in the environment we use to build the docs, and we could inspect that kernel for metadata. Would that be OK?
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) |
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. |
What conversion tool are you talking about? |
I see several scenarios:
My point is mainly that thebe |
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? |
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: "."
// ...
}
|
I think that the most important is for So I think we can infer |
I will come back to this soonish™ (going through the beginning-of-semester pileup). |
I believe the thebe documentation is wrong here, see jupyter-book/thebe#349, #59. Since thebe 0.6.0, the kernel is apparently spawned based on |
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: