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

add "distribution name" notice on "installing kubeflow" + style improvements #3643

Conversation

thesuperzapper
Copy link
Member

@thesuperzapper thesuperzapper commented Dec 5, 2023

The changes in this PR:

  • The main purpose of this PR is to add a warning that says Kubeflow on <PLATFORM> is just name, and not the only way to install Kubeflow on that platform.
  • We also update the "header colors" of the tables, to look nicer.
  • We also update the text above each table to be more correct.
  • We also updated the text for the "raw manifests", to better explain that using them requires "manual patches of YAML".

After we merge this, we need to move the Kubeflow on AWS distribution to the legacy distribution table, there is an existing PR to do this:

Screenshot

Installing-Kubeflow-Kubeflow

@google-oss-prow google-oss-prow bot requested review from alfsuse and knkski December 5, 2023 21:50
@thesuperzapper thesuperzapper force-pushed the add-notice-on-distros-page branch from b99f5d9 to 218eff7 Compare December 5, 2023 22:12
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: thesuperzapper
Once this PR has been reviewed and has the lgtm label, please assign animeshsingh for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@google-oss-prow google-oss-prow bot added size/M and removed size/S labels Dec 5, 2023
@thesuperzapper
Copy link
Member Author

@zijianjoy can review this and approve if you think it's an improvement.

I think it's an important change, because right now, users are confused about the Kubeflow on XXXX distributions, and this PR adds a notice that tells users that these are not "official" from Kubeflow's perspective, they are just their names.

@thesuperzapper thesuperzapper changed the title add distribution name notice on "installing kubeflow" add distribution name notice on "installing kubeflow" + style improvements Dec 5, 2023
@thesuperzapper thesuperzapper changed the title add distribution name notice on "installing kubeflow" + style improvements add "distribution name" notice on "installing kubeflow" + style improvements Dec 7, 2023
@jbottum
Copy link
Contributor

jbottum commented Jan 30, 2024

I am supportive of the additional language on distribution names. On the other changes, I am questioning if the community has agreed on the 6 month rule to define legacy distributions. This seems like a policy that needs to be agreed upon before making web site updates.

@thesuperzapper
Copy link
Member Author

@jbottum the main feedback from the community call was about the wording of the "distribution name notice", based on that, I have updated it to say "not the only", rather than "not the official":

Screenshot 2024-02-06 at 10 13 26

Also, regarding the 6-month rule, let's discuss that in a separate change, because that wording is currently on the website, and the other changes from this PR are very helpful to new users.

If we are happy with the change I would love to get this merged, so people know that "Kubeflow on AWS" is not the only way to use Kubeflow on AWS (especially now that "Kubeflow on AWS" is no longer maintained).

@jbottum
Copy link
Contributor

jbottum commented Feb 20, 2024

I am ok with these changes

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

/assign @kubeflow/kubeflow-steering-committee @kubeflow/wg-manifests-leads @kubeflow/wg-training-leads @kubeflow/wg-notebooks-leads for review

<b>advanced users may choose to install the manifests directly</b> by following <a href="https://github.com/kubeflow/manifests#installation">these instructions</a>.
The raw <a href="https://github.com/kubeflow/manifests"><code>kubeflow/manifests</code></a> are aggregated by the <a href="https://github.com/kubeflow/community/tree/master/wg-manifests">Manifests Working Group</a>
and are intended to be used as the <strong>base of packaged distributions</strong>.
However, advanced users that are willing to make manual patches may choose to install the manifests directly by following <a href="https://github.com/kubeflow/manifests#installation">these instructions</a>.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by manual patches here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean in terms of changing the YAML manifests themselves (customize overlays, ConfigMaps, etc), for anything more than a fully default install.

This is the key point people need to know, that if they use the manifests, they need to be comfortable with making Kubernetes patches.

Copy link
Member

Choose a reason for hiding this comment

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

Basically, they need to modify Kustomize overlays manually if they want to have any changes to the default Kubeflow installation. Maybe we should say this rather than manual patches. I am not sure if users will easily understand this message.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure it's super helpful to list out the possible kinds of manual patches, as there are so many.

Do you have a specific wording you think would help people, without confusing them?

Copy link
Member

@andreyvelich andreyvelich Feb 20, 2024

Choose a reason for hiding this comment

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

Maybe we could say:

However, advanced users may choose to install the manifests directly by following these instructions. 

Users can modify the Kubeflow components kustomize manifests to apply required changes before deploying Kubeflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have pushed some updates, which remove the "manual patches" stuff.

It also links users to the actual release branches of the manifests, rather than master, which may or may not actually work.

Screenshot 2024-02-20 at 12 09 44

@thesuperzapper
Copy link
Member Author

@andreyvelich based on your comments in the meeting today, I pushed a small update to the PR with the following changes:

  1. Added "What is Kubeflow?" to before the Kubeflow introduction at the top
    • Linked the "Introduction" and "Architecture" pages more clearly at the top.
  2. Updated the information in the "Raw Kubeflow Manifests":
    • Now links directly to the specific v1.8.0 release tags in kubeflow/manifests rather than linking people to master (which may not be installable at any specific time).

Comment on lines 20 to 21
1. [__Packaged Distributions__](#packaged-distributions-of-kubeflow) <sup>(recommended)</sup>
2. [__Raw Manifests__](#raw-kubeflow-manifests) <sup>(advanced users)</sup>
Copy link
Member

@andreyvelich andreyvelich Feb 20, 2024

Choose a reason for hiding this comment

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

TBH, I would avoid to say that Package distribution is recommended way of installing Kubeflow. I think, users should chose what they want without explicit notice.
Also, in the following PRs I would like to have 3rd option: Kubeflow Component Standalone, which will explain that Kubeflow is composable and will link to independent components installation guides.
That will help users to understand that they can use individual Kubeflow component separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree we should not technically "recommend" anything, I have updated the PR.


Regarding adding a third "option" of installing standalone components, I am not sure that it's quite the right way to present it. We really want to present Kubeflow as a unified platform, rather than a collection of tools.

I am not sure if it's a good idea to link people to guides that show how to install specific components, not as part of the larger platform.

Copy link
Member

Choose a reason for hiding this comment

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

We really want to present Kubeflow as a unified platform, rather than a collection of tools.

I think, this is debatable. One of the big advantages of Kubeflow is that it could be easily integrated into existing ML platforms without any changes.
For example, if users just need engine for Training, they can use Training Operator or engine for Serving they can use KServe.
I know many large enterprises who are using individual Kubeflow components since they already have some ML capabilities in their platforms.

If we explain Kubeflow microservice architecture and composability properly we will get more attraction from the ML community and other companies.

cc @bigsur0

Copy link
Member Author

Choose a reason for hiding this comment

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

@andreyvelich either way, we can discuss that in a future PR. I think this PR is probably ready to merge now, unless you have any other concerns?

It's critical we get this merged, to help users understand that "Kubeflow on XXXX" does not mean that it's the only way to install Kubeflow on that platform.

1. [Use a packaged distribution](#packaged-distributions-of-kubeflow)
2. [Use the raw manifests](#raw-kubeflow-manifests) <sup>(for advanced users)</sup>
1. [__Packaged Distributions__](#packaged-distributions-of-kubeflow)
2. [__Raw Manifests__](#raw-kubeflow-manifests) <sup>(advanced users)</sup>
Copy link
Member

Choose a reason for hiding this comment

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

Do you need to remove for advance users as well ?
Since we removed warning from raw manifests installation section.

Copy link
Member Author

Choose a reason for hiding this comment

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

We still kind of have a warning, it's just less forceful.

But either way, installing from the manifests is really for advanced users only, we don't want people new to Kubeflow/Kubernetes thinking that its a good place to start.

Copy link
Member

Choose a reason for hiding this comment

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

From my point of view, we should even remove these warnings.
I think, users already understood this by reading the following:
"""
Packaged distributions are maintained by various organizations and typically aim to provide a simplified installation and management experience for Kubeflow.
"""

Would do others think @kubeflow/wg-manifests-leads @kubeflow/kubeflow-steering-committee ?

Copy link
Member Author

@thesuperzapper thesuperzapper Feb 23, 2024

Choose a reason for hiding this comment

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

I strongly think we need to keep a warning of some kind (for the raw manifests).

The new warning from this PR is more specific that users need to understand Kubernetes, Istio and Kubeflow itself, but is overall less forceful (because if someone really is experienced, they shouldn't be afraid of it, but they should know what they're getting themselves into).

Kubeflow and it's dependencies are really one of the most complicated things people regularly deploy on Kubernetes.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Andrew here. Let's rather work with the smallest denominator and refine later on if needed. Then we can merge this PR without the warning and bring any controversial ideas to the KSC agenda.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just so we are clear, the existing website says (for advanced users), and this PR just removes the word "for" so it becomes (advanced users), so its not really a significant change.

I can re-add the "for" if you like, lol.

If we want to change how we present the "raw manifests" to be a non-advanced option, we should do that in a separate PR, after a lot of community discussion.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense @thesuperzapper. Let's discuss these changes in separate PR, if @juliusvonkohout are ok with it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes of course, if is really just removing "for"

@andreyvelich
Copy link
Member

I think, we can merge this PR given our discussions in #3643 (comment) and #3643 (comment)
/lgtm
/assign @kubeflow/kubeflow-steering-committee @juliusvonkohout @james-jwu @zijianjoy

@thesuperzapper
Copy link
Member Author

thesuperzapper commented Feb 27, 2024

@zijianjoy @james-jwu because this updates the CSS stylesheet, we need a root approver.

This PR should be ready to merge now, if you approve.

NOTE: the DOC check is not actually marked as "required", so it can be merged if approved.

Copy link
Member

@andreyvelich andreyvelich left a comment

Choose a reason for hiding this comment

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

/assign @james-jwu @jbottum @johnugeorge @terrytangyuan @zijianjoy @juliusvonkohout if you are ok with these changes let's merge this PR.


The Kubeflow community is not able to provide support for environment-specific issues when using the raw manifests.
When using the raw manifests, the Kubeflow community is not able to provide support for environment-specific issues or custom configurations.
Copy link
Member

@juliusvonkohout juliusvonkohout Mar 1, 2024

Choose a reason for hiding this comment

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

What about

"When using the raw manifests, the Kubeflow maintainers will not provide unpaid support for environment-specific issues or custom configurations. Nevertheless we welcome contributions and bug reports very much. If you need individual support, please consider using a packaged distribution or paid consulting."

instead of

"When using the raw manifests, the Kubeflow community is not able to provide support for environment-specific issues or custom configurations. If you need support, please consider using a packaged distribution."

Disclaimer: I do Kubeflow Consulting, probably @thesuperzapper as well. But i think it is fair to mention that in general next to packaged commercial distributions.

Copy link
Member

@andreyvelich andreyvelich Mar 1, 2024

Choose a reason for hiding this comment

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

@juliusvonkohout Can you submit followup PR to make this change ? I think, this PR is opened for so long and we should move forward.

@andreyvelich
Copy link
Member

andreyvelich commented Mar 1, 2024

/hold due to #3693

@thesuperzapper
Copy link
Member Author

Closing because we merged #3693

@thesuperzapper thesuperzapper deleted the add-notice-on-distros-page branch September 4, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants