New Trial Template API controller implementation by andreyvelich · Pull Request #1202 · kubeflow/katib

andreyvelich · 2020-06-02T15:16:41Z

This is new API for Trial Template.
See comment: #906 (comment).

In TrialParameters user must define parameters that are replaced in Trial Template.
As @gaocegege proposed, I added Reference to make connection between TrialParameters and Parameters (search space).
Also, in TrialSpec metadata.name and metadata.namespace must be omitted, since we automatically added it in Katib controller.

I added ConfigMap specification to TrialTemplate, if user wants to read Trial Template from configMap.

In the future, we can extend TrialTemplate with custom settings (for example, how to get succeeded state of the Job), to support any Kubernetes CRDs.
In this CRDs we should be able to run TrialJobs, such as Argo, etc (#1081).

/assign @gaocegege @jlewi @johnugeorge @sperlingxx
/cc @czheng94 @terrykong @nielsmeima

k8s-ci-robot · 2020-06-02T15:16:44Z

@andreyvelich: GitHub didn't allow me to request PR reviews from the following users: nielsmeima, czheng94, terrykong.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

This is new API for Trial Template.
See comment: #906 (comment).

In TrialParameters user must define parameters that are replaced in Trial Template.
As @gaocegege proposed, I added Reference to make connection between TrialParameters and Parameters (search space).
Also, in TrialSpec metadata.name and metadata.namespace must be omitted, since we automatically added it in Katib controller.

I added ConfigMap specification to TrialTemplate, if user wants to read Trial Template from configMap.

In the future, we can extend TrialTemplate with custom settings (for example, how to get succeeded state of the Job), to support any Kubernetes CRDs.
In this CRDs we should be able to run TrialJobs, such as Argo, etc (#1081).

/assign @gaocegege @jlewi @johnugeorge @sperlingxx
/cc @czheng94 @terrykong @nielsmeima

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.

kubeflow-bot · 2020-06-02T15:16:49Z

This change is

gaocegege · 2020-06-03T01:52:16Z

+	TrialSpec *unstructured.Unstructured `json:"trialSpec,omitempty"`
+
+	// Name of config map where Trial template is located
+	ConfigMapName string `json:"configMapName,omitempty"`


The field is already in GoTemplate, should we remove it here?

Yeah, but I think when we implement new logic we will not support GoTemplate ?

SGTM, then can we just remove gotemplate?

gaocegege · 2020-06-03T01:52:28Z

+	// Retain indicates that Trial resources must be not cleanup
+	Retain bool `json:"retain,omitempty"`
+
 	GoTemplate *GoTemplate `json:"goTemplate,omitempty"`


Can we move it into TrialSpec?

In that case what do you think about the name where Unstructured template will be located?

type TrialSpec struct { <must be some name> *unstructured.Unstructured `json:"<name>,omitempty"` ConfigMapName string `json:"configMapName,omitempty"` ConfigMapNamespace string `json:"configMapNamespace,omitempty"` TemplatePath string `json:"templatePath,omitempty"` }

How about this (learn from https://github.com/kubernetes/api/blob/master/core/v1/types.go#L51):

type TrialTemplate struct { Retain bool `json:"retain,omitempty"` TrialSource `json:",inline"` // List of parameres that are used in Trial template TrialParameters []TrialParameterSpec `json:"trialParameters,omitempty"` } type TrialSource struct { ConfigMap *ConfigMapSource `json:"configMap"` TrialSpec *unstructured.Unstructured `json:"trialSpec"` } type ConfigMapSource struct { // Name of config map where Trial template is located ConfigMapName string `json:"configMapName,omitempty"` // Namespace of config map where Trial template is located ConfigMapNamespace string `json:"configMapNamespace,omitempty"` // Path in config map where Trial template is located TemplatePath string `json:"templatePath,omitempty"` }

@gaocegege SGTM, it is nice to follow k8s APIs structure way.
What do you think @johnugeorge ?

Looks great

sperlingxx · 2020-06-03T03:34:10Z

Generally, it looks great to me. Will we deprecate rawTemplate or keep supporting it?

johnugeorge · 2020-06-03T05:52:01Z

+	// Retain indicates that Trial resources must be not cleanup
+	Retain bool `json:"retain,omitempty"`
+
 	GoTemplate *GoTemplate `json:"goTemplate,omitempty"`


Are we supporting Gotemplate also?

All current examples in v1beta1 are working with GoTemplate.
You think we should remove it and don't run e2e test until we implement new logic?

I think we should remove and disable test if we decide to remove it.

I agree. Lets make all changes to trial template API in a single PR for better tracking . Once new TrialTemplate implementation is added, move all examples to new trial template.

Ok, I will remove GoTemplate in this PR and disable all e2e tests.

andreyvelich · 2020-06-03T10:02:05Z

Generally, it looks great to me. Will we deprecate rawTemplate or keep supporting it?

Thanks. I think we can deprecate it, when we implement new logic. Can't find any case when rawTemplate can give us additional functionality.

jlewi · 2020-06-03T12:23:30Z

Great to see this coming along. I'll let the Katib experts review this.

Remove GoTemplate from Experiment API

andreyvelich · 2020-06-04T19:18:37Z

If we delete GoTemplate API in this PR, v1alpha3 controller image also can't be built.

We need to fix problems in API package, for example: https://github.com/andreyvelich/katib/blob/new-trial-template-api/pkg/apis/controller/experiments/v1beta1/experiment_defaults.go#L57.

So maybe we can delete GoTemplate in the next PR, where we implement new logic for the controller.
In that case, we will not stop Katib development on other issues.

What do you think @gaocegege @johnugeorge ?

gaocegege · 2020-06-05T01:27:44Z

If we delete GoTemplate API in this PR, v1alpha3 controller image also can't be built.

I am not sure why. I think v1alpha3 is decoupled with v1beta1.

andreyvelich · 2020-06-05T01:49:14Z

If we delete GoTemplate API in this PR, v1alpha3 controller image also can't be built.

I am not sure why. I think v1alpha3 is decoupled with v1beta1.

I think the problem is with this: https://github.com/kubeflow/katib/blob/master/pkg/apis/controller/addtoscheme_katib_v1beta1.go#L26-L29.
Since v1beta1 and v1alpha3 in the same package apis, while building image for v1alpha3, it checks that we don't have errors in v1beta1 package: https://github.com/kubeflow/katib/blob/master/pkg/apis/controller/experiments/v1beta1/register.go#L23.

So the solution can be comment the lines for v1beta1, from init() function:
https://github.com/kubeflow/katib/blob/master/pkg/apis/controller/addtoscheme_katib_v1beta1.go#L26-L29

Then, I can build image for v1alpha3.

Implement new Trial Template to controller

Fix experiment defaults change valid/invalid experiment

andreyvelich · 2020-06-08T00:56:35Z

I commited changes for the controller and examples in this PR to avoid problems in testing.
List of changes:

Change all examples in v1beta1 to support new Trial Template
Submit new API changes for Experiment API and Trial API (RunSpec now also unstructured).
Modify deepcopy and experiment_defaults to support new API
Delete old trial template configMap. I left only ConfigMap with trial template which is labeled: app: katib-trial-templates. So users can correct see them in the UI.

In Experiment Controller I think we should createTrialInstance only when Suggestion returns some trials. It can avoid unexpected errors with already created trials.

katib/pkg/controller.v1beta1/experiment/experiment_controller.go

Lines 353 to 360 in cebec02

    
           // If Trial Assignment is not ready we don't need to create Trials 
        
           if len(trialNames) != 0 { 
        
           	logger.Info("Create Trials", "trialNames", trialNames) 
        
           	for _, trial := range trials { 
        
           		if err = r.createTrialInstance(instance, &trial); err != nil { 
        
           			logger.Error(err, "Create trial instance error", "trial", trial) 
        
           			continue 
        
           		}

Modify all tests for Experiment and Trial controller.
Add new logic in generator.go to support new Trial Template.
Add new test for generator_test.go.
Comment Validator for Trial Template. Need to modify it in the future PRs.

I think it is ready for review.
After this PR, we should pass the test for the new Trial Template. We still need to add validation and improve generator_test.go with more tests, which can be tracked here: #1208.

/cc @gaocegege @johnugeorge

sperlingxx · 2020-06-08T03:32:26Z

-		if err = r.createTrialInstance(instance, &trial); err != nil {
-			logger.Error(err, "Create trial instance error", "trial", trial)
-			continue
+		trialNames = append(trialNames, trial.Name)


I think this paragraph can be simplified to

for _, trial := range trials { if err = r.createTrialInstance(instance, &trial); err != nil { logger.Error(err, "Create trial instance error", "trial", trial) continue } }

In that case we try to create trials even if trials==[ ] . Is that correct?

@andreyvelich If trials == [], the foreach loop will be skipped?

@sperlingxx Thanks, I got your point. Will make change.

gaocegege

/lgtm

Thanks for your contribution! 🎉 👍

gaocegege · 2020-06-10T01:53:05Z

/lgtm

/assign @sperlingxx @johnugeorge

sperlingxx

/lgtm

Great contribution!

johnugeorge · 2020-06-10T14:23:43Z

/approve

k8s-ci-robot · 2020-06-10T14:23:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [johnugeorge]

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

czheng94 · 2020-06-10T17:10:49Z

So excited about this!! 🎉

* Add new Trial Template API * Add TrialSource in TrialTemplate Remove GoTemplate from Experiment API * Add init logic for new trial template * Change examples Implement new Trial Template to controller * Modify trial template configMap for new version of Trial Template Fix experiment defaults change valid/invalid experiment * Run gofmt * Fix hyperband * Remove num-epochs from grid example * Add tag to file mc example * Modify create Trials loop

Add new Trial Template API

b1eb0e9

k8s-ci-robot assigned gaocegege, jlewi, johnugeorge and sperlingxx Jun 2, 2020

k8s-ci-robot added the size/M label Jun 2, 2020

gaocegege reviewed Jun 3, 2020

View reviewed changes

johnugeorge reviewed Jun 3, 2020

View reviewed changes

jlewi removed their assignment Jun 3, 2020

Add TrialSource in TrialTemplate

59ea302

Remove GoTemplate from Experiment API

k8s-ci-robot added size/L and removed size/M labels Jun 5, 2020

andreyvelich mentioned this pull request Jun 5, 2020

Support new Trial Template in v1beta1 #1208

Closed

6 tasks

andreyvelich force-pushed the new-trial-template-api branch from 1868e6b to 59ea302 Compare June 7, 2020 21:19

k8s-ci-robot added size/M and removed size/L labels Jun 7, 2020

andreyvelich added 2 commits June 7, 2020 22:29

Add init logic for new trial template

99ef718

Change examples

05c1ef6

Implement new Trial Template to controller

k8s-ci-robot added size/XXL and removed size/M labels Jun 7, 2020

Modify trial template configMap for new version of Trial Template

cebec02

Fix experiment defaults change valid/invalid experiment

k8s-ci-robot requested review from gaocegege and johnugeorge June 8, 2020 00:56

andreyvelich added 4 commits June 8, 2020 02:15

Run gofmt

bca595e

Fix hyperband

29c7383

Remove num-epochs from grid example

9428fc0

Add tag to file mc example

6293107

sperlingxx reviewed Jun 8, 2020

View reviewed changes

andreyvelich changed the title ~~New Trial Template API~~ New Trial Template API controller implementation Jun 8, 2020

gaocegege reviewed Jun 9, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Jun 9, 2020

Modify create Trials loop

d441cb4

k8s-ci-robot removed the lgtm label Jun 9, 2020

k8s-ci-robot added the lgtm label Jun 10, 2020

sperlingxx approved these changes Jun 10, 2020

View reviewed changes

k8s-ci-robot added the approved label Jun 10, 2020

k8s-ci-robot merged commit 88da808 into kubeflow:master Jun 10, 2020

andreyvelich mentioned this pull request Jun 10, 2020

[Feature] Modify Job provider to support any kind of Kubernetes CRDs #1214

Closed

andreyvelich mentioned this pull request Jul 20, 2020

support trial meta injection in trial template rendering #1259

Merged

andreyvelich deleted the new-trial-template-api branch October 6, 2021 00:27

Conversation

andreyvelich commented Jun 2, 2020

Uh oh!

k8s-ci-robot commented Jun 2, 2020

Uh oh!

kubeflow-bot commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaocegege Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sperlingxx commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvelich commented Jun 3, 2020

Uh oh!

jlewi commented Jun 3, 2020

Uh oh!

andreyvelich commented Jun 4, 2020

Uh oh!

gaocegege commented Jun 5, 2020

Uh oh!

andreyvelich commented Jun 5, 2020

Uh oh!

andreyvelich commented Jun 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaocegege left a comment

Choose a reason for hiding this comment

Uh oh!

gaocegege commented Jun 10, 2020

Uh oh!

sperlingxx left a comment

Choose a reason for hiding this comment

Uh oh!

johnugeorge commented Jun 10, 2020

Uh oh!

k8s-ci-robot commented Jun 10, 2020

Uh oh!

czheng94 commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gaocegege Jun 4, 2020 •

edited

Loading

sperlingxx commented Jun 3, 2020 •

edited

Loading