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

Allow mounting jupyterlite content #49

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

martinRenou
Copy link
Member

@martinRenou martinRenou commented Jan 25, 2024

cc. @trungleduc @DerThorsten @jtpio

This allows optionally mounting the jupyterlite content into the kernel.

  • It is disabled by default, not changing the behavior of the kernel
  • It is enabled by default if only generating a voici output

@martinRenou
Copy link
Member Author

What's remaining in this PR:

If /files/ is an existing directory at runtime, we should have the kernel cd into that directory automatically, providing a smooth experience to the user

@martinRenou martinRenou added the enhancement New feature or request label Jan 25, 2024
@jtpio
Copy link
Member

jtpio commented Jan 25, 2024

needs a corner case in Voici for when jupyterlite-xeus addon is there)

Or maybe the other way around? The jupyterlite-xeus addon checking if it is a voici build. So all this logic stays in the place?

@martinRenou
Copy link
Member Author

Right! I'll check how to do that.

We may want to let the user overwrite this behavior if needed.

@martinRenou martinRenou marked this pull request as ready for review January 25, 2024 14:28
None,
allow_none=True,
config=True,
description="Whether or not to mount the jupyterlite content into the kernel. This allows bypassing the file-system core JupyterLite implementation.",
Copy link
Member

Choose a reason for hiding this comment

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

Does it completely bypass, or does it provide a way to bundle additional files at build time?

Copy link
Member

Choose a reason for hiding this comment

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

Answering my own question after checking the code below: it does seem to bypass:

if (await this._remote.isDir('/files')) {
    await this._remote.cd('/files');
} else if (options.mountDrive) {
    await this._remote.cd(localPath);
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something we could add to the docs once we have #17.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it completely bypass, or does it provide a way to bundle additional files at build time?

You can still provide extra mount points 👍🏽 It's just that now jupyterlite-xeus has an extra mount point /files that contains the jupyterlite content.

Answering my own question after checking the code below: it does seem to bypass:

No it does not. The service worker is still enabled. So if you do an os.chdir into the mount point of the service worker based filesystem, you'd be using that.

The code you pointed to is only changing the cwd from where the kernel is starting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed it would be nice to clarify all this in proper documentation

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtpio I updated the description to be a bit more clear

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, thanks for the extra context 👍

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio merged commit 7f79a59 into jupyterlite:main Jan 25, 2024
10 checks passed
@martinRenou martinRenou deleted the mount_jlite_content branch January 25, 2024 18:40
@martinRenou
Copy link
Member Author

Thank you! Let's work on some proper documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants