-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add ability to specify binder environment url along with notebook #464
base: main
Are you sure you want to change the base?
Conversation
- The 'binder_env_url' field is introduced into the 'notebooks' field of the dataset schema, to allow a binder environment to be specified along with a notebook. - This URL will replace the base environment url that was previously hardcoded; while the base url remains as the default. - A fix is added to the flag that sets whether the binder button is displayed or not. - The palmerpenguins dataset metadata is updated to include some arbitrary binder_env_url in order to test the new functionality - An HTMLescape function is added to encode notebook names with e.g. spaces correctly for HTML.
✅ Deploy Preview for datalad-catalog canceled.
|
if (notebook.hasOwnProperty("binder_env_url") && notebook["binder_env_url"]) { | ||
environment_url = notebook["binder_env_url"] | ||
} | ||
} | ||
|
||
binder_url = | ||
environment_url + | ||
"?urlpath=git-pull%3Frepo%3D" + | ||
content_url + | ||
"%26urlpath%3Dnotebooks%252F" + | ||
content_repo_name + | ||
"%252F" + | ||
notebook_name + | ||
"%3Frepourl%3D%22" + | ||
dataset_url + | ||
"%22"; | ||
notebook_name; |
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 you are trying to do too much here, and end up being too restrictive.
AFAIK, the ?urlpath=git-pull
and probably the following bits come from nbgitpuller, which pulls the notebook into the defined environment. This is in general considered good practice (because changing just the notebook does not necessitate binder container rebuild), but what if I have my binder link ready and do not want to use the nbgitpuller?
I do not fully parse the other components at the moment, but it looks like there may be further assumptions.
In my current example, I have a repo with environment and notebook. I generated the Binder URL (https://mybinder.org/v2/gh/sfb1451/rostami_etal_2024-demo/HEAD?labpath=Demo.ipynb
) with the help of https://mybinder.org/. If I were more familiar with nbgitpuller and wanted to use it, I would probably be able to generate a matching URL too.
So... why not just have a metadata property for Binder URL?
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 agree with your observations, and I'd add that I think both should be an option, i.e.:
- have a metadata property on the dataset schema that allows to specify a fully self-contained binder url, which would open an environment that contains the relevant notebook(s)
- have the property on a per-notebook level as proposed in this PR.
This makes it possible to specify a fully custom URL for Binder, without having to use datalad-binder as the "environment" repo. This applies yet-unmerged changes from datalad/datalad-catalog/pull/464 Technical note: done by downloading a PR patch from GitHub, (add .patch to URL), replacing file paths, editing it by hand to exclude commits and changes to irrelevant files, and applying it with Git (patch - apply plain patch in magit). Co-authored-by: Stephan Heunis <[email protected]>
Closes #459