-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
clean up default spawner_ui_config.yaml
#6736
clean up default spawner_ui_config.yaml
#6736
Conversation
I think this is a big quality-of-life improvement so would appreciate a review! /assign @kimwnasptd |
@kimwnasptd can you take a quick look at this, its a formatting change but makes the config much easier to use. |
bbb232f
to
4f46575
Compare
@kimwnasptd can we try and get this merged soon, as I think its a significant improvement in readability for the |
@orfeas-k @tasos-ale @elenzio9, as you are quite active now in the web apps, you also might want to review. |
@thesuperzapper I'm not really fond of putting all this documentation in the ConfigMap itself. IMO this makes it more difficult to read both the docs bits (i.e. no table of contents) and the YAML itself. I'd propose to have a separation of concerns. A different place to document the API and a different place for configuring it (yaml). We've also seen multiple requests for more documentation and this can be a good start #6914 So I'd propose to instead:
|
@kimwnasptd I agree that we can provide improved API docs, especially on the notebook section of the website (and am happy to help with this) but I think the changes proposed in this PR are still worth having. The This PR simply makes the existing docstrings/examples more clear and re-orders the file. If you think this PR adds too much detail, can you please comment on the lines which you think are not needed? |
@kimwnasptd I really think we should try and get this cleanup merged as it will make the Do you think you will have time this week to make a GitHub review so we can get this merged? You mentioned that you thought some of the docstrings were a bit long, can you point them out. |
@thesuperzapper I tend to agree with this
I suggest adding a new page under the API Reference. In general, it's dangerous to over-document source files, as it results in not exposing that information in the official docs. Anyway, since you've done a lot of great work here, a possible path forward could be:
|
@StefanoFioravanzo I am happy with that plan, if you are too, can you give it an Note, this won't make it to Kubeflow 1.7 either way, so it won't affect anyone until Kubeflow 1.8. Once we have merged, I will look at updating the |
@thesuperzapper I don't know the details of how the spawner config works. I'd love for @kimwnasptd to approve the technical review as well /lgtm |
@StefanoFioravanzo do you mind giving this an |
Sure @thesuperzapper , sorry for the delay, I caught up with my GitHub emails just now. Sure let's go ahead and merge this. Can we also start a discussion to bring this to docs? If you ping me in a PR or issue I am happy to help you out or review /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: StefanoFioravanzo, Talador12 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:
Approvers can indicate their approval by writing |
The change introduced by kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes kubeflow#7273
The change introduced by kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes kubeflow#7273
The change introduced by #6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by #6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by kubeflow/kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by kubeflow/kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by kubeflow/kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by kubeflow/kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by kubeflow/kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by kubeflow/kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
The change introduced by kubeflow/kubeflow#6736 removed the default GPU vendors list, which causes an issue when trying to select a vendor from the dropdown menu if the vendors list is not configured. This commit can be reverted if proper documentation is provided for users/distributions to configure the dropdown menu. Fixes #7273
This PR significantly cleans up (but does not change) the default
spawner_ui_config.yaml
to make it much easier to understand and customize.The only change to default values is that I have removed the default
gpus.value.vendors
list, and left the previous values as a commented example, as most clusters don't have GPUs, so there is no reason to include them by default.I have also updated the examples for
affinityConfig
andtolerationGroup
to demonstrate how a user can cause Notebook to be scheduled on exclusive Nodes (no other Notebooks), using the newnamespaceSelector
for pod-anti-affinity added in K8S 1.22.