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.

Sign up for GitHub

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

Jump to bottom

chore(sdk): remove kfp.deprecated from sdk, legacy samples, and legacy tests #11366

Merged
merged 2 commits into from
Nov 12, 2024
Merged

chore(sdk): remove kfp.deprecated from sdk, legacy samples, and legacy tests #11366

merged 2 commits into from
Nov 12, 2024

Conversation

chensun
Copy link
Member

@chensun chensun commented Nov 8, 2024

Description of your changes:

Checklist:

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@chensun chensun changed the title chore(sdk): remove kfp.deprecated code chore(sdk): remove kfp.deprecated code, legacy samples, and legacy tests Nov 8, 2024
@chensun chensun changed the title chore(sdk): remove kfp.deprecated code, legacy samples, and legacy tests chore(sdk): remove kfp.deprecated from sdk, legacy samples, and legacy tests Nov 8, 2024
@github-actions github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 8, 2024
@HumairAK HumairAK added this to the KFP SDK 2.11 milestone Nov 8, 2024
HumairAK
HumairAK reviewed Nov 8, 2024
View reviewed changes
samples/core/loop_parameter/loop_parameter_test.py Outdated
import kfp
from kfp.samples.test.utils import debug_verify
Copy link
Collaborator

Choose a reason for hiding this comment

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

debug_verify doesn't seem like it's ever used

HumairAK
HumairAK reviewed Nov 8, 2024
View reviewed changes
samples/core/retry/retry_test.py Outdated
@@ -13,11 +13,10 @@
# limitations under the License.

import kfp
Copy link
Collaborator

@HumairAK HumairAK Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

feel free to consider this out of scope, but there's a couple of instances in the updated files, where the direct kfp imports don't look like they are used at all, seems like we can just drop them all together (unless it is intentional?)

HumairAK
HumairAK reviewed Nov 8, 2024
View reviewed changes
samples/v2/cache_test.py Outdated
import kfp_server_api
from ml_metadata.proto import Execution

from ..test.two_step import two_step_pipeline
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this has been removed

HumairAK
HumairAK reviewed Nov 8, 2024
View reviewed changes
samples/v2/pipeline_with_secret_as_env_test.py Outdated
Comment on lines 20 to 29
import kfp
from kfp.samples.test.utils import KfpTask
from kfp.samples.test.utils import run_pipeline_func
from kfp.samples.test.utils import TaskInputs
from kfp.samples.test.utils import TaskOutputs
from kfp.samples.test.utils import TestCase
import kfp_server_api
from ml_metadata.proto import Execution

from kfp.samples.test.utils import KfpTask, TaskInputs, TaskOutputs, TestCase, run_pipeline_func
from .pipeline_secret_env import pipeline_secret_env
Copy link
Collaborator

@HumairAK HumairAK Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

kfp/taskinputs/taksoutputs/execution look like they aren't used
and .pipeline_secret_env looks like it may have been removed sometime in the past

same applies here and here

@HumairAK
Copy link
Collaborator

HumairAK commented Nov 8, 2024

thanks for cleaning house @chensun !
just left some nits, otherwise /lgtm

@github-actions github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 11, 2024
@rimolive
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Nov 12, 2024
@chensun
Copy link
Member Author

chensun commented Nov 12, 2024

/approve

@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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 11a8b86 into kubeflow:master Nov 12, 2024
24 checks passed
@chensun chensun deleted the remove-kfp-deprecated branch November 12, 2024 22:38
@gregsheremeta gregsheremeta mentioned this pull request Nov 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HumairAK HumairAK HumairAK left review comments

@animeshsingh animeshsingh Awaiting requested review from animeshsingh

@gregsheremeta gregsheremeta Awaiting requested review from gregsheremeta

@rimolive rimolive Awaiting requested review from rimolive

Assignees

@rimolive rimolive

Labels
approved ci-passed All CI tests on a pull request have passed lgtm size/XXL
Projects
None yet
Milestone
KFP SDK 2.11
Development

Successfully merging this pull request may close these issues.
3 participants
@chensun @HumairAK @rimolive