feat(sdk): client to support KFP v2 API. #7411

chensun · 2022-03-14T16:27:22Z

Description of your changes:

Copy over _auth.py and _client.py from deprecated folder to kfp/client.
Styling and formatting fix.
Use pipeline_manifest and runtime_config in ApiPipelineSpec.

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

connor-mccarthy

Thanks, Chen!

Just a few very small questions and potential changes -- curious what you think.

connor-mccarthy · 2022-03-14T16:42:20Z

sdk/python/kfp/__init__.py


 TYPE_CHECK = True
+
+from kfp.client import Client


What do you think about adding a pylint ignore comment so that is passes a linting check?

Suggested change

from kfp.client import Client

from kfp.client import Client # pylint: disable=wrong-import-position

Sure, thanks for the suggestion!

connor-mccarthy · 2022-03-14T16:43:08Z

sdk/python/kfp/__init__.py

@@ -16,9 +16,13 @@
 # https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
 __path__ = __import__("pkgutil").extend_path(__path__, __name__)

-__version__ = '2.0.0b0'
+__version__ = '2.0.0-alpha.0'


Not this line of code

Should Copyright on this file be 2022 because we are presently modifying it?

connor-mccarthy · 2022-03-14T16:46:11Z

sdk/python/kfp/client/auth.py

+                credentials[client_id]['other_client_id'],
+                credentials[client_id]['other_client_secret'],
+                credentials[client_id]['refresh_token'], client_id)
+    if other_client_id is None or other_client_secret is None:


nit / not your code:

This could be refactored to make it a little bit clearer what is returned if the predicate if other_client_id is None or other_client_secret is None is met:

if other_client_id is None or other_client_secret is None: # fetch IAP auth token: service accounts return get_auth_token_from_sa(client_id) # fetch IAP auth token: user account # Obtain the ID token for provided Client ID with user accounts. # Flow: get authorization code -> exchange for refresh token -> obtain # and return ID token refresh_token = get_refresh_token_from_client_id( other_client_id, other_client_secret) credentials = {} if os.path.exists(LOCAL_KFP_CREDENTIAL): with open(LOCAL_KFP_CREDENTIAL, 'r') as f: credentials = json.load(f) credentials[client_id] = {} credentials[client_id]['other_client_id'] = other_client_id credentials[client_id]['other_client_secret'] = other_client_secret credentials[client_id]['refresh_token'] = refresh_token # TODO: handle the case when the refresh_token expires, which only # happens if the refresh_token is not used once for six months. if not os.path.exists(os.path.dirname(LOCAL_KFP_CREDENTIAL)): os.makedirs(os.path.dirname(LOCAL_KFP_CREDENTIAL)) with open(LOCAL_KFP_CREDENTIAL, 'w') as f: json.dump(credentials, f) return id_token_from_refresh_token(other_client_id, other_client_secret, refresh_token, client_id)

This file and client.py as well may have many refactoring opportunities, let alone the pylint errors.
Will leave those for later PRs-- feel empowered to propose refactoring PRs as well :)

connor-mccarthy · 2022-03-14T16:51:59Z

sdk/python/kfp/client/client.py

+
+        token = None
+
+        # "existing_token" is designed to accept token generated outside of SDK.


What do you think about removing this commented out code? I don't know much about its origin, but we're not using it here nor in deprecated.

Not knowing much about the origin either, but existing_token seems to me it's still used as an optional parameter.
Maybe this chunk of comments should really be moved into the docstring instead.

connor-mccarthy · 2022-03-14T16:57:23Z

sdk/python/setup.py

@@ -138,6 +139,6 @@ def find_version(*file_path_parts):
        'console_scripts': [
            'dsl-compile = kfp.deprecated.compiler.main:main',
            'dsl-compile-v2 = kfp.compiler.main:main',
-            'kfp=kfp.__main__:main'


Not this line of code

Copyright 2022 here?

chensun · 2022-03-15T00:08:17Z

/test kubeflow-pipelines-samples-v2

chensun · 2022-03-15T02:20:10Z

/retest

…7400) * chore(sdk): clean, dedup, reconcile, organize requirements * apply yapf formatting * correct copyright

* chore(sdk): fix test missing tempdir cleanup * fix test path * fix test path * clean up setUp method * change string placeholder from s to test_dir

…2-client

chensun · 2022-03-15T17:22:35Z

/retest

google-oss-prow · 2022-03-15T18:03:39Z

@chensun: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
kubeflow-pipelines-v2-go-test	`ea5483c`	link	true	`/test kubeflow-pipelines-v2-go-test`
kubeflow-pipeline-upgrade-test	`ea5483c`	link	true	`/test kubeflow-pipeline-upgrade-test`
kubeflow-pipeline-e2e-test	`ea5483c`	link	true	`/test kubeflow-pipeline-e2e-test`
kubeflow-pipeline-backend-test	`ea5483c`	link	true	`/test kubeflow-pipeline-backend-test`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

connor-mccarthy

Thanks, Chen!

/approve
/lgtm

google-oss-prow · 2022-03-15T18:07:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: connor-mccarthy

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:

~~sdk/OWNERS~~ [connor-mccarthy]

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

chensun · 2022-03-15T21:05:11Z

/retest

* copy _client.py and _auth.py from deprecated folder * Make kfp.Client support v2 API (IR). * fix test golden * address comments * copyright year * chore(backend): clean up pipelinespec.Value usage (kubeflow#7407) * chore(sdk): clean, dedup, reconcile, organize requirements (kubeflow#7400) * chore(sdk): clean, dedup, reconcile, organize requirements * apply yapf formatting * correct copyright * chore(sdk): fix test missing tempdir cleanup (kubeflow#7403) * chore(sdk): fix test missing tempdir cleanup * fix test path * fix test path * clean up setUp method * change string placeholder from s to test_dir * release note Co-authored-by: Connor McCarthy <mccarthy.connor.james@gmail.com>

chensun added 2 commits March 14, 2022 08:28

copy _client.py and _auth.py from deprecated folder

5179b45

Make kfp.Client support v2 API (IR).

dcff6d1

chensun requested review from ji-yaqi and connor-mccarthy March 14, 2022 16:27

chensun assigned connor-mccarthy Mar 14, 2022

google-oss-prow bot added the size/XXL label Mar 14, 2022

connor-mccarthy requested changes Mar 14, 2022

View reviewed changes

connor-mccarthy reviewed Mar 14, 2022

View reviewed changes

chensun added 3 commits March 14, 2022 23:01

fix test golden

5c16687

address comments

3942d64

copyright year

01859e6

chensun and others added 4 commits March 15, 2022 16:37

chore(backend): clean up pipelinespec.Value usage (kubeflow#7407)

553ec7b

chore(sdk): clean, dedup, reconcile, organize requirements (kubeflow#…

cf310ab

…7400) * chore(sdk): clean, dedup, reconcile, organize requirements * apply yapf formatting * correct copyright

chore(sdk): fix test missing tempdir cleanup (kubeflow#7403)

833f565

* chore(sdk): fix test missing tempdir cleanup * fix test path * fix test path * clean up setUp method * change string placeholder from s to test_dir

release note

ea5483c

chensun force-pushed the v2-client branch from b75e34b to ea5483c Compare March 15, 2022 16:38

Merge branch 'master' of https://github.com/kubeflow/pipelines into v…

b17b6f6

…2-client

connor-mccarthy approved these changes Mar 15, 2022

View reviewed changes

google-oss-prow bot added the lgtm label Mar 15, 2022

google-oss-prow bot added the approved label Mar 15, 2022

google-oss-prow bot merged commit b741926 into kubeflow:master Mar 15, 2022

chensun deleted the v2-client branch March 16, 2022 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk): client to support KFP v2 API. #7411

feat(sdk): client to support KFP v2 API. #7411

chensun commented Mar 14, 2022

connor-mccarthy left a comment

connor-mccarthy Mar 14, 2022

chensun Mar 14, 2022

connor-mccarthy Mar 14, 2022

chensun Mar 14, 2022

connor-mccarthy Mar 14, 2022

chensun Mar 14, 2022

connor-mccarthy Mar 14, 2022

chensun Mar 14, 2022

connor-mccarthy Mar 14, 2022

chensun Mar 14, 2022

chensun commented Mar 15, 2022

chensun commented Mar 15, 2022

chensun commented Mar 15, 2022

google-oss-prow bot commented Mar 15, 2022 •

edited

Loading

connor-mccarthy left a comment

google-oss-prow bot commented Mar 15, 2022

chensun commented Mar 15, 2022

	from kfp.client import Client
	from kfp.client import Client # pylint: disable=wrong-import-position


		token = None

		# "existing_token" is designed to accept token generated outside of SDK.

feat(sdk): client to support KFP v2 API. #7411

feat(sdk): client to support KFP v2 API. #7411

Conversation

chensun commented Mar 14, 2022

connor-mccarthy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun commented Mar 15, 2022

chensun commented Mar 15, 2022

chensun commented Mar 15, 2022

google-oss-prow bot commented Mar 15, 2022 • edited Loading

connor-mccarthy left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented Mar 15, 2022

chensun commented Mar 15, 2022

google-oss-prow bot commented Mar 15, 2022 •

edited

Loading