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

[Fix] Fix model serving resource #3690

Merged
merged 9 commits into from
Jul 19, 2024
Merged

Conversation

arpitjasa-db
Copy link
Contributor

@arpitjasa-db arpitjasa-db commented Jun 22, 2024

Changes

This PR fixes the provisioned throughput (PTP), external model (EM), and inference tables journeys via Terraform.

We also remove invalid validation checks and planned invalid checks. We do not add new checks since the source-of-truth should be left to the API backend, which will throw an informative error as necessary when invalid parameters are provided.

Should resolve #3676.

Tests

We add new acceptance tests to cover the different endpoint types that can be created.

  • make test run locally
  • relevant change in docs/ folder
  • covered with integration tests in internal/acceptance
  • relevant acceptance tests are passing
  • using Go SDK

@arpitjasa-db arpitjasa-db requested review from a team as code owners June 22, 2024 01:54
@arpitjasa-db arpitjasa-db requested review from mgyucht, alexott, tanmay-db and hectorcast-db and removed request for a team June 22, 2024 01:54
@arpitjasa-db
Copy link
Contributor Author

@mgyucht @alexott @tanmay-db @hectorcast-db I don't have access to the integration tests, so can someone let me know if they're passing or not?

Locally when I make the provider, the resources seem to be working fine, except for the external_model endpoint, which seems to get created in Databricks just fine, but Terraform never seems to realize it and waits for its "creation" indefinitely until it times out. Would appreciate some help looking into this as well thanks!

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@66a881f). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3690   +/-   ##
=======================================
  Coverage        ?   81.59%           
=======================================
  Files           ?      196           
  Lines           ?    19699           
  Branches        ?        0           
=======================================
  Hits            ?    16074           
  Misses          ?     2668           
  Partials        ?      957           
Files Coverage Δ
serving/resource_model_serving.go 79.72% <100.00%> (ø)

@arpitjasa-db
Copy link
Contributor Author

arpitjasa-db commented Jun 22, 2024

@mgyucht @alexott @tanmay-db @hectorcast-db I don't have access to the integration tests, so can someone let me know if they're passing or not?

Locally when I make the provider, the resources seem to be working fine, except for the external_model endpoint, which seems to get created in Databricks just fine, but Terraform never seems to realize it and waits for its "creation" indefinitely until it times out. Would appreciate some help looking into this as well thanks!

@alexott I have a theory the EM functionality broke with this commit, since the functionality has been broken since version 1.39.0, and the only other change in that version was the commit that @840 wrote to support EM in the first place (and I imagine that @840 validated it was working).

@edwardfeng-db
Copy link
Contributor

Triggered integration tests for this

@edwardfeng-db edwardfeng-db self-requested a review June 25, 2024 13:50
@edwardfeng-db
Copy link
Contributor

Synced offline about integration test failures

@arpitjasa-db
Copy link
Contributor Author

arpitjasa-db commented Jun 28, 2024

@edwardfeng-db looks like the PTP integration test is now passing. The EM test is still failing but that's because of the underlying Go SDK methods for create/wait that are causing this issue where the EM endpoint is being created, but the status is not being fetched correctly by the wait method. How can we fix that? @alexott @mgyucht @hectorcast-db @tanmay-db ?

@hectorcast-db hectorcast-db changed the title Fix model serving resource [Fix] Fix model serving resource Jul 11, 2024
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Two small questions I'd like to confirm before moving forward, otherwise LGTM!

serving/resource_model_serving.go Show resolved Hide resolved
serving/resource_model_serving.go Outdated Show resolved Hide resolved
@arpitjasa-db arpitjasa-db requested a review from mgyucht July 16, 2024 08:25
@tanmay-db tanmay-db mentioned this pull request Jul 18, 2024
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

LGTM based on our discussion offline. Later, once model serving endpoints support updating the auto capture configuration without the config needing to be disabled, we can remove ForceNew from those fields and allow them to be updated.

@mgyucht mgyucht added this pull request to the merge queue Jul 19, 2024
Merged via the queue into databricks:main with commit ab4fd40 Jul 19, 2024
6 checks passed
github-merge-queue bot pushed a commit to databricks/cli that referenced this pull request Jul 23, 2024
## Changes

This includes a fix for model serving endpoints.

See
databricks/terraform-provider-databricks#3690.

## Tests

n/a
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.

[ISSUE] Issue with databricks_model_serving resource
6 participants