Skip to content

Commit

Permalink
use trialSpec.metadata as prefix of trialMeta reference
Browse files Browse the repository at this point in the history
  • Loading branch information
sperlingxx committed Jul 16, 2020
1 parent f3d58b9 commit 92ead2c
Show file tree
Hide file tree
Showing 20 changed files with 326 additions and 277 deletions.
20 changes: 12 additions & 8 deletions pkg/controller.v1beta1/consts/const.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,17 +137,21 @@ const (
// LabelTrialTemplateConfigMapValue is the label value for the Trial templates configMap
LabelTrialTemplateConfigMapValue = "katib-trial-templates"

// TrialTemplateReplaceFormat is the format to make substitution in Trial template from Names in TrialParameters
// TrialTemplateParamReplaceFormat is the format to make substitution in Trial template from Names in TrialParameters
// E.g if Name = learningRate, according value in Trial template must be ${trialParameters.learningRate}
TrialTemplateReplaceFormat = "${trialParameters.%v}"
TrialTemplateParamReplaceFormat = "${trialParameters.%v}"

// TrialTemplateReplaceFormatRegex is the regex for Trial template format
TrialTemplateReplaceFormatRegex = "\\{trialParameters\\..+?\\}"
// TrialTemplateParamReplaceFormatRegex is the regex for Trial template format
TrialTemplateParamReplaceFormatRegex = "\\{trialParameters\\..+?\\}"

// TrialTemplateTrialName is the placeholder of trial name in trial template
TrialTemplateTrialName = "Name"
// TrialTemplateTrialNamespace is the placeholder of trial's namespace in trial template
TrialTemplateTrialNamespace = "Namespace"
TrialTemplateMetaReplaceFormat = "${trialSpec.metadata.%v}"

TrialTemplateMetaReplaceFormatRegex = "\\{trialSpec\\.metadata\\..+?\\}"

TrialTemplateMetaKeyOfName = "name"
TrialTemplateMetaKeyOfNamespace = "namespace"
TrialTemplateMetaKeyOfKind = "kind"
TrialTemplateMetaKeyOfAPIVersion = "apiVersion"

// UnavailableMetricValue is the value when metric was not reported or metric value can't be converted to float64
UnavailableMetricValue = "unavailable"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func TestReconcileExperiment(t *testing.T) {
t.Errorf("ConvertObjectToUnstructured failed: %v", err)
}
generator.EXPECT().GetRunSpecWithHyperParameters(gomock.Any(), gomock.Any(),
gomock.Any(), gomock.Any()).Return(
gomock.Any(), gomock.Any(), gomock.Any()).Return(
returnedUnstructured,
nil).AnyTimes()

Expand Down
12 changes: 11 additions & 1 deletion pkg/controller.v1beta1/experiment/experiment_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package experiment

import (
"context"
"github.com/kubeflow/katib/pkg/controller.v1beta1/consts"

"k8s.io/apimachinery/pkg/api/errors"

Expand Down Expand Up @@ -38,7 +39,7 @@ func (r *ReconcileExperiment) createTrialInstance(expInstance *experimentsv1beta
hps := trialAssignment.ParameterAssignments
trial.Spec.ParameterAssignments = trialAssignment.ParameterAssignments

runSpec, err := r.GetRunSpecWithHyperParameters(expInstance, trial.Name, trial.Namespace, hps)
runSpec, err := r.GetRunSpecWithHyperParameters(expInstance, trial.Name, trial.Namespace, hps, buildTrialMetaForRunSpec(trial))
if err != nil {
logger.Error(err, "Fail to get RunSpec from experiment", expInstance.Name)
return err
Expand All @@ -61,6 +62,15 @@ func (r *ReconcileExperiment) createTrialInstance(expInstance *experimentsv1beta

}

func buildTrialMetaForRunSpec(trial *trialsv1beta1.Trial) map[string]string {
return map[string]string{
consts.TrialTemplateMetaKeyOfName: trial.Name,
consts.TrialTemplateMetaKeyOfNamespace: trial.Namespace,
consts.TrialTemplateMetaKeyOfKind: trial.Kind,
consts.TrialTemplateMetaKeyOfAPIVersion: trial.APIVersion,
}
}

func needUpdateFinalizers(exp *experimentsv1beta1.Experiment) (bool, []string) {
deleted := !exp.ObjectMeta.DeletionTimestamp.IsZero()
pendingFinalizers := exp.GetFinalizers()
Expand Down
32 changes: 19 additions & 13 deletions pkg/controller.v1beta1/experiment/manifest/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const (
type Generator interface {
InjectClient(c client.Client)
GetTrialTemplate(instance *experimentsv1beta1.Experiment) (string, error)
GetRunSpecWithHyperParameters(experiment *experimentsv1beta1.Experiment, trialName, trialNamespace string, assignments []commonapiv1beta1.ParameterAssignment) (*unstructured.Unstructured, error)
GetRunSpecWithHyperParameters(experiment *experimentsv1beta1.Experiment, trialName, trialNamespace string, assignments []commonapiv1beta1.ParameterAssignment, trialMeta map[string]string) (*unstructured.Unstructured, error)
GetSuggestionConfigData(algorithmName string) (map[string]string, error)
GetMetricsCollectorImage(cKind commonapiv1beta1.CollectorKind) (string, error)
}
Expand Down Expand Up @@ -60,22 +60,21 @@ func (g *DefaultGenerator) GetSuggestionConfigData(algorithmName string) (map[st
}

// GetRunSpecWithHyperParameters returns the specification for trial with hyperparameters.
func (g *DefaultGenerator) GetRunSpecWithHyperParameters(experiment *experimentsv1beta1.Experiment, trialName, trialNamespace string, assignments []commonapiv1beta1.ParameterAssignment) (*unstructured.Unstructured, error) {
func (g *DefaultGenerator) GetRunSpecWithHyperParameters(experiment *experimentsv1beta1.Experiment, trialName, trialNamespace string, assignments []commonapiv1beta1.ParameterAssignment, trialMeta map[string]string) (*unstructured.Unstructured, error) {

// Get string Trial template from Experiment spec
trialTemplate, err := g.GetTrialTemplate(experiment)
if err != nil {
return nil, err
}

// Trial meta data which may be required by TrialTemplate
trialMeta := map[string]string{
consts.TrialTemplateTrialName: trialName,
consts.TrialTemplateTrialNamespace: trialNamespace,
}

// Apply parameters to Trial Template from assignment
replacedTemplate, err := g.applyParameters(trialTemplate, experiment.Spec.TrialTemplate.TrialParameters, assignments, trialMeta)
replacedTemplate, err := g.applyParameters(trialTemplate, experiment.Spec.TrialTemplate.TrialParameters, assignments)
if err != nil {
return nil, err
}
// Apply metadata to Trial Template from assignment
replacedTemplate, err = g.applyMetadata(replacedTemplate, trialMeta)
if err != nil {
return nil, err
}
Expand All @@ -92,7 +91,7 @@ func (g *DefaultGenerator) GetRunSpecWithHyperParameters(experiment *experiments
return runSpec, nil
}

func (g *DefaultGenerator) applyParameters(trialTemplate string, trialParams []experimentsv1beta1.TrialParameterSpec, assignments []commonapiv1beta1.ParameterAssignment, trialMeta map[string]string) (string, error) {
func (g *DefaultGenerator) applyParameters(trialTemplate string, trialParams []experimentsv1beta1.TrialParameterSpec, assignments []commonapiv1beta1.ParameterAssignment) (string, error) {
// Number of parameters must be equal
if len(assignments) != len(trialParams) {
return "", fmt.Errorf("Number of Trial assignment from Suggestion: %v not equal to number Trial parameters from Experiment: %v", len(assignments), len(trialParams))
Expand All @@ -107,15 +106,22 @@ func (g *DefaultGenerator) applyParameters(trialTemplate string, trialParams []e
for _, parameter := range trialParams {

if parameterValue, ok := assignmentsMap[parameter.Reference]; ok {
trialTemplate = strings.Replace(trialTemplate, fmt.Sprintf(consts.TrialTemplateReplaceFormat, parameter.Name), parameterValue, -1)
trialTemplate = strings.Replace(trialTemplate, fmt.Sprintf(consts.TrialTemplateParamReplaceFormat, parameter.Name), parameterValue, -1)
} else {
return "", fmt.Errorf("Unable to find parameter: %v in parameter assignment %v", parameter.Reference, assignmentsMap)
}
}

return trialTemplate, nil
}

func (g *DefaultGenerator) applyMetadata(trialTemplate string, trialMeta map[string]string) (string, error) {
var tempMatchKey string
// Inject TrialMeta if needed
for key, value := range trialMeta {
if strings.Contains(trialTemplate, fmt.Sprintf(consts.TrialTemplateReplaceFormat, key)) {
trialTemplate = strings.Replace(trialTemplate, fmt.Sprintf(consts.TrialTemplateReplaceFormat, key), value, -1)
tempMatchKey = fmt.Sprintf(consts.TrialTemplateMetaReplaceFormat, key)
if strings.Contains(trialTemplate, tempMatchKey) {
trialTemplate = strings.Replace(trialTemplate, tempMatchKey, value, -1)
}
}

Expand Down
33 changes: 25 additions & 8 deletions pkg/controller.v1beta1/experiment/manifest/generator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,18 @@ package manifest

import (
"errors"
"github.com/kubeflow/katib/pkg/controller.v1beta1/consts"
"math"
"reflect"
"testing"

"github.com/golang/mock/gomock"
commonapiv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/common/v1beta1"
experimentsv1beta1 "github.com/kubeflow/katib/pkg/apis/controller/experiments/v1beta1"
util "github.com/kubeflow/katib/pkg/controller.v1beta1/util"
"github.com/kubeflow/katib/pkg/controller.v1beta1/util"
katibclientmock "github.com/kubeflow/katib/pkg/mock/v1beta1/util/katibclient"
batchv1 "k8s.io/api/batch/v1"
v1 "k8s.io/api/core/v1"
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)
Expand Down Expand Up @@ -51,8 +52,10 @@ func TestGetRunSpecWithHP(t *testing.T) {
"--num-layers=5",
},
Env: []v1.EnvVar{
{Name: "trial_name", Value: "trial-name"},
{Name: "trial_ns", Value: "trial-namespace"},
{Name: consts.TrialTemplateMetaKeyOfName, Value: "trial-name"},
{Name: consts.TrialTemplateMetaKeyOfNamespace, Value: "trial-namespace"},
{Name: consts.TrialTemplateMetaKeyOfKind, Value: "Job"},
{Name: consts.TrialTemplateMetaKeyOfAPIVersion, Value: "batch/v1"},
},
},
},
Expand Down Expand Up @@ -122,8 +125,14 @@ func TestGetRunSpecWithHP(t *testing.T) {
},
}

mockMetadata := map[string]string{
"name": "trial-name",
"namespace": "trial-namespace",
"kind": "Job",
"apiVersion": "batch/v1",
}
for _, tc := range tcs {
actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments)
actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments, mockMetadata)

if tc.Err && err == nil {
t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription)
Expand Down Expand Up @@ -300,8 +309,14 @@ spec:
},
}

mockMetadata := map[string]string{
"name": "trial-name",
"namespace": "trial-namespace",
"kind": "Job",
"apiVersion": "batch/v1",
}
for _, tc := range tcs {
actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments)
actualRunSpec, err := p.GetRunSpecWithHyperParameters(tc.Instance, "trial-name", "trial-namespace", tc.ParameterAssignments, mockMetadata)
if tc.Err && err == nil {
t.Errorf("Case: %v failed. Expected err, got nil", tc.testDescription)
} else if !tc.Err {
Expand Down Expand Up @@ -335,8 +350,10 @@ func newFakeInstance() *experimentsv1beta1.Experiment {
"--num-layers=${trialParameters.numberLayers}",
},
Env: []v1.EnvVar{
{Name: "trial_name", Value: "${trialParameters.Name}"},
{Name: "trial_ns", Value: "${trialParameters.Namespace}"},
{Name: consts.TrialTemplateMetaKeyOfName, Value: "${trialSpec.metadata.name}"},
{Name: consts.TrialTemplateMetaKeyOfNamespace, Value: "${trialSpec.metadata.namespace}"},
{Name: consts.TrialTemplateMetaKeyOfKind, Value: "${trialSpec.metadata.kind}"},
{Name: consts.TrialTemplateMetaKeyOfAPIVersion, Value: "${trialSpec.metadata.apiVersion}"},
},
},
},
Expand Down
34 changes: 17 additions & 17 deletions pkg/mock/v1alpha3/api/manager.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 92ead2c

Please sign in to comment.