Skip to content
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

📌 Fix location to executable .md files in Thebe #505

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Dec 4, 2024

Users with .md files that contain kernelspecs currently report that the cwd of running kernels in Thebe (via the power-button UX) is incorrect.

Digging into this, Thebe assumes that we have an ipynb file when performing filepath substitution during session provisioning. This means that the cwd is incorrect for .md files.

This PR relaxes the extension check, and introduces a maybe-incorrect .ipynb extension because it's ultimately an arbitrary filename, and we don't then need to use path-handling routines.

Copy link

changeset-bot bot commented Dec 4, 2024

🦋 Changeset detected

Latest commit: e07326e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
@myst-theme/jupyter Patch
@myst-theme/providers Patch
@myst-theme/frontmatter Patch
@myst-theme/diagrams Patch
@myst-theme/site Patch
@myst-theme/styles Patch
@myst-theme/common Patch
@myst-theme/icons Patch
@myst-theme/search Patch
@myst-theme/search-minisearch Patch
@myst-theme/book Patch
@myst-theme/article Patch
myst-to-react Patch
myst-demo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@agoose77 agoose77 requested a review from stevejpurves December 4, 2024 17:42
if (match) {
console.debug('session starter match:', match);
// Choose an arbitrary suffix, not important
path = `${match[1]}/${pageSlug}-${notebookSlug}.ipynb`;
Copy link
Contributor

@stevejpurves stevejpurves Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's the first part of the path here that is important, not the filename - but it's fine (an consistent) that this appears as ipynb in the jupyter lab session manager. Having a .md in the session manager is weird? (i.e. that never occurs even in when people are using jupytext within jupyter lab? or does it).
If we wanted could preserve the extension from the incoming location, secondary to the fix though.

Copy link
Contributor

@stevejpurves stevejpurves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good catch and simple fix 👍
suggest we just merge and leave the ipynb inconsistency if it is one.

@stevejpurves stevejpurves merged commit ceb8196 into main Dec 10, 2024
2 checks passed
@stevejpurves stevejpurves deleted the agoose77/fix-theme-path branch December 10, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants