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

[SDK] Use Training Client without Kube Config #1740

Merged
merged 2 commits into from
Jan 24, 2023
Merged

[SDK] Use Training Client without Kube Config #1740

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c15cfae
[SDK] Use Training Client without Kube Config
andreyvelich Jan 24, 2023
3a4532d
Skip kube-config when using client config
andreyvelich Jan 24, 2023
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
32 changes: 20 additions & 12 deletions sdk/python/kubeflow/training/api/training_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,35 +36,43 @@
class TrainingClient(object):
def __init__(
self,
config_file=None,
context=None,
client_configuration=None,
persist_config=True,
Copy link
Member

Choose a reason for hiding this comment

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

@andreyvelich Is it possible to set a persistent_config with a client_configuration?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think persistent_config is used when we send client_configuration to load_kube_config() for Kubernetes python client.

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense.

config_file: str = None,
context: str = None,
client_configuration: client.Configuration = None,
no_config: bool = False,
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary? If client_configuration is not None, then it should take precedence over config_file

Copy link
Member

@tenzen-y tenzen-y Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If we remove no_config, I guess we can not inform about missing client_configuration, right?
WDYT?

Copy link
Member Author

@andreyvelich andreyvelich Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@tenzen-y is right . I did this since user can set config_file and client_configuration to add additional information about the config at the same time.

Copy link
Member Author

@andreyvelich andreyvelich Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We had discussion with @johnugeorge around this.

I guess kube-config supports all configuration that user can set via client.Configuration() object (proxy, token. etc.).
@terrytangyuan @tenzen-y Did you ever see the use-case when user wants to set custom client_configuration with existing kube-config ?

If not, we can do this:

  • If user set config_file use load_kube_config().
  • If user doesn't set config_file and doesn't set client_configuration use load_kube_config() or load_incluster_config().
  • If user set client_configuration skip load_kube_config() and load_incluster_config().

no_config flag might look a bit hacky.

Copy link
Member

Choose a reason for hiding this comment

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

No, I have not seen that use case.

Copy link
Member Author

@andreyvelich andreyvelich Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@tenzen-y @terrytangyuan @johnugeorge I modified the PR to ignore kube config when client_configuration is set.
Let's see if we get any user request to change it back.

persist_config: bool = True,
):
"""TrainingClient constructor.

Args:
config_file: Name of the kube-config file. Defaults to ~/.kube/config.
config_file: Path to the kube-config file. Defaults to ~/.kube/config.
context: Set the active context. Defaults to current_context from the kube-config.
client_configuration: The kubernetes.client.Configuration to set configs to.
no_config: Whether to ignore the kube-config for cluster authentication.
In that case, you have to provide the client configuration
with the Bearer token to use the client.
You can find an example here: https://github.com/kubernetes-client/python/blob/67f9c7a97081b4526470cad53576bc3b71fa6fcc/examples/remote_cluster.py#L31
persist_config: If True, config file will be updated when changed.
"""

self.in_cluster = None
if config_file or not utils.is_running_in_k8s():
if no_config and client_configuration is None:
raise ValueError(
"Client configuration must be set when kube-config is ignored"
)

if config_file or (not utils.is_running_in_k8s() and not no_config):
config.load_kube_config(
config_file=config_file,
context=context,
client_configuration=client_configuration,
persist_config=persist_config,
)
self.in_cluster = False
else:
elif not no_config:
config.load_incluster_config()
self.in_cluster = True

self.custom_api = client.CustomObjectsApi()
self.core_api = client.CoreV1Api()
k8s_client = client.ApiClient(client_configuration)
self.custom_api = client.CustomObjectsApi(k8s_client)
self.core_api = client.CoreV1Api(k8s_client)
self.api_client = ApiClient()

# ------------------------------------------------------------------------ #
Expand Down