(JWA): Add server type selector for jupterlab, vs-code and r-studio

[davidspek]

Closes: #3362
This PR adds a selector to the notebook creation page allowing the user to select what type of server they want to start. The options are: Jupyter, VS-Code and R-Studio. When VS-Code or R-Studio are selected, the settings required to run the selected server type are added to the Notebook using the updates to the notebook-controller from #5660. These settings are also applied to custom images (if this is unwanted, leave jupyterlab selected). To go along with this, the spawner_ui_config.yaml has been modified to have 3 different types of images (jupyterImage, vscodeImage and rstudioImage).

Server type selector:

When VS-Code is selected, the drop down list shows entries from the vsCodeImage section of the spawner_ui_config.yaml:

The same goes for R-Studio:

The index page has a new column that show what type of server a give notebook is:

/cc @kimwnasptd @StefanoFioravanzo @thesuperzapper @elikatsis @yanniszark

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: DavidSpek
To complete the pull request process, please assign elikatsis after the PR has been reviewed.
You can assign the PR to them by writing /assign @elikatsis in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• components/crud-web-apps/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[castrojo]

We need to ask permission for using the logos from the relevant parties.

Adding these links for reference:

Rstudio: https://rstudio.com/about/trademark/
Jupyter: https://jupyter.org/governance/trademarks.html

I'll chase down VSCode's usage.

[davidspek]

Thanks for the links @castrojo. Regarding Jupyter, I think it is pretty clear from the text already:

All trademarks are subject to “nominative use rules” that allow use of the trademark to name the trademarked entity in a way that is minimal and does not imply a sponsorship relationship with the trademark holder.
As such, stating accurately that software or a service integrates with Jupyter software, that it uses Jupyter software, that it is compatible with Jupyter software, or that it contains Jupyter software, is always allowed. In those cases, you may use the word “Jupyter”, unaltered Jupyter logos, or images or videos of Jupyter applications to indicate this, without our prior approval. This is true both for non-commercial and commercial uses.

[davidspek]

@castrojo I've just sent an email to R-Studio regarding the use of the logo.

[kimwnasptd]

I'll also start reviewing this PR so that we can have the necessary infra ready for the new R-Studio and VSCode images. This can be done in parallel with our discussion around the image names #5575. I'll also try to update our design doc with the designs we will make here.

@davidspek is the PR ready for review?

cc @kubeflow/wg-notebooks-leads

[davidspek]

@kimwnasptd Yes it's ready for review.

[kimwnasptd]

Starting looking into this. I'm good with how the form looks and how the config map will be updated. Although we should redesign its fields at some point in the future but for now it's more than fine.

The first thing we should change though is how the SVGs are sent to the frontend. Let's have the frontend directly fetch them from the backend and not inline them in the config. I'm looking into how we can do this and what parts need to be changed, so I'll provide some more feedback and snippets/examples in a little bit.

Thanks for the effort @davidspek!

[davidspek]

@kimwnasptd Two quick things.

1. The request sent with the notebook CRD needs a small fix to send a string with JSON for the RStudio headers. I will do this in a moment.
2. Regarding the SVG, the reason for having them in the yaml is so that people can very easily swap them out without needing to rebuild the JWA image (this might be needed due to licensing). I wasn't able to get the frontend to grab the SVG from the yaml directly.

Regarding the spawner_ui.yaml in general, me and Mathew were already thinking about what would be a better modular structure and started on a proposal for this. An important idea here is that an arbitrary number of image groups can be in the yaml, and the frontend picks up on this.

[kimwnasptd]

Regarding the SVG, the reason for having them in the yaml is so that people can very easily swap them out without needing to rebuild the JWA image (this might be needed due to licensing). I wasn't able to get the frontend to grab the SVG from the yaml directly.

I know, but we shouldn't include static files into the configuration yaml, as it seems a little bit dirty. The browser will also not be able to cache these svgs in case they are embedded in the config.

Providing a URL for them instead, in the configuration yaml, will make it easy for people to select any SVG they want and the browser will also be able to cache them.

[kimwnasptd]

The request sent with the notebook CRD needs a small fix to send a string with JSON for the RStudio headers. I will do this in a moment.

Makes sense, give me a heads up when you're ready

[davidspek]

@kimwnasptd I just fixed the JSON part so that is ready. Regarding the SVG in the yaml, it isn't very pretty, but the way it works is that the backend dumps the SVG's to a file in the assets where other SVGs are located if they don't exist on container startup. So I believe browser caching will work as expected. Using a URL could break existing deployments if the SVG isn't hosted at that location anymore. Grabbing the SVG from the yaml directly would break browser caching I think so that should probably be avoided then.

[kimwnasptd]

Some first comments after diving a little bit deeper into the code and the commits.

Before jumping into different parts of the review I'd like to invest some time and discuss a little bit more about the structure of the commits. These are some tips to make a PR easier for the reviewer to review.

The first thing to remember is that a lot of reviewers will be reviewing the code by looking at the commits one by one and not directly from the GitHub UI, where it shows the entire list of changes. With this in mind some tips are:

1. Try to organize and group your commits based on the functionality they provide.
   • This will make it easier for the reviewer to review chunks of new functionality and not have to keep a state in mind while looking at other commits to have the bigger picture
   • There's no definitive formula for that tip and I believe every developer has room of improvement in this area. Here's an example of this improvement that @elikatsis mentioned for my code Fixes in Tensorboard web app #5693 (comment)
2. Don't include commits that change the code that another commit introduced in the same PR.
   • This goes hand to hand with the previous tip. Having all the the relevant changes of a functionality grouped into a single commit drastically improves the review process. The reviewer can completely isolate specific commits of implementation.
   • Fixup or squash commits that should be squashed. Don't have commits titles that start with Fixup ...
3. Once the review process starts, and the reviewer submits the first review changes, then from there on submit specific commits that address the reviewer's comments.
   • This will make it easy for the reviewer to be able to quickly check your latest commits and understand if the changes you did align with the proposed changes they had in mind
   • If you rebase the commits then try to avoid squashing the review commits. Here's an example PR in which I marked the review commits with a review: prefix to make it easier for you and @elikatsis to be able to glance at the commits and see their changes Fixes in Tensorboard web app #5693
4. Don't name commits with titles like small fixes as the person looking at it cannot deduce what the changes are about. Ideally these fixes should be integrated into other commits, or be separate commits for each smaller fix

These are just some friendly tips from a quick look on the PR that I believe could drastically improve the PRs you submit and make them really easy to review. And again let me stress out that I'm not trying to neither undermine your work nor try and talk from a high horse. Every developer always has room to improve on how they use git and collaborate with each other, myself included!

These are only meant to be tips from on fellow developer to another :)

[davidspek]

The failed test was the access-management build: https://argo.kubeflow-testing.com/workflows/kubeflow-test-infra/kubeflow-kubeflow-presubmit-ac-mgr-tests-5646-2605340-5088-94f9?tab=workflow&nodeId=kubeflow-kubeflow-presubmit-ac-mgr-tests-5646-2605340-5088-94f9-681790487.
See my comment here: #5693 (comment)

[davidspek]

@kimwnasptd How important is it to have the image groups (jupyter, vscode, rstudio) togetehr under image? I got started on trying to implement this and I am noticing that this change would require a change in most of the logic in the frontend (maybe also backend), which I think will start getting close to what Mathew and I are creating a proposal for. If the following structure can be used, I can get this part of the review sorted very quickly:

image: #for backwards compatibility
  value:
  options:
    -
imageVSCode:
  value:
  options:
    -
imageRStudio:
  value:
  options:
    -
allowCustomImage: true

[kimwnasptd]

@kimwnasptd How important is it to have the image groups (jupyter, vscode, rstudio) togetehr under image?

I'm OK with not putting them under image and using your mentioned format. The move to replace readOnly with allowCustomImage is a good next step forward that unties our hands from needing to group them together.

My first thought would be to rename image to imageJupyter. But considering that the UI doesn't handle gracefully the case where the config is not as expected I would also vote for keeping the image name for backwards compatibility. Let's just add a comment below it that these images are for the jupyter servers.

[davidspek]

@kimwnasptd Great timing! I just pushed the commit that implements this. There is already a comment that specifies the image: is for Jupyter images.

[kimwnasptd]

@davidspek looking at the commits I see you've addressed all of my comments, thanks!

I'll test the PR tomorrow in my cluster and if everything works as expected I'll merge the PR

[davidspek]

@kimwnasptd Awesome! For what it's worth I've just deployed this latest iteration along with the notebook-controller that's on ECR and it is working as expected (got my cluster somewhat fixed again). Don't forget to update the spawner YAML or the JWA backend won't be happy.

[kimwnasptd]

I tested the changes locally and in my cluster and the app works as we described.

Thank you for your efforts on this @davidspek, it is a very nice addition!

I'll merge this PR and move forward with updating the manifests. I was preparing a PR that adds some missing ENV Vars plus the use of the AWS image, so I'll include these changes there as well. I'll also modify the spawner.config in the manifests to match the one from this PR.

I'll just use the Notebook images in the configmap from this PR, but we can update to use the latest ones in a subsequent PR

/lgtm
/approve

[google-oss-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DavidSpek, kimwnasptd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• components/crud-web-apps/OWNERS [kimwnasptd]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment