Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added databricks_model_serving resource #2054

Merged
merged 18 commits into from
Mar 8, 2023
Merged

Conversation

arpitjasa-db
Copy link
Contributor

@arpitjasa-db arpitjasa-db commented Mar 1, 2023

This PR adds Model Serving Endpoints as a Terraform resource to the Databricks Terraform Provider, addressing #2040.

@arpitjasa-db arpitjasa-db requested review from nfx and vladimirk-db March 1, 2023 20:02
@arpitjasa-db arpitjasa-db force-pushed the endpoints branch 3 times, most recently from 20412f7 to f592dcb Compare March 1, 2023 22:08
docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
common/reflect_resource.go Outdated Show resolved Hide resolved
docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint.go Outdated Show resolved Hide resolved
docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provider/provider.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint_test.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint_test.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint_test.go Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint_test.go Outdated Show resolved Hide resolved
docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
Copy link

@vladimirk-db vladimirk-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please provide a PR description? It would make it easier to review for people not familiar with the codebase already.

docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
docs/resources/serving_endpoint.md Outdated Show resolved Hide resolved
serving-endpoints/resource_serving_endpoint.go Outdated Show resolved Hide resolved
common/reflect_resource.go Outdated Show resolved Hide resolved
Copy link

@ankit-db ankit-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arpitjasa-db mind holding merge on this? We have to change the name of the product, and the link to the docs will change. Not sure right now what the link name will be, so give us till the end of the day if that's ok?

@arpitjasa-db
Copy link
Contributor Author

@arpitjasa-db mind holding merge on this? We have to change the name of the product, and the link to the docs will change. Not sure right now what the link name will be, so give us till the end of the day if that's ok?

Yeah no worries, I'll update the PR with the new name once you have it.

@arpitjasa-db arpitjasa-db requested review from nfx and alexott March 3, 2023 20:13
@arpitjasa-db
Copy link
Contributor Author

@ankit-db I saw the email from Craig that the name is changing from Serverless Real-time Inference to Model Serving, so I updated the PR accordingly. Please take a look at the PR along with any other ML Inference folks (@sueann or @mehtayogita ) to make sure the rest of the naming and the docs line up with what is expected.

docs/resources/mlflow_model_serving.md Outdated Show resolved Hide resolved
docs/resources/mlflow_model_serving.md Outdated Show resolved Hide resolved
@arpitjasa-db
Copy link
Contributor Author

@nfx @alexott Some of my unit tests seem to be failing with this cryptic error message

testing.go:426: 
        	Error Trace:	/home/runner/work/terraform-provider-databricks/terraform-provider-databricks/qa/testing.go:426
        	            				/opt/hostedtoolcache/go/1.19.6/x64/src/net/http/server.go:2109
        	            				/opt/hostedtoolcache/go/1.19.6/x64/src/net/http/server.go:2947
        	            				/opt/hostedtoolcache/go/1.19.6/x64/src/net/http/server.go:1991
        	            				/opt/hostedtoolcache/go/1.19.6/x64/src/runtime/asm_amd64.s:1594
        	Error:      	Input ('') needs to be valid json.
        	            	JSON parsing error: 'unexpected end of JSON input'
        	Test:       	TestModelServingRead
        	Messages:   	json strings do not match

Do you know what could be causing this? Stepped through the tests in a debugger and it's weird this error seems to show up after the test passes the assertions.

@nkvuong
Copy link
Contributor

nkvuong commented Mar 6, 2023

@arpitjasa-db basically the json parsing error comes from https://github.com/databricks/terraform-provider-databricks/blob/master/qa/testing.go#L426, where ExpectedRequest is an invalid json string.

I then notice this definition in the sdk -
https://github.com/databricks/databricks-sdk-go/blob/main/service/endpoints/model.go#L166

Name string `json:"-" url:"-"`

This might be causing the problem with json marshal

Copy link
Contributor

@nfx nfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, pending the successful acceptance test results

client.create_model_version(name="%[1]s-model", source=source, run_id=run1_id)
while client.get_model_version(name="%[1]s-model", version="1").getStatus() != ModelRegistry.ModelVersionStatus.READY:
time.sleep(10)
while client.get_model_version(name="%[1]s-model", version="2").getStatus() != ModelRegistry.ModelVersionStatus.READY:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder how can be encode this in our Python SDK :)

@nfx nfx linked an issue Mar 8, 2023 that may be closed by this pull request
@arpitjasa-db
Copy link
Contributor Author

Integration/Acceptance test passes with new version of Go SDK!
Screen Shot 2023-03-08 at 9 48 37 AM

Copy link

@ankit-db ankit-db left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit

docs/resources/mlflow_model.md Outdated Show resolved Hide resolved
Copy link
Contributor

@nkvuong nkvuong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggested changes in docs

docs/resources/model_serving.md Outdated Show resolved Hide resolved
docs/resources/model_serving.md Outdated Show resolved Hide resolved
docs/resources/model_serving.md Outdated Show resolved Hide resolved
@nfx nfx merged commit c23c570 into databricks:master Mar 8, 2023
@arpitjasa-db arpitjasa-db deleted the endpoints branch March 8, 2023 18:29
nfx added a commit that referenced this pull request Mar 9, 2023
* Added databricks_model_serving resource (#2054).
* Added Unity Catalog on GCP support (#2000).
* Deprecate schedule block in databricks_sql_query resource (#2078).
* Fixed InitScripts Type to work with GCS and ABFS in databricks_cluster resource (#2067).
* Added more testing for databricks_tables data source (#2075).
* Added more testing for databricks_schemas data source (#2074).
* Migrate databricks_node_type data source to Go SDK (#2070).
* Migrate databricks_schemas data to Go SDK (#2065).
* Migrate databricks_tables data source to SDK (#2068).
* Migrate databricks_views data source to Go SDK (#2073).
* Migrated databricks_git_credential to Go SDK (#2069).
* Migrated databricks_shares data source to Go SDK (#2072).

Updated dependency versions:

* Bump github.com/databricks/databricks-sdk-go from v0.3.3 to v0.4.0 (#2086).
* Bump golang.org/x/mod from 0.8.0 to 0.9.0 (#2076).
@nfx nfx mentioned this pull request Mar 9, 2023
nfx added a commit that referenced this pull request Mar 9, 2023
* Added databricks_model_serving resource (#2054).
* Added Unity Catalog on GCP support (#2000).
* Deprecate schedule block in databricks_sql_query resource (#2078).
* Fixed InitScripts Type to work with GCS and ABFS in databricks_cluster resource (#2067).
* Added more testing for databricks_tables data source (#2075).
* Added more testing for databricks_schemas data source (#2074).
* Migrate databricks_node_type data source to Go SDK (#2070).
* Migrate databricks_schemas data to Go SDK (#2065).
* Migrate databricks_tables data source to SDK (#2068).
* Migrate databricks_views data source to Go SDK (#2073).
* Migrated databricks_git_credential to Go SDK (#2069).
* Migrated databricks_shares data source to Go SDK (#2072).

Updated dependency versions:

* Bump github.com/databricks/databricks-sdk-go from v0.3.3 to v0.4.0 (#2086).
* Bump golang.org/x/mod from 0.8.0 to 0.9.0 (#2076).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add resource for Serverless Real-Time Inference endpoints
9 participants