Added databricks_registered_model resource#2771
Conversation
arpitjasa-db
requested review from
a team,
alexott,
mgyucht,
nfx,
nkvuong,
smurching and
vladimirk-db
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## master #2771 +/- ##
==========================================
- Coverage 88.21% 84.75% -3.47%
==========================================
Files 145 149 +4
Lines 11989 13050 +1061
==========================================
+ Hits 10576 11060 +484
- Misses 930 1391 +461
- Partials 483 599 +116
|
| m["name"].ForceNew = true | ||
| m["catalog_name"].ForceNew = true | ||
| m["schema_name"].ForceNew = true | ||
| m["storage_location"].ForceNew = true |
There was a problem hiding this comment.
If it's computed, we may not need ForceNew.
There was a problem hiding this comment.
Yeah, but if someone manually sets this field after it's been computed, it still tries to modify the field in-place instead of replacing the whole resource. That's why I added both attributes
nkvuong
left a comment
nkvuong
left a comment
There was a problem hiding this comment.
couple of suggested changes
smurching
left a comment
smurching
left a comment
There was a problem hiding this comment.
LGTM, thanks @arpitjasa-db !
Signed-off-by: Arpit Jasapara <[email protected]>
|
integration tests passed (failed tests are for |
tanmay-db
left a comment
tanmay-db
left a comment
There was a problem hiding this comment.
LGTM, comments have also been addressed.
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
Non-blocking comment.
Note that the FullName field is a read-only field only present on the create response and as parameter to the other API calls, so it doesn't show up in the resource schema (only as ID).
I looked at the schema diff and this is the result:
{
"block": {
"attributes": {
"catalog_name": {
"description_kind": "plain",
"required": true,
"type": "string"
},
"comment": {
"description_kind": "plain",
"optional": true,
"type": "string"
},
"id": {
"computed": true,
"description_kind": "plain",
"optional": true,
"type": "string"
},
"name": {
"description_kind": "plain",
"required": true,
"type": "string"
},
"schema_name": {
"description_kind": "plain",
"required": true,
"type": "string"
},
"storage_location": {
"computed": true,
"description_kind": "plain",
"optional": true,
"type": "string"
}
},
"description_kind": "plain"
},
"version": 0
}
docs/resources/mlflow_model.md
Outdated
|
|
||
| This resource allows you to create [MLflow models](https://docs.databricks.com/applications/mlflow/models.html) in Databricks. | ||
|
|
||
| **Note** This documentation covers the Workspace Model Registry. Databricks recommends using [Models in Unity Catalog](registered_model.md). Models in Unity Catalog provides centralized model governance, cross-workspace access, lineage, and deployment. Workspace Model Registry will be deprecated in the future. |
There was a problem hiding this comment.
The deprecation notice can be left out if we don't know when this will happen (or ever).
mgyucht
left a comment
mgyucht
left a comment
There was a problem hiding this comment.
To unblock the release of this resource, I'll approve. I think we can expand the set of fields accepted by the input and those provided for users to read after creation as a follow-up.
|
|
||
| func ResourceRegisteredModel() *schema.Resource { | ||
| s := common.StructToSchema( | ||
| catalog.CreateRegisteredModelRequest{}, |
There was a problem hiding this comment.
This will work, but it will only include attributes defined in the create request. If the other fields in RegisteredModelInfo are of use to users, we may want to use catalog.RegisteredModelInfo and set Computed: true on all other fields. Note that if we do this, you'll need to set Optional: false on all of those fields, as this helper method infers that these fields are optional (which in this case is incorrect: read only fields in TF providers are Computed: true and Optional: false). Since adding read-only values to a TF resource is non-breaking, we can revisit this later.
There was a problem hiding this comment.
Another note: owner is not part of the create structure but is part of the update, so users won't be able to set an owner for the model. We can revisit this later if need be.
| * `name` - (Required) The name of the registered model. | ||
| * `catalog_name` - (Required) The name of the catalog where the schema and the registered model reside. | ||
| * `schema_name` - (Required) The name of the schema where the registered model resides. | ||
| * `comment` - The comment attached to the registered model. |
There was a problem hiding this comment.
Do you not want to document storage_location? Can users set this explicitly?
There was a problem hiding this comment.
For now, I will merge as is, and we can add more documentation in a follow-up.
## Changes <!-- Summary of your changes that are easy to understand --> Add UC Registered Models support to Databricks Asset Bundles as new resource `registered_model`. Also added UC Permission support via new resource `grant`. ## Tests <!-- How is this tested? --> Tested via unit tests and manual testing with [example PR](https://github.com/databricks/bundle-examples-internal/pull/80) and [custom Terraform provider](databricks/terraform-provider-databricks#2771). <img width="698" alt="Screenshot 2023-10-08 at 4 57 23 PM" src="https://github.com/databricks/cli/assets/87999496/bcf605a9-7894-443b-865a-f7e240037815"> <img width="1109" alt="Screenshot 2023-10-08 at 4 56 47 PM" src="https://github.com/databricks/cli/assets/87999496/e4d6e424-cd70-4809-8843-6939ed2e172f"> <img width="1091" alt="Screenshot 2023-10-08 at 4 56 57 PM" src="https://github.com/databricks/cli/assets/87999496/88ebaabb-67db-4a11-88a5-df087e2e41c0"> --------- Signed-off-by: Arpit Jasapara <[email protected]> Co-authored-by: Andrew Nester <[email protected]> Co-authored-by: Pieter Noordhuis <[email protected]>
10 participants
Changes
This PR adds Models in UC as a Terraform resource to the Databricks Terraform Provider, named in a future-proof way and to disambiguate from the soon-to-be deprecated workspace MLflow models.
Tests
make testrun locallydocs/folderinternal/acceptanceAlso tested manually using the
make install-version of the Terraform provider that includes these changes and the Dogfood staging workspace.