Python SDK - Generate Name functionality for creating experiments.

[bharathk005]

What this PR does / why we need it:
Adds generate name functionality to Python SDK while creating experiments.
If metadata.name and metadata.generateName are missing, the code appends a new prefix called "exp-" to generateName.
This makes the server generate a new name with the prefix.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #
#2243

[bharathk005]

Please provide feedback on the approach.
I will make changes and do more testing based on this.

[tenzen-y]

@bharathk005 Could you sign to the DCO?

[andreyvelich]

Thank you for updating the SDK @bharathk005!

I think, the point from @fensterwetter was that our SDK should not block to use generateName parameter while using create_experiment API.

So here when we rise Exception or print that Experiment has been created, SDK will fail since experiment.metadata.name doesn't exist.

I think, we can check that name or generate_name exists before running create_namespaced_custom_object and then use this value later in the code.

[bharathk005]

@tenzen-y and @andreyvelich thanks for the suggestions.
Please review the change and let me know.

[andreyvelich]

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

[bharathk005]

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

[tenzen-y]

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?

[andreyvelich]

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

[andreyvelich]

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 ?

[tenzen-y]

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.

[tenzen-y]

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.

[bharathk005]

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

[bharathk005]

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

[andreyvelich]

Thank you for the updates @bharathk005!
/lgtm
/assign @tenzen-y @johnugeorge

[tenzen-y]

I hope to add implementing some unit tests, but we don't have any unit tests now for SDK :(

ref: #2184

/lgtm
/approve

@kubeflow/wg-automl-leads Could you restart some CI jobs?

[tenzen-y]

/hold

[andreyvelich]

Thank you for your contribution @bharathk005 🎉
/hold cancel

[bharathk005]

@andreyvelich is this part of 0.17?

[andreyvelich]

@andreyvelich is this part of 0.17?

Sure, sorry that we haven't merged this PR yet.
@bharathk005 Please can you try to rebase it ? The tests might be fixed.

[bharathk005]

@andreyvelich is this part of 0.17?

Sure, sorry that we haven't merged this PR yet. @bharathk005 Please can you try to rebase it ? The tests might be fixed.

@andreyvelich Done. Please validate.

[andreyvelich]

Thanks for the update @bharathk005!
/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, bharathk005, tenzen-y

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:

• OWNERS [andreyvelich,tenzen-y]

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