add support for flexible config (via env var) for the pipline service and UI, fix broken links (pointed to API vs UI service) #1293

yaronha · 2019-05-07T13:59:51Z

provide a client SDK mechanism to override pipline API & UI service endpoint URLs via environment var
also fixes an issue of broken ui link when not using the default host (host is API ep and link is a UI ep)

this addresses issue #1235

This change is

… and UI, fix broken links (pointed to API vs UI service)

googlebot · 2019-05-07T13:59:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

k8s-ci-robot · 2019-05-07T14:00:07Z

Hi @yaronha. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2019-05-07T14:00:07Z

Hi @yaronha. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

yaronha · 2019-05-07T14:03:04Z

I signed it!

googlebot · 2019-05-07T14:03:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

yaronha · 2019-05-07T23:42:42Z

/assign @gaoning777

yaronha · 2019-05-12T21:53:18Z

@gaoning777 @Ark-kun any update on this (tiny) PR ?

…-pipeline-ep-env

Ark-kun · 2019-05-14T01:44:45Z

any update on this (tiny) PR ?

Sorry. We'll try to review it soon. @kevinbache WDYT?

yaronha · 2019-05-26T21:51:31Z

@Ark-kun any updates on it? waiting for ~20 days on a rather simple patch

gaoning777 · 2019-06-04T23:47:17Z

/ok-to-test

gaoning777 · 2019-06-05T01:37:41Z

Would you mind resolving the conflicts? There seems to be a recent PR that is in conflict with this PR.

Ark-kun · 2019-06-05T03:47:21Z

/lgtm

yaronha · 2019-06-05T15:59:30Z

@Ark-kun @gaoning777 test failed, but seem to be related to some CI/infra issue not in my code, can u give it a look?

gaoning777 · 2019-06-05T16:39:28Z

/test kubeflow-pipeline-e2e-test

gaoning777 · 2019-06-05T16:42:16Z

THe KFP also runs a sample test in the background, which shows "run-notebook-tfx-tests: AttributeError: 'Client' object has no attribute '_uihost'"

detailed logs are: https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubeflow_pipelines/1293/kubeflow-pipeline-sample-test/1136281786020532224

hongye-sun

Hi @yaronha, I recently added support to parameterize the namespace in the sdk in #1443. Other than the namespace, is there any other use case that you want to customize the API host by env var?

hongye-sun · 2019-06-05T17:51:07Z

sdk/python/kfp/_client.py

@@ -52,21 +55,23 @@ def __init__(self, host=None, client_id=None, namespace='kubeflow'):
    """

    self._host = host
+    self._uihost = os.environ.get(KF_PIPELINES_UI_ENDPOINT_ENV, '')


Can default value be host? Otherwise, it will break currently local notebook + port forward use case.

@hongye-sun i'm trying to make the UI and API ports configurable from the environment variables, this way we dont have to use proxy server when there is a proper API/UI gateway.

i wasnt clear about the port forward case, are you forwarding the UI port or the API port, currently the it seems like host and config.host identifies the API port and the current code is broken, im one place you have:

if in_cluster: config.host = Client.IN_CLUSTER_DNS_NAME.format(namespace) return config

(i.e. host == API port)

in another:

if config.host: config.host = os.path.join(config.host, Client.KUBE_PROXY_PATH.format(namespace))

@yaronha, it makes sense to have API and UI addresses as separate configurations. We have a wiki to guide user to local port-forward on ambassador (https://www.kubeflow.org/docs/other-guides/accessing-uis/), which is the gateway for both API and UIs. We also have a notebook tutorial (https://github.com/amygdala/examples/blob/cookbook/cookbook/pipelines/notebooks/kfp_remote_deploy-no-IAP.ipynb) to guide user to use the SDK with local port forward and I think the UI link is working at this time. IAP guide (https://github.com/amygdala/examples/blob/cookbook/cookbook/pipelines/notebooks/kfp_remote_deploy.ipynb) should probably work, too. I think we should make the existing tutorial continues working, which assumes API and UI are both from the same host. That's why I suggest to keep the host as uihost if the env var is missing.

@hongye-sun the examples above fall into two categories, using Proxy and using proprietary Google API gateway (IAP), we or others may have our own UI/API gateway with strong identity management (like IAP) so need a way to specify the the path to it.

The API and UI can be different hosts, since the API can be an internal cluster/DNS name, while the UI is accessed from a remote browser and need a publicly exposed URL.

@yaronha, thanks for letting me know your use cases. let's keep both host and uihost. The only thing missing in the PR is that it changes the behavior for uihost if user doesn't provide env var, which might cause existing tutorial break. Could you make sure that in case of env var is missing, the uihost equals to host, just like today?

Thanks, i will update

@hongye-sun did u review my changes?

Sorry for the delay. One more comment.

hongye-sun · 2019-06-05T17:52:17Z

sdk/python/kfp/_client.py

@@ -52,21 +55,23 @@ def __init__(self, host=None, client_id=None, namespace='kubeflow'):
    """

    self._host = host


host or os.environ.get(KF_PIPELINES_ENDPOINT_ENV)?

its a simple/standard way to say host if host else os.env...

I mean if we should set _host to be the value from env var if it's missing? Either it's from parameter or env var, it should be the same thing.

@hongye-sun i consolidated self._host and self._uihost since _host wasnt used, now if no env var and host is provided it will use host (still not clear, how host will open up the browser on the UI if its the remote API endpoint)

hongye-sun · 2019-06-05T17:52:38Z

sdk/python/kfp/_client.py

@@ -52,21 +55,23 @@ def __init__(self, host=None, client_id=None, namespace='kubeflow'):
    """

    self._host = host


probably change the name to _api_host

// I think host is can be misleading here as people think they do not need the https:// schema. endpoint or uri might be more appropriate.

@hongye-sun it seems like self._host is not used anymore so maybe we should only keep one, once we clarify the use of API vs UI (above)

I think we need two one for API and one for UI. API endpoint can be passed as parameter or env var. UI endpoint can only be customized via env var. The default value of UI endpoint if env var is missing is same as API host. This is useful if the endpoint is from ambassador use case.

Oh, sorry. API endpoint, might not need env var support as we support namespace customization.

Ark-kun · 2019-06-05T19:47:41Z

/test kubeflow-pipeline-e2e-test

Ark-kun · 2019-06-06T21:36:51Z

/test kubeflow-pipeline-e2e-test

hongye-sun · 2019-06-10T21:27:55Z

sdk/python/kfp/_client.py

    config = self._load_config(host, client_id, namespace)
    api_client = kfp_server_api.api_client.ApiClient(config)
    self._run_api = kfp_server_api.api.run_service_api.RunServiceApi(api_client)
    self._experiment_api = kfp_server_api.api.experiment_service_api.ExperimentServiceApi(api_client)

  def _load_config(self, host, client_id, namespace):
    config = kfp_server_api.configuration.Configuration()
+    config.host = config.host or os.environ.get(KF_PIPELINES_ENDPOINT_ENV)


host = host or os.environ.get(KF_PIPELINES_ENDPOINT_ENV, None)

Otherwise, the code will fail if no host and env var are provided.

@hongye-sun i don't think get fails, will just return None
just try this: print('' or os.environ.get('SOMEKEY')) on your laptop

you are right on the environ.get. You still need to change the config.host to host or you should change rest of the method body to check against config.host instead of host.

@hongye-sun you can see the line below, (if host: config.host = host), host will take precedence over the env var, i'm not clear why it doesnt address the case ?

Better to go with test cases:

For example, (host=None, os.environ.get(KF_PIPELINES_ENDPOINT_ENV)='https://iap-endpoint/', client_id=''), in ln 70, iap token won't be fetched.

Another example, (host=None, os.environ.get(KF_PIPELINES_ENDPOINT_ENV)='https://iap-endpoint/', client_id=None), in ln 79, it will ignore config.host and override it with either incluster DNS or the default kubu config's endpoint.

@hongye-sun ok, got u, switched to host, make more sense

yaronha · 2019-06-17T17:34:25Z

@IronPan @gaoning777 can u look into this PR?

yaronha · 2019-06-18T20:01:30Z

/retest

gaoning777 · 2019-06-25T22:28:37Z

/ok-to-test

gaoning777 · 2019-06-25T22:28:56Z

/lgtm

gaoning777 · 2019-06-25T22:30:40Z

Could you resolve the conflicts? Sorry for the delay, I was OOO.

yaronha · 2019-07-16T15:53:11Z

@IronPan @gaoning777 i resolved the conflict, please review this before doing more changes on that file so i wont need to do the merge for the 4th time

IronPan · 2019-07-16T17:59:58Z

/lgtm
/approve

k8s-ci-robot · 2019-07-16T18:00:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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~~ [IronPan]

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

yaronha · 2019-07-16T20:24:11Z

/retest

add support for flexible config (via env var) for the pipline service…

11854f6

… and UI, fix broken links (pointed to API vs UI service)

k8s-ci-robot requested review from gaoning777 and hongye-sun May 7, 2019 13:59

k8s-ci-robot added size/S needs-ok-to-test labels May 7, 2019

k8s-ci-robot assigned gaoning777 May 7, 2019

yaronha added 2 commits May 13, 2019 00:53

Merge branch 'master' of https://github.com/yaronha/pipelines into kf…

31f3097

…-pipeline-ep-env

support https prefix

7b80eae

Ark-kun assigned kevinbache May 14, 2019

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jun 4, 2019

k8s-ci-robot assigned Ark-kun Jun 5, 2019

k8s-ci-robot added the lgtm label Jun 5, 2019

merge with master

e8f72ce

k8s-ci-robot removed the lgtm label Jun 5, 2019

change to LF

1e1acb9

init _uihost

d12c95c

hongye-sun reviewed Jun 5, 2019

View reviewed changes

Ark-kun assigned hongye-sun Jun 5, 2019

Ark-kun requested a review from IronPan June 7, 2019 01:01

Ark-kun assigned IronPan Jun 7, 2019

set _uihost from host param if specified no env var

d986e4f

hongye-sun reviewed Jun 10, 2019

View reviewed changes

change config.host to host

869db23

k8s-ci-robot added the lgtm label Jun 25, 2019

merge with upstream master

c2e0eeb

k8s-ci-robot removed the lgtm label Jul 16, 2019

k8s-ci-robot added the lgtm label Jul 16, 2019

k8s-ci-robot added the approved label Jul 16, 2019

k8s-ci-robot merged commit 6e51714 into kubeflow:master Jul 16, 2019

gaoning777 mentioned this pull request Jan 8, 2020

flexible pipeline service (host) path in client SDK #1235

Closed

		@@ -52,21 +55,23 @@ def __init__(self, host=None, client_id=None, namespace='kubeflow'):
		"""

		self._host = host

add support for flexible config (via env var) for the pipline service and UI, fix broken links (pointed to API vs UI service) #1293

add support for flexible config (via env var) for the pipline service and UI, fix broken links (pointed to API vs UI service) #1293

Conversation

yaronha commented May 7, 2019 • edited by jlewi Loading

googlebot commented May 7, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

k8s-ci-robot commented May 7, 2019

k8s-ci-robot commented May 7, 2019

yaronha commented May 7, 2019

googlebot commented May 7, 2019

yaronha commented May 7, 2019

yaronha commented May 12, 2019

Ark-kun commented May 14, 2019

yaronha commented May 26, 2019

gaoning777 commented Jun 4, 2019

gaoning777 commented Jun 5, 2019

Ark-kun commented Jun 5, 2019

yaronha commented Jun 5, 2019

gaoning777 commented Jun 5, 2019

gaoning777 commented Jun 5, 2019

hongye-sun 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

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

Ark-kun commented Jun 5, 2019

Ark-kun commented Jun 6, 2019

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

yaronha commented Jun 17, 2019

yaronha commented Jun 18, 2019

gaoning777 commented Jun 25, 2019 • edited Loading

gaoning777 commented Jun 25, 2019

gaoning777 commented Jun 25, 2019

yaronha commented Jul 16, 2019

IronPan commented Jul 16, 2019

k8s-ci-robot commented Jul 16, 2019

yaronha commented Jul 16, 2019

yaronha commented May 7, 2019 •

edited by jlewi

Loading

gaoning777 commented Jun 25, 2019 •

edited

Loading