Restructured Kubeflow Pipelines docs #3737
Conversation
|
@hbelmiro Awesome work! I have just a few comments:
WDYT? |
|
/hold for review |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Done.
Sounds good. I created two issues for that:
Should we add them (and new ones that may arise) to #3712 even if they may not be completed on time for 1.9? |
|
/unhold |
There was a problem hiding this comment.
There are still redirect issues, we need to make sure that all the old pages still redirect correctly, namely:
- https://www.kubeflow.org/docs/components/pipelines/v1/introduction/
- https://www.kubeflow.org/docs/components/pipelines/v2/introduction/
Which both currently 404.
hbelmiro
force-pushed
the
restructure-kfp
branch
2 times, most recently
from
bdfc735
to
9a7cf22
Compare
|
@thesuperzapper @rimolive @HumairAK @milosjava |
|
@rimolive @StefanoFioravanzo @thesuperzapper 404s from #3737 (review) fixed. |
|
@thesuperzapper @StefanoFioravanzo Can you please verify if there are other 404s remaining? |
|
These we'll fix in a follow PR: |
|
@hbelmiro it was very hard to review because the ordering was random, so I sorted them alphabetically and went through each one to ensure its source and target were correct and that no pages were missing. Here is the fixed copy, please update the PR to replace the redirects you added to end of the After that I can LGTM. |
Signed-off-by: hbelmiro <[email protected]>
|
@thesuperzapper done. |
|
@hbelmiro thanks for your work on this, I think its ok to merge now. /lgtm Note, we will still get 404 for all the pre-v1/v2 split pages, but since we didn't want to make a new "component" path (e.g. rename the root prefix from |
|
@james-jwu or @zijianjoy can you please approve this significant refactor of the Kubeflow Pipelines docs if you agree? We need a root approver because we are updating the redirects. |
There was a problem hiding this comment.
Thank you for this hard work @hbelmiro!
We need someone from the KFP team to approve it.
/assign @kubeflow/pipelines
| @@ -1,353 +0,0 @@ | |||
| +++ | |||
There was a problem hiding this comment.
I think we should keep the v2/installation/quickstart content instead of redirecting people to v1. It contains practical guidance to actually getting started with the product.
There was a problem hiding this comment.
@zijianjoy we plan to create a proper v2 installation page in a follow-up PR. This PR is intended just to move pages avoiding changing their contents, otherwise, the review would be too difficult.
|
It contains a huge change in website architecture, including @connor-mccarthy who originally introduced the current structure to take a look. |
|
@chensun @connor-mccarthy @zijianjoy |
|
My 2c: with the removal of the quickstart page and the new structure, some very basic usage patterns (e.g., define and compile a component) are deeply nested on the documentation site. I would consider restoring a quickstart guide. A typical and critical reason for reviewing the docs is to determine whether to use Pipelines or some other ML orchestration system. A key input to this choice is often the SDK authoring experience. With the current restructuring, "Run a pipeline" is quite deep in the tree. 
I wont block on this, but want to raise it for consideration before merging. |
|
@connor-mccarthy the But it probably needs to be updated for "Kubeflow Platform" deployments, which need special steps to submit pipelines from outside the cluster, probably by linking to the However, I agree that we should link users from the |
|
@connor-mccarthy @thesuperzapper |
|
I agree on those points, especially providing links in the Getting Started section. As Helber mentioned, this is something we can work on right after merging this PR. @connor-mccarthy It's important we proceed quickly so that we can start working independently on several improvements, including what you suggested, in time for the Kubeflow 1.9 release. If you don't have other concerns, could you approve? We will prfioritize addressing your concerns right after. |
|
/lgtm Thanks everyone for the effort and discussion on this! |
|
@zijianjoy we still need your approval to get this merged. |
|
James Liu is OOO. I will approve on his behalf. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: connor-mccarthy, james-jwu, milosjava 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 |
Resolves #3716.
This PR:
cc @StefanoFioravanzo @diegolovison @milosjava