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.

Sign up for GitHub

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

Jump to bottom

Migrate databricks_mlflow_model to go sdk #2257

Merged
merged 19 commits into from
Aug 11, 2023
Merged

Migrate databricks_mlflow_model to go sdk #2257

merged 19 commits into from
Aug 11, 2023

Conversation

michael-berk
Copy link
Contributor

@michael-berk michael-berk commented May 2, 2023 •
edited by alexott
Loading

Changes

Migrate MLflow model to Go SDK and update testing file.

Tests

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

@michael-berk michael-berk requested review from nfx and alexott May 2, 2023 16:52
@michael-berk michael-berk mentioned this pull request May 2, 2023
Closed
5 tasks
@codecov-commenter
Copy link

codecov-commenter commented May 2, 2023 •
edited
Loading

Codecov Report

Merging #2257 (76bc919) into master (8f50ea1) will decrease coverage by 0.07%.
The diff coverage is 83.33%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2257      +/-   ##
==========================================
- Coverage   88.16%   88.09%   -0.07%     
==========================================
  Files         145      145              
  Lines       12094    12108      +14     
==========================================
+ Hits        10663    10667       +4     
- Misses        939      944       +5     
- Partials      492      497       +5
Files Changed Coverage Δ
mlflow/resource_mlflow_model.go 81.13% <83.33%> (-18.87%) ⬇️

nfx
nfx requested changes May 5, 2023
View reviewed changes
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.

please address the comments from the previous PR review

@michael-berk
Copy link
Contributor Author

@nfx I think this is ready to merge - all comments from the prior PR are addressed

nfx
nfx requested changes May 9, 2023
View reviewed changes
nfx
nfx requested changes May 9, 2023
View reviewed changes
@michael-berk michael-berk requested a review from nfx May 15, 2023 14:13
nfx
nfx previously requested changes May 15, 2023
View reviewed changes
@michael-berk
Copy link
Contributor Author

Waiting on go sdk fix. Currently the ml model get request returns a nil Registered model.

@michael-berk
Copy link
Contributor Author

@nfx thanks for the fix. Acceptance and unit tests are passing.

@michael-berk michael-berk requested a review from nfx July 3, 2023 23:09
@michael-berk michael-berk requested review from a team as code owners July 25, 2023 00:58
@michael-berk michael-berk self-assigned this Jul 25, 2023
@michael-berk
Copy link
Contributor Author

@alexott @nfx this should be good to merge.

alexott
alexott approved these changes Jul 29, 2023
View reviewed changes
Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm

@mgyucht mgyucht self-requested a review August 10, 2023 04:09
mgyucht
mgyucht reviewed Aug 10, 2023
View reviewed changes
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.

Thanks for contributing this change! Moving to the Go SDK is a thankless task. Two small nits, otherwise we can get this into the next release!

@michael-berk michael-berk requested a review from mgyucht August 10, 2023 19:28
@mgyucht mgyucht dismissed nfx’s stale review August 11, 2023 07:33

Main comments have been addressed now that Go SDK has been updated.

@mgyucht mgyucht added this pull request to the merge queue Aug 11, 2023
Merged via the queue into databricks:master with commit 84696ff Aug 11, 2023
@tanmay-db tanmay-db mentioned this pull request Aug 25, 2023
Merged
5 tasks
alexott added a commit that referenced this pull request Sep 27, 2023
@alexott
Fix missing registered_model_id attribute in `databricks_mlflow_mod…
ab6ee5e 
…el` resource

This attribute was lost during refactoring in #2257, and it broke assignment of
permissions.  Also expanded corresponding integration test to have check for that

This fixes #2730
@alexott alexott mentioned this pull request Sep 27, 2023
Merged
5 tasks
github-merge-queue bot pushed a commit that referenced this pull request Sep 27, 2023
@alexott
Fix missing registered_model_id attribute in `databricks_mlflow_mod…
81293ea 
…el` resource (#2732)

This attribute was lost during refactoring in #2257, and it broke assignment of
permissions.  Also expanded corresponding integration test to have check for that

This fixes #2730
nkvuong pushed a commit that referenced this pull request Oct 3, 2023
@alexott @nkvuong
Fix missing registered_model_id attribute in `databricks_mlflow_mod…
63f8fdc 
…el` resource (#2732)

This attribute was lost during refactoring in #2257, and it broke assignment of
permissions.  Also expanded corresponding integration test to have check for that

This fixes #2730
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexott alexott alexott approved these changes

@mgyucht mgyucht mgyucht approved these changes

@nfx nfx Awaiting requested review from nfx

Assignees

@michael-berk michael-berk

Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michael-berk @codecov-commenter @alexott @nfx @mgyucht