Add support for models in direct deployment#3625
Conversation
14 failing tests:
|
denik
left a comment
There was a problem hiding this comment.
Looks good, minor suggestion in testserver.
|
|
||
| return Response{ | ||
| Body: ml.GetModelResponse{ | ||
| RegisteredModelDatabricks: &ml.ModelDatabricks{ |
There was a problem hiding this comment.
Would it make sense to store ml.ModelDatabricks in the state directly and then use MapGet() instead of custom method with remapping?
There was a problem hiding this comment.
Yes, but the benefit is marginal, so I'll skip it for now.
| >>> [CLI] bundle plan | ||
| update models.my_model | ||
|
|
||
| Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged |
There was a problem hiding this comment.
the comments says "this should be a no-op" but the plan shows an update.
There was a problem hiding this comment.
We show a change even though field is annotated as skip right? I observed the same behaviour in models.
There was a problem hiding this comment.
Discussed offline - TF does make a call but the call does not have "tags" in the request, so the request is essentially noop.
Direct should not make a request at all since the field is marked as 'skip', it's a bug that it does. Fixed in #3636
There was a problem hiding this comment.
The json plan makes it more clear -- the field shows up but it's marked as skip and the whole resource is marked as skip.
{
"plan": {
"resources.models.my_model": {
"action": "skip",
"changes": {
"local": {
"tags": {
"action": "skip"
}
}
}
}
}
}
Changes
This PR adds support for direct deployment for MLflow models, matching the existing Terraform behaviour today.
Why
To make direct deployments possible.
Tests
Test asserting that TF and direct behaviour match for all relevent fields.