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

Added outdated warning to all pipelines v1 pages #3775

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

hbelmiro
Copy link
Contributor

@hbelmiro hbelmiro commented Jun 24, 2024

Resolves: #3751

Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@hbelmiro
Copy link
Contributor Author

cc @chensun @thesuperzapper

@thesuperzapper
Copy link
Member

@hbelmiro looks good, simple fix.

/lgtm

@thesuperzapper
Copy link
Member

@hbelmiro Although we may want to have a new "out of date" warning for all the pages in the "Legacy V1" section:

https://www.kubeflow.org/docs/components/pipelines/legacy-v1/

Which explains that users are looking at the docs for Kubeflow Pipelines 1.0, which is no longer supported (but which Kubeflow Pipelines 2.0 supports as backwards compatible).

@hbelmiro
Copy link
Contributor Author

@thesuperzapper something like the following?

{{% alert title="Out of date" color="warning" %}}
This guide contains outdated information pertaining to Kubeflow v1. You may want to access the documentation for the latest Kubeflow Pipelines version that supports v1 as backward compatible.
{{% /alert %}}

cc @rimolive

@thesuperzapper
Copy link
Member

@hbelmiro I think something more direct about telling users to see the new docs (and linking them).

{{% alert title="Old Version" color="warning" %}}
This page is about __Kubeflow Pipelines V1__, for information about __Kubeflow Pipelines V2__, please see the [new docs](__root_link_to_v2_docs__).

Please note that Kubeflow Pipelines V2 supports running V1 pipelines in a [backwards compatible mode](__page_which_explains_this__).
{{% /alert %}}

On the page which explains the backwards compatibility we should outline:

  1. You need to use the KFP V1 SDK to submit/compile the pipelines to V2 (you can't use the V2 SDK for this)
  2. That the new object store integration is different.
  3. A brief overview of the differences between V1 / V2 (we already have a migration page, so perhaps just link this).
  4. A dicussion of how long we plan to keep backwards compatibility for V1 in KFP.

@hbelmiro hbelmiro changed the title Removed outdated warning from pipelines pages Added outdated warning to all pipelines v1 pages Jun 27, 2024
@hbelmiro
Copy link
Contributor Author

@thesuperzapper

@hbelmiro I think something more direct about telling users to see the new docs (and linking them).

{{% alert title="Old Version" color="warning" %}}
This page is about __Kubeflow Pipelines V1__, for information about __Kubeflow Pipelines V2__, please see the [new docs](__root_link_to_v2_docs__).

Please note that Kubeflow Pipelines V2 supports running V1 pipelines in a [backwards compatible mode](__page_which_explains_this__).
{{% /alert %}}

Added.

On the page which explains the backwards compatibility we should outline:

1. You need to use the KFP V1 SDK to submit/compile the pipelines to V2 (you can't use the V2 SDK for this)

2. That the new object store integration is different.

3. A brief overview of the differences between V1 / V2 (we already have a migration page, so perhaps just link this).

4. A dicussion of how long we plan to keep backwards compatibility for V1 in KFP.

This is out of the scope of this PR. Can I ask you to open an issue? We can then add it to #3712.

@rimolive
Copy link
Member

/lgtm

@thesuperzapper
Copy link
Member

@hbelmiro we can probably include most of the information people need to know in the warning itself, and just link them to the migration page.

How about we use this warning:

{{% alert title="Old Version" color="warning" %}}
This page is about __Kubeflow Pipelines V1__, please see the [V2 documentation](/docs/components/pipelines) for the latest information.

Note, while the V2 backend is able to run pipelines submitted by the V1 SDK, we strongly recommend [migrating to the V2 SDK](/docs/components/pipelines/user-guides/migration).
For reference, the final release of the V1 SDK was [`kfp==1.8.22`](https://pypi.org/project/kfp/1.8.22/), and its reference documentation is [available here](https://kubeflow-pipelines.readthedocs.io/en/1.8.22/).
{{% /alert %}}

Either way, I definitely think we can improve the V2 migration guide, because it's quite hard to follow right now, and does not really "sell you" on why you should go through the effort.

@rimolive
Copy link
Member

@hbelmiro Open a follow-up issue with @thesuperzapper requests and let's move to other issues. There's much more important sections to cover in the new KFP docs.

@thesuperzapper
Copy link
Member

@rimolive this PR does not block anything, so there is no need to rush it?

Also, I think it's very important for users to understand V1 vs V2 as right now its very confusing to users.

It a very straightforward update I have suggested in #3775 (comment), and if @hbelmiro agrees, they can choose to update this PR so we can merge it.

@rimolive
Copy link
Member

@thesuperzapper It does block the only contributor working on the remaining items for KFP docs, as listed in #3712.

Your suggestion can be worked later when we have the other pending topics done.

@rimolive
Copy link
Member

rimolive commented Jul 4, 2024

/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rimolive

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-prow google-oss-prow bot merged commit a780c76 into kubeflow:master Jul 4, 2024
6 checks passed
@hbelmiro hbelmiro deleted the out-of-date-warning branch July 4, 2024 14:35
@thesuperzapper
Copy link
Member

@hbelmiro I am disappointed this PR was merged without responding to my comments in my review #3775 (comment).

The update I proposed was very important, so I have raised a new PR update the text:

@rimolive
Copy link
Member

rimolive commented Jul 6, 2024

Thanks for opening the issue. We'll be working to fix until 1.9 release.

@hbelmiro
Copy link
Contributor Author

hbelmiro commented Jul 6, 2024

@hbelmiro I am disappointed this PR was merged without responding to my comments in my review #3775 (comment).

@thesuperzapper the issue that this PR addresses was originally to remove the warnings. You asked that here, I created the issue, and sent this PR.
Then in this review, you asked to re-add the warning, and then change it, and then change it again, and again...

@rimolive left clear here that we needed to move on with this PR to fix other pending issues.

@thesuperzapper
Copy link
Member

@hbelmiro this warning is one of the most important parts of the docs, as there is a lot of confusion right now with V1 vs V2.

But in any case, if you agree with the update to the wording proposed in #3792, it should be ready to merge.

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.

Add "Out of date" warnings to all Pipelines v1 docs
3 participants