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

Python SDK - Generate Name functionality for creating experiments. #2272

Merged
merged 12 commits into from
Apr 2, 2024
Merged

Python SDK - Generate Name functionality for creating experiments. #2272

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e7eefc5
added dco
bharathk005 Mar 5, 2024
1039f4b
updated condition
bharathk005 Mar 5, 2024
5be7150
added exception to catch missing name and generateName
bharathk005 Mar 7, 2024
ccc1be2
updated experiment_name in create_experiment
bharathk005 Mar 7, 2024
15d4582
py sdk create_exp - added type validation
bharathk005 Mar 10, 2024
9e24312
Merge branch 'kubeflow:master' into feature-py-sdk-create-exp-generat…
bharathk005 Apr 2, 2024
84f42d3
added dco
bharathk005 Mar 5, 2024
e75ad6b
updated condition
bharathk005 Mar 5, 2024
67f12c1
added exception to catch missing name and generateName
bharathk005 Mar 7, 2024
56b3f31
updated experiment_name in create_experiment
bharathk005 Mar 7, 2024
c583c68
py sdk create_exp - added type validation
bharathk005 Mar 10, 2024
a4e4b62
Merge branch 'feature-py-sdk-create-exp-generateName' of github.com:b…
bharathk005 Apr 2, 2024
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
20 changes: 13 additions & 7 deletions sdk/python/v1beta1/kubeflow/katib/api/katib_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,29 +93,35 @@ def create_experiment(

namespace = namespace or self.namespace

if 'name' in experiment.metadata and experiment.metadata.name:
Copy link
Member

Choose a reason for hiding this comment

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

I think, it will raise Exception if experiment.metadata.name is not set.
We might need to have something like this:

         experiment_name = None

        if type(experiment) == models.V1beta1Experiment:
            if experiment.metadata.name is not None:
                experiment_name = experiment.metadata.name
            elif experiment.metadata.generate_name is not None:
                experiment_name = experiment.metadata.generate_name
        elif "name" in experiment["metadata"]:
            experiment_name = experiment["metadata"]["name"]
        elif "generateName" in experiment["metadata"]:
            experiment_name = experiment["metadata"]["generateName"]
        if experiment_name is None:
            raise ValueError("Experiment must have name or generateName")

Any thoughts @droctothorpe @tenzen-y @johnugeorge @bharathk005 @fensterwetter

Copy link
Contributor Author

@bharathk005 bharathk005 Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@andreyvelich thanks for the review.
I have added some changes based on your suggestions. Test cases pass on my machine.

Regarding the type assertion, the function already expects a "models.V1beta1Experiment" type:

katib/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

Lines 78 to 80 in 1039f4b

def create_experiment(
self,
experiment: models.V1beta1Experiment,

Please let me know if we still need to check for the type.

Copy link
Member

Choose a reason for hiding this comment

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

I think, it will raise Exception if experiment.metadata.name is not set. We might need to have something like this:

         experiment_name = None

        if type(experiment) == models.V1beta1Experiment:
            if experiment.metadata.name is not None:
                experiment_name = experiment.metadata.name
            elif experiment.metadata.generate_name is not None:
                experiment_name = experiment.metadata.generate_name
        elif "name" in experiment["metadata"]:
            experiment_name = experiment["metadata"]["name"]
        elif "generateName" in experiment["metadata"]:
            experiment_name = experiment["metadata"]["generateName"]
        if experiment_name is None:
            raise ValueError("Experiment must have name or generateName")

Any thoughts @droctothorpe @tenzen-y @johnugeorge @bharathk005 @fensterwetter

Additionally, I would suggest to add the Exception if both name and generateName are specified.
@andreyvelich WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Won't Kubernetes Python client throw an error when we try to create custom resources in that case ?

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the type assertion, the function already expects a "models.V1beta1Experiment" type:

That's true, but since Python is not strong typed language, users can still send dict as Experiment. Otherwise, we need to check type(experiment) and through exception. Do we want to give users functionality to pass dict with create_experiment API @droctothorpe @tenzen-y @johnugeorge ?

Copy link
Member

@tenzen-y tenzen-y Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Won't Kubernetes Python client throw an error when we try to create custom resources in that case ?

I imaged like 102-103:

katib/sdk/python/v1beta1/kubeflow/katib/api/katib_client.py

Lines 102 to 103 in 5be7150

if experiment_name is None:
raise ValueError("Experiment must have a name or generateName")

So, my concerns have gone away. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

That's true, but since Python is not strong typed language, users can still send dict as Experiment. Otherwise, we need to check type(experiment) and through exception. Do we want to give users functionality to pass dict with create_experiment API @droctothorpe @tenzen-y @johnugeorge ?

I agree with the necessary for type assertion, but I think this is PR to support name generation.
So, I'm ok with working on it in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tenzen-y @andreyvelich thanks for the comments.

  1. Type assertion
  2. Throwing error if both name and generateName are provided
    Are these the changes that need to be implemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tenzen-y @andreyvelich Thanks for your time. I have pushed a commit with the changes mentioned in this thread. Please review and let me know.

experiment_name = experiment.metadata.name
else:
experiment_name = experiment.metadata.generate_name

try:
self.custom_api.create_namespaced_custom_object(
outputs = self.custom_api.create_namespaced_custom_object(
constants.KUBEFLOW_GROUP,
constants.KATIB_VERSION,
namespace,
constants.EXPERIMENT_PLURAL,
experiment,
)
experiment_name = outputs.metadata.name # if "generate_name" is used, "name" gets a prefix from server
except multiprocessing.TimeoutError:
raise TimeoutError(
f"Timeout to create Katib Experiment: {namespace}/{experiment.metadata.name}"
f"Timeout to create Katib Experiment: {namespace}/{experiment_name}"
)
except Exception as e:
if hasattr(e, "status") and e.status == 409:
raise Exception(
f"A Katib Experiment with the name {namespace}/{experiment.metadata.name} already exists."
f"A Katib Experiment with the name {namespace}/{experiment_name} already exists."
)
raise RuntimeError(
f"Failed to create Katib Experiment: {namespace}/{experiment.metadata.name}"
f"Failed to create Katib Experiment: {namespace}/{experiment_name}"
)

# TODO (andreyvelich): Use proper logger.
print(f"Experiment {namespace}/{experiment.metadata.name} has been created")
print(f"Experiment {namespace}/{experiment_name} has been created")

if self._is_ipython():
if self.in_cluster:
Expand All @@ -125,9 +131,9 @@ def create_experiment(
IPython.display.HTML(
"Katib Experiment {} "
'link <a href="/_/katib/#/katib/hp_monitor/{}/{}" target="_blank">here</a>'.format(
experiment.metadata.name,
experiment_name,
namespace,
experiment.metadata.name,
experiment_name,
)
)
)
Expand Down
Loading