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

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

Merged
merged 9 commits into from
Jul 16, 2019
Merged

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

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
11854f6
add support for flexible config (via env var) for the pipline service…
yaronha May 7, 2019
31f3097
Merge branch 'master' of https://github.com/yaronha/pipelines into kf…
yaronha May 12, 2019
7b80eae
support https prefix
yaronha May 12, 2019
e8f72ce
merge with master
yaronha Jun 5, 2019
1e1acb9
change to LF
yaronha Jun 5, 2019
d12c95c
init _uihost
yaronha Jun 5, 2019
d986e4f
set _uihost from host param if specified no env var
yaronha Jun 7, 2019
869db23
change config.host to host
yaronha Jun 14, 2019
c2e0eeb
merge with upstream master
yaronha Jul 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions sdk/python/kfp/_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@

from ._auth import get_auth_token

KF_PIPELINES_ENDPOINT_ENV = 'KF_PIPELINES_ENDPOINT'
KF_PIPELINES_UI_ENDPOINT_ENV = 'KF_PIPELINES_UI_ENDPOINT'

class Client(object):
""" API Client for KubeFlow Pipeline.
"""
Expand All @@ -51,22 +54,23 @@ def __init__(self, host=None, client_id=None, namespace='kubeflow'):
client_id: The client ID used by Identity-Aware Proxy.
"""

self._host = host
self._uihost = os.environ.get(KF_PIPELINES_UI_ENDPOINT_ENV, host)
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if host:
config.host = host

token = None
if host and client_id:
# fetch IAP auth token
token = get_auth_token(client_id)

if token:
config.api_key['authorization'] = token
config.api_key_prefix['authorization'] = 'Bearer'
Expand Down Expand Up @@ -111,12 +115,12 @@ def _is_ipython(self):
return True

def _get_url_prefix(self):
if self._host:
if self._uihost:
# User's own connection.
if self._host.startswith('http://') or self._host.startswith('https://'):
return self._host
if self._uihost.startswith('http://') or self._uihost.startswith('https://'):
return self._uihost
else:
return 'http://' + self._host
return 'http://' + self._uihost

# In-cluster pod. We could use relative URL.
return '/pipeline'
Expand Down