-
Notifications
You must be signed in to change notification settings - Fork 63
Review Jupyterlab extension docs #700
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
base: main
Are you sure you want to change the base?
Review Jupyterlab extension docs #700
Conversation
Soap2G
left a comment
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.
Hi @ftorradeflot, many thanks, and sorry for missing that!
LGTM.
|
sorry, it wasn't just this change. This was only an initial commit to be able to create the PR I did some more changes and I think it is fine for the time being. But I have two philosophical questions:
|
|
What started being a crazy idea ended up being a reality in a very short time (god bless gemini). I added automatic import of the operator part from the jupyterlab extension repository, so we only have to maintain one copy of the configuration documentation. Feedback will be very welcome @Soap2G |
Soap2G
left a comment
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.
Thanks @ftorradeflot, looks good. I've added a few comments related to CONFIGURATION.md, since it's not a plain copy paste and it might be worth to implement the previous changes in there as well.
| **Requirements:** | ||
| - At least one Rucio instance | ||
| - A storage system attached to the JupyterLab installation via FUSE (Filesystem in Userspace): | ||
| - The storage must be mounted on the host machine or Kubernetes nodes where JupyterLab pods run | ||
| - Common FUSE implementations include: | ||
| - **EOS**: CERN's disk-based storage system (via [`eos fuse mount`](https://eos-docs.web.cern.ch/configuration/fuse.html)) | ||
| - **CephFS**: Distributed filesystem (via [`ceph-fuse`](https://docs.ceph.com/en/latest/man/8/ceph-fuse/)) | ||
| - **XRootD**: High-performance data access protocol (via [`xrootdfs`](https://manpages.debian.org/testing/xrootd-fuse/xrootdfs.1.en.html)) | ||
| - The mounted storage must be registered as a Rucio Storage Element (RSE) in your Rucio instance | ||
| - Should be shared among multiple users with read permissions for all users | ||
| - It's recommended that quotas be disabled, as the extension does not handle quota errors gracefully | ||
|
|
||
| For mounting examples, see the [ESCAPE VRE infrastructure documentation](https://github.com/vre-hub/vre/tree/master/infrastructure) |
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 would leave this part available in the documentation, since it's an additional help for folks that are looking into POSIX mounts.
Could we update the CONFIGURATION.md file to include these items?
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.
Sure! I think this is the way to go
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.
Maybe we could do the same with the User documentation
|
|
||
| Example: `/etc/grid-security/certificates` | ||
|
|
||
| #### VOMS `vomses` Path - `voms_vomses_path` |
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.
If we import CONFIGURATION.md, we should get rid of the --vomsidr option in there, since it's not used anymore in voms-proxy-init.
| In this mode, files are transferred by Rucio to a storage mounted to the JupyterLab server. This is the recommended mode for shared environments. | ||
|
|
||
| **Requirements:** | ||
| - At least one Rucio instance |
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.
In CONFIGURATION.md there's a line here that mentions Jupyterlab 2, while we generally mention >=3 (see this comment)
| ### Instance Configuration | ||
|
|
||
| #### Name - `name` | ||
| A unique machine-readable identifier for the Rucio instance. It is recommended to use FQDN (Fully Qualified Domain Name). Must be declared locally in the configuration file or set via the `RUCIO_NAME` environment variable. |
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.
In COFNIGURATION.md this text is different (there's a generic "declared locally"); we should update it with this text instead.
See this comment
| **2. Add a custom authenticator to `hub.extraConfig`:** | ||
|
|
||
| The `hub.extraConfig` section allows you to inject custom Python code into the JupyterHub configuration. Here, we define a custom authenticator class that handles OIDC token exchange with Rucio. This authenticator intercepts the user authentication flow, exchanges the OIDC token for a Rucio-specific token, and injects it into the spawned user environment. | ||
|
|
||
| See an example implementation in the [ESCAPE VRE Helm Chart](https://github.com/vre-hub/vre/blob/527982d0a9beeb6098dbb9f73e16ff65f4796b88/infrastructure/cluster/flux/jhub/jhub-release.yaml#L71). | ||
|
|
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 part is different wrt CONFIGURATION.md. I've updated it here to make it less heavy.
We could probably keep that, see this comment
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 understand that the picture I've put before is SWAN specific, but I think it highlighted a few useful features, like metadata searching, and it was compatible with rucio_jupyter_browse.png (same datasets, just not available). Any chance we can reproduce that with these new images?
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.
The problem with this image wasn't that it is swan-specific, the scopes and names are not relevant. The main (and only ) point for changing the image was that in the pop-up dialog you had "rucio_swan".
If you can generate the same image with "my_data" in the pop-up it would be the best
|
I incorporated all the changes in CONFIGURATION.md file to the source repo and created a PR: rucio/jupyterlab-extension#107 |
I was late to review the original PR including the jupyterlab extension documentation #695. This PR is an extension of the previous one, just to be able to include my review of the changes.