Creates a default experiment at API server set up time #1089

rileyjbauer · 2019-04-05T18:31:22Z

This experiment is named Default and is used whenever a CreateRun request is received without specifying a containing experiment.

This change is

…s restarted without clearing DB

…nly covers existence of default experiment

rileyjbauer · 2019-04-08T22:11:14Z

/test kubeflow-pipeline-e2e-test

Ark-kun · 2019-04-10T01:52:18Z

backend/test/initialization/initialization_test.go

+
+// Check the namespace have ML job installed and ready
+func (s *InitializationTest) SetupTest() {
+	if !*runIntegrationTests {


What's the purpose of this switch?

BTW, go test supports test case regex which allows only running the tests you want.

This prevents the test from running as part of the the travis CI tests

neuromage · 2019-04-10T03:08:26Z

backend/src/apiserver/main.go

@@ -61,6 +62,11 @@ func main() {
 		glog.Fatalf("Failed to load samples. Err: %v", err.Error())
 	}

+	err = createDefaultExperiment(resourceManager)
+	if err != nil {
+		glog.Fatalf("Failed to create default experiment. Err: %v", err.Error())


No need for err.Error(), as %v format will do the right thing. err.Error() returns a string, while %v will defer to the String() call of the passed in argument. It's better to defer this to the latter. Here and elsewhere in the code.

Ah, got it. Done.

neuromage · 2019-04-10T03:09:19Z

backend/src/apiserver/main.go

+	// First check that we don't already have a default experiment ID in the DB.
+	defaultExperimentId, err := resourceManager.GetDefaultExperimentId()
+	if err != nil {
+		return errors.New(fmt.Sprintf("Failed to check if default experiment exists. Err: %v", err.Error()))


You can use fmt.Errorf instead of errors.New(fmt.Sprintf(... to achieve the same effect.

Ah, good to know!

neuromage · 2019-04-11T19:22:19Z

backend/src/apiserver/storage/default_experiment_store.go

+	defaultDBValue = sq.Eq{"DefaultExperimentId": ""}
+)
+
+type DefaultExperimentStoreInterface interface {


Do we use this interface anywhere btw?

It's used in client_manager and resource_manager. This is what we do in all of the stores, which of course doesn't mean it's correct, but they should all be changed if it is not, and that should be a separate PR.

neuromage · 2019-04-11T19:22:55Z

backend/src/apiserver/storage/default_experiment_store.go

+)
+
+var (
+	defaultDBValue = sq.Eq{"DefaultExperimentId": ""}


Maybe defaultExperimentDBValue (since this is shared across all files under storage)

Sure, done.

neuromage · 2019-04-11T19:24:48Z

backend/src/apiserver/storage/default_experiment_store.go

+
+func (s *DefaultExperimentStore) InitializeDefaultExperimentTable() error {
+	// First check that the table is in fact empty
+	getDefaultExperimentSql, getDefaultExperimentArgs, err := sq.Select("*").From("default_experiments").ToSql()


You can use shorter names here e.g. querySql and queryArgs. Up to you.

Also, you may want to just write this as SELECT * from default_experiments. There are no parameters here, and the use of sq.select doesn't buy us anything, and probably makes it harder to read. Also, saves having to check error below, since that cannot happen.

Good catch. I updated db_status_store as well

neuromage

/lgtm

IronPan · 2019-04-15T18:20:04Z

backend/src/apiserver/resource/resource_manager.go

@@ -214,6 +217,31 @@ func (r *ResourceManager) CreateRun(apiRun *api.Run) (*model.RunDetail, error) {
 		return nil, util.NewInternalServerError(err, "Failed to create a workflow for (%s)", workflow.Name)
 	}

+	hasExperiment := false


could you move the logic here into a util function, maybe named something like setExperimentIfNotPresent?

IronPan · 2019-04-15T18:23:20Z

backend/src/apiserver/storage/default_experiment_store.go

+}
+
+// Implementation of a DefaultExperimentStoreInterface. This stores the default experiment's ID,
+// which is created as the API server is initialized.


the first time the API server is initialized.

IronPan · 2019-04-15T18:24:34Z

backend/src/apiserver/storage/default_experiment_store.go

+	db *DB
+}
+
+func (s *DefaultExperimentStore) InitializeDefaultExperimentTable() error {


this can be a private method.

IronPan · 2019-04-15T18:30:53Z

test/initialization-test/OWNERS

@@ -0,0 +1,6 @@
+approvers:


add your name?

IronPan · 2019-04-15T21:00:21Z

/lgtm
/approve

k8s-ci-robot · 2019-04-15T21:00:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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:

~~backend/OWNERS~~ [IronPan]
~~test/OWNERS~~ [IronPan]

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

rileyjbauer · 2019-04-15T21:08:31Z

/test kubeflow-pipeline-e2e-test

rileyjbauer · 2019-04-15T22:07:57Z

/test kubeflow-pipeline-e2e-test

* fix nested param generation issue * change logic to only apply to parent counter

rileyjbauer added 6 commits April 3, 2019 16:52

WIP - Create default experiment upon API server initialization

e39445d

Default experiment initialization caused crashes if API server pod wa…

864f4ec

…s restarted without clearing DB

Adding new table to store default experiment ID

ba78837

Add default experiment type model definition

9d572f1

Minor fixes, everything seems to work now

29995a8

Clean up. Renamed to default_experiment_store

44c5e76

rileyjbauer assigned neuromage and IronPan Apr 5, 2019

k8s-ci-robot added do-not-merge/work-in-progress labels Apr 5, 2019

k8s-ci-robot requested review from IronPan and vicaire April 5, 2019 18:31

k8s-ci-robot added size/L labels Apr 5, 2019

rileyjbauer added 2 commits April 5, 2019 17:40

Adds tests for the default_experiment_store

8b204b3

Add integration test for verifying initial cluster state. Currently o…

35dcc3b

…nly covers existence of default experiment

k8s-ci-robot added size/XL and removed size/L labels Apr 8, 2019

Don't run initialization tests except as integration tests

28b54c4

rileyjbauer changed the title ~~[WIP] - Creates a default experiment at API server set up time~~ Creates a default experiment at API server set up time Apr 8, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress label Apr 8, 2019

Fixes comments

1d816ce

Ark-kun reviewed Apr 10, 2019

View reviewed changes

neuromage reviewed Apr 11, 2019

View reviewed changes

PR comments and cleanup

09c95f8

neuromage reviewed Apr 12, 2019

View reviewed changes

k8s-ci-robot added lgtm labels Apr 12, 2019

IronPan reviewed Apr 15, 2019

View reviewed changes

Extract code in resource_manager to helper func

5b02d36

k8s-ci-robot removed lgtm labels Apr 15, 2019

k8s-ci-robot added the lgtm label Apr 15, 2019

k8s-ci-robot added the approved label Apr 15, 2019

rileyjbauer mentioned this pull request Apr 15, 2019

Test coverage for runs without experiments #685

Closed

k8s-ci-robot merged commit fbae0f8 into kubeflow:master Apr 15, 2019

rileyjbauer mentioned this pull request Apr 16, 2019

Allow creating runs without experiments #1175

Merged

rileyjbauer deleted the create-default-experiment branch May 6, 2019 22:13

HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024

fix(sdk): fix nested param generation issue (kubeflow#1089)

0ab85f9

* fix nested param generation issue * change logic to only apply to parent counter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creates a default experiment at API server set up time #1089

Creates a default experiment at API server set up time #1089

rileyjbauer commented Apr 5, 2019 •

edited

Loading

rileyjbauer commented Apr 8, 2019

Ark-kun Apr 10, 2019

rileyjbauer Apr 11, 2019

neuromage Apr 10, 2019

rileyjbauer Apr 11, 2019

neuromage Apr 10, 2019

rileyjbauer Apr 11, 2019

neuromage Apr 11, 2019

rileyjbauer Apr 11, 2019

neuromage Apr 11, 2019

rileyjbauer Apr 11, 2019

neuromage Apr 11, 2019

rileyjbauer Apr 11, 2019

neuromage left a comment

IronPan Apr 15, 2019

rileyjbauer Apr 15, 2019

IronPan Apr 15, 2019

rileyjbauer Apr 15, 2019

IronPan Apr 15, 2019

rileyjbauer Apr 15, 2019

IronPan Apr 15, 2019

rileyjbauer Apr 15, 2019

IronPan commented Apr 15, 2019

k8s-ci-robot commented Apr 15, 2019

rileyjbauer commented Apr 15, 2019

rileyjbauer commented Apr 15, 2019

Creates a default experiment at API server set up time #1089

Creates a default experiment at API server set up time #1089

Conversation

rileyjbauer commented Apr 5, 2019 • edited Loading

rileyjbauer commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neuromage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IronPan commented Apr 15, 2019

k8s-ci-robot commented Apr 15, 2019

rileyjbauer commented Apr 15, 2019

rileyjbauer commented Apr 15, 2019

rileyjbauer commented Apr 5, 2019 •

edited

Loading