Added `databricks_mlflow_model` and `databricks_mlflow_experiment` resources #931

jensendw · 2021-11-19T21:21:11Z

Provides basic support for managing MLflow models and experiments, should hopefully address #861 as well.

databricks_mlflow_experiment

resource "databricks_mlflow_experiment" "test" {
  name = "/Users/myuserid/my-experiment"
  artifact_location = "dbfs:/tmp/my-experiment"
  description = "My MLflow experiment description"
}

Argument Reference

The following arguments are supported:

name - (Required) Name of MLflow experiment. It must be an absolute path within the Databricks workspace, e.g. /Users/<some-username>/my-experiment. For more information about changes to experiment naming conventions, see mlflow docs.
artifact_location - Path to dbfs:/ or s3:// artifact location of the MLflow experiment.
description - The description of the MLflow experiment.

databricks_mlflow_model

resource "databricks_mlflow_model" "test" {
  name = "My MLflow Model"
  description = "My MLflow model description"
  tags {
    key   = "key1"
    value = "value1"
  }
  tags {
    key   = "key2"
    value = "value2"
  }
}

Argument Reference

The following arguments are supported:

name - (Required) Name of MLflow model.
description - The description of the MLflow model.
tags - Tags for the MLflow model.

MLFlow experiment work

adding test for experiment

Correct case mismatch.

…orm-provider-databricks into mlflow-experiment-update

Mlflow experiment update

Included artifact_location in experiment and updated docs

* including artifact_location * removing force_new from api struct * Refactor API objects and struct names. * Refactor API objects and struct names. * Improve docs. Fix acceptance test. Co-authored-by: nsoni2 <navin_soni@intuit.com> Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com> Co-authored-by: soninavin <soni.navin@gmail.com>

refactoring api function

* Remove toAPIObject. Refactor tests. * Remove toAPIObject. Refactor tests. Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com>

removing experiment tags

nfx

don't create api package, unless it's reused or big. move api wrapper to resource file instead to keep consistency.
comments for experiment resource are the same as for model resource.

nfx · 2021-11-19T21:39:02Z

docs/index.md

@@ -48,6 +48,10 @@ Databricks SQL
 * Manage [dashboards](resources/sql_dashboard.md) and their [widgets](resources/sql_widget.md).
 * Provide [global configuration for all SQL Endpoints](docs/resources/sql_global_config.md)

+MLFlow
+* Create [MLFlow models](resources/mlflow_models.md).


rename mlflow_models.md to mlflow_model.md

also add an entry to README.md

nfx · 2021-11-19T21:40:10Z

docs/index.md

@@ -48,6 +48,10 @@ Databricks SQL
 * Manage [dashboards](resources/sql_dashboard.md) and their [widgets](resources/sql_widget.md).
 * Provide [global configuration for all SQL Endpoints](docs/resources/sql_global_config.md)

+MLFlow
+* Create [MLFlow models](resources/mlflow_models.md).
+* Create [MLFlow experiments](resources/mlflow_experiments.md).


mlflow_experiments.md to mlflow_experiment.md

nfx · 2021-11-19T21:41:03Z

docs/resources/mlflow_experiments.md

+
+The following arguments are supported:
+
+* `name` - (Required) Name of MLflow experiment. It must be an absolute path within the Databricks workspace, e.g. `/Users/<some-username>/my-experiment`. For more information about changes to experiment naming conventions, see https://docs.databricks.com/applications/mlflow/experiments.html#experiment-migration.


hide url in [..](url)

nfx · 2021-11-19T21:41:23Z

docs/resources/mlflow_experiments.md

+The following arguments are supported:
+
+* `name` - (Required) Name of MLflow experiment. It must be an absolute path within the Databricks workspace, e.g. `/Users/<some-username>/my-experiment`. For more information about changes to experiment naming conventions, see https://docs.databricks.com/applications/mlflow/experiments.html#experiment-migration.
+* `artifact_location` - Path to dbfs:/ or s3:// artificate location of the MLflow experiment.


typo: artificate

nfx · 2021-11-19T21:45:25Z

mlflow/resource_model.go

+		})
+
+	return common.Resource{
+		Create: func(ctx context.Context, data *schema.ResourceData, c *common.DatabricksClient) error {


please make sure that ResourceData is always named d - d *schema.ResourceData. it's a convention.

nfx · 2021-11-19T21:56:38Z

mlflow/api/model.go

+// MLFlowModelAPI defines the response object from the API
+type Model struct {
+	Name                 string   `json:"name"`
+	CreationTimestamp    int64    `json:"creation_timestamp,omitempty"`


add tf:"computed"

nfx · 2021-11-19T21:56:43Z

mlflow/api/model.go

+type Model struct {
+	Name                 string   `json:"name"`
+	CreationTimestamp    int64    `json:"creation_timestamp,omitempty"`
+	LastUpdatedTimestamp int64    `json:"last_updated_timestamp,omitempty"`


add tf:"computed"

nfx · 2021-11-19T21:56:47Z

mlflow/api/model.go

+	Name                 string   `json:"name"`
+	CreationTimestamp    int64    `json:"creation_timestamp,omitempty"`
+	LastUpdatedTimestamp int64    `json:"last_updated_timestamp,omitempty"`
+	UserID               string   `json:"user_id,omitempty"`


add tf:"computed"

nfx · 2021-11-19T21:56:53Z

mlflow/api/model.go

+	CreationTimestamp    int64    `json:"creation_timestamp,omitempty"`
+	LastUpdatedTimestamp int64    `json:"last_updated_timestamp,omitempty"`
+	UserID               string   `json:"user_id,omitempty"`
+	LatestVersions       []string `json:"latest_versions,omitempty"`


add tf:"computed"

nfx · 2021-11-19T21:57:31Z

mlflow/api/model.go

+
+// Read ...
+func (a ModelAPI) Read(modelName string) (*Model, error) {
+	//var d MLFLowModelAPI


remove commented irrelevant code

codecov · 2021-11-19T22:03:05Z

Codecov Report

Merging #931 (e269506) into master (5ad456b) will increase coverage by 0.02%.
The diff coverage is 87.65%.

@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
+ Coverage   85.30%   85.32%   +0.02%     
==========================================
  Files          93       95       +2     
  Lines        8582     8663      +81     
==========================================
+ Hits         7321     7392      +71     
- Misses        773      778       +5     
- Partials      488      493       +5

Impacted Files	Coverage Δ
mlflow/resource_model.go	`84.61% <84.61%> (ø)`
mlflow/resource_experiment.go	`90.00% <90.00%> (ø)`
provider/provider.go	`94.06% <100.00%> (+0.10%)`	⬆️

…ricks into mlflow

Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com>

kuritsu · 2021-11-22T20:14:44Z

@nfx Please check our new changes, trying to address all your suggestions.
Regarding the tf:"computed" suggestion, is this really needed as this is a DTO (Data Transfer Object) for only talking to the MLFlow API, but not the Terraform internal state?

nfx

Better! :) please see more comments

nfx · 2021-11-23T09:24:28Z

mlflow/resource_experiment.go

+}
+
+// ExperimentAPI ...
+type ExperimentAPI struct {


rename to ExperimentsAPI (e.g. almost all APIs are plurals of their entities)

nfx · 2021-11-23T09:26:50Z

mlflow/resource_experiment.go

+	return a.client.Post(a.context, "/mlflow/experiments/delete", d, &d)
+}
+
+///func ResourceMLFlowExperiment() {}


remove commented code

nfx · 2021-11-23T09:29:09Z

mlflow/resource_experiment.go

+				return err
+			}
+
+			data.Set("name", ad.Name)


you don't need to do d.Set(...), because it's already handled by StructToData

nfx · 2021-11-23T09:30:22Z

mlflow/resource_experiment.go

+			data.SetId(ad.ExperimentId)
+			return nil
+		},
+		Read: func(ctx context.Context, data *schema.ResourceData, c *common.DatabricksClient) error {


please follow conventions of this software project: replace data *schema.ResourceData with d *schema.ResourceData. anyone working on other resources would immediately be confused with this argument naming.

nfx · 2021-11-23T09:30:38Z

mlflow/resource_experiment.go

+			data.SetId(ad.ExperimentId)
+			return nil
+		},
+		Update: func(ctx context.Context, data *schema.ResourceData, c *common.DatabricksClient) error {


data -> d

nfx · 2021-11-23T09:57:02Z

mlflow/resource_model.go

+}
+
+// Update ...
+func (a ModelAPI) Update(d *ModelDto) error {


variable naming

nfx · 2021-11-23T09:57:43Z

mlflow/resource_model.go

+
+	return common.Resource{
+		Create: func(ctx context.Context, data *schema.ResourceData, c *common.DatabricksClient) error {
+			var ad ModelDto


ad has no semantic meaning, while model does

nfx · 2021-11-23T09:58:10Z

mlflow/resource_model.go

+			return nil
+		},
+		Read: func(ctx context.Context, data *schema.ResourceData, c *common.DatabricksClient) error {
+			var d Model


remove this line

nfx · 2021-11-23T09:58:19Z

mlflow/resource_model.go

+		},
+		Read: func(ctx context.Context, data *schema.ResourceData, c *common.DatabricksClient) error {
+			var d Model
+			ad, err := NewModelAPI(ctx, c).Read(data.Id())


model, err := ...

nfx · 2021-11-23T09:58:49Z

mlflow/resource_model.go

+				return err
+			}
+
+			if err := common.StructToData(d, s, data); err != nil {


rewrite it with return common.StructToData(model, s, d)

nfx · 2021-11-23T10:00:07Z

@kuritsu @jensendw added more comments.

Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com>

kuritsu · 2021-11-23T20:20:24Z

@nfx Round 2 of suggestions addressed. Please check.

nfx

@kuritsu We're almost there! Make Delete() method accept a string argument and remove copy-pasted comment.

And merge in/rebase onto master branch.

And then I'm glad to squash and merge this PR

nfx · 2021-11-23T23:03:43Z

mlflow/resource_experiment.go

+		},
+		Delete: func(ctx context.Context, d *schema.ResourceData, c *common.DatabricksClient) error {
+			e := Experiment{ExperimentId: d.Id()}
+			return NewExperimentsAPI(ctx, c).Delete(&e)


Can you refactor Delete method to accept just a string id? E.g. we don't need any other fields than ID to delete an entity. And within that method you'll probably use that struct, but outer interface would be simple and consistent with the rest of resources.

Consistency is key, as eventually we'll try to migrate to go1.17 generics and Delete is one of the most similar methods everywhere.

nfx · 2021-11-23T23:05:39Z

mlflow/resource_model.go

+	Description string    `json:"description,omitempty"`
+}
+
+// ResourceDashboard ...


Maybe remove this copy-pasted comment?...

nfx

Lgtm

…sources (databricks#931) Provides basic support for managing MLflow models and experiments, should hopefully address databricks#861 as well. # databricks_mlflow_experiment ```hcl resource "databricks_mlflow_experiment" "test" { name = "/Users/myuserid/my-experiment" artifact_location = "dbfs:/tmp/my-experiment" description = "My MLflow experiment description" } ``` ## Argument Reference The following arguments are supported: * `name` - (Required) Name of MLflow experiment. It must be an absolute path within the Databricks workspace, e.g. `/Users/<some-username>/my-experiment`. For more information about changes to experiment naming conventions, see [mlflow docs](https://docs.databricks.com/applications/mlflow/experiments.html#experiment-migration). * `artifact_location` - Path to dbfs:/ or s3:// artifact location of the MLflow experiment. * `description` - The description of the MLflow experiment. # databricks_mlflow_model ```hcl resource "databricks_mlflow_model" "test" { name = "My MLflow Model" description = "My MLflow model description" tags { key = "key1" value = "value1" } tags { key = "key2" value = "value2" } } ``` ## Argument Reference The following arguments are supported: * `name` - (Required) Name of MLflow model. * `description` - The description of the MLflow model. * `tags` - Tags for the MLflow model. --- Co-authored-by: nsoni2 <navin_soni@intuit.com> Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com> Co-authored-by: Daniel Jensen <daniel_jensen@intuit.com> Co-authored-by: Serge Smertin <259697+nfx@users.noreply.github.com>

Daniel Jensen and others added 28 commits November 15, 2021 12:59

Base mlflow model work

a8df71a

fix api struct naming

0705ece

push changes

6a2a2d4

cleanup and better tests

c81a4b2

MLFlow experiment work

c8c9e32

Add update + tests

47a7e8a

Merge pull request #1 from jensendw/mlflow-snavin

912f441

MLFlow experiment work

adding test for experiment

6cbcf84

Merge pull request #3 from jensendw/mlflow-snavin

fee6b43

adding test for experiment

Add update implementation.

ccb3046

Correct case mismatch.

Added models/experiments docs (#2)

e704148

Add update implementation.

c566b7a

Correct case mismatch.

Merge branch 'mlflow-experiment-update' of github.com:jensendw/terraf…

8e07f2c

…orm-provider-databricks into mlflow-experiment-update

Fix unneeded line change.

d35ee95

Merge pull request #4 from jensendw/mlflow-experiment-update

43aefe8

Mlflow experiment update

Add acceptance tests (#5)

63be400

including artifact_location

aefeac3

removing force_new from api struct

d29035d

Merge pull request #6 from jensendw/mlflow-snavin

ab7536a

Included artifact_location in experiment and updated docs

refactoring api function

b891f50

Merge pull request #8 from jensendw/mlflow-snavin

710cf07

refactoring api function

Mlflow refactor2 (#9)

92d7167

* Remove toAPIObject. Refactor tests. * Remove toAPIObject. Refactor tests. Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com>

removing experiment tags

dbd81f6

fix acceptance tests

f04636d

Merge pull request #10 from jensendw/mlflow-snavin

f77ca5e

removing experiment tags

Fix acceptance tests (#11)

77a1a4e

Merge branch 'master' into mlflow

d6458cd

nfx self-requested a review November 19, 2021 21:38

nfx requested changes Nov 19, 2021

View reviewed changes

Daniel Jensen and others added 3 commits November 19, 2021 14:28

fixes

22f1588

Merge branch 'mlflow' of github.com:jensendw/terraform-provider-datab…

469d907

…ricks into mlflow

Refactor (remove api pkg) for PR. (#12)

801d106

Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com>

nfx requested changes Nov 23, 2021

View reviewed changes

Fix PR suggestions pt 2. (#13)

8ccf62e

Co-authored-by: Jeffrey Alvarez <Jeffrey_Alvarez@intuit.com>

nfx requested changes Nov 23, 2021

View reviewed changes

Fix last round of suggestions.

df7a6af

nfx approved these changes Nov 24, 2021

View reviewed changes

Merge branch 'master' into mlflow

e269506

nfx changed the title ~~MLflow support~~ Added databricks_mlflow_model and databricks_mlflow_experiment Nov 24, 2021

nfx changed the title ~~Added databricks_mlflow_model and databricks_mlflow_experiment~~ Added databricks_mlflow_model and databricks_mlflow_experiment resources Nov 24, 2021

nfx merged commit a796ffa into databricks:master Nov 24, 2021

nfx added this to the v0.4.0 milestone Nov 24, 2021


		The following arguments are supported:

		* `name` - (Required) Name of MLflow experiment. It must be an absolute path within the Databricks workspace, e.g. `/Users/<some-username>/my-experiment`. For more information about changes to experiment naming conventions, see https://docs.databricks.com/applications/mlflow/experiments.html#experiment-migration.

Added databricks_mlflow_model and databricks_mlflow_experiment resources #931

Added databricks_mlflow_model and databricks_mlflow_experiment resources #931

Conversation

jensendw commented Nov 19, 2021 • edited by nfx Loading

databricks_mlflow_experiment

Argument Reference

databricks_mlflow_model

Argument Reference

nfx 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 19, 2021 • edited Loading

Codecov Report

kuritsu commented Nov 22, 2021

nfx 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

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

nfx commented Nov 23, 2021

kuritsu commented Nov 23, 2021 • edited Loading

nfx 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

nfx left a comment

Choose a reason for hiding this comment

Added `databricks_mlflow_model` and `databricks_mlflow_experiment` resources #931

Added `databricks_mlflow_model` and `databricks_mlflow_experiment` resources #931

jensendw commented Nov 19, 2021 •

edited by nfx

Loading

codecov bot commented Nov 19, 2021 •

edited

Loading

kuritsu commented Nov 23, 2021 •

edited

Loading