-
Notifications
You must be signed in to change notification settings - Fork 151
Add MLflow SDK support for Model Registry as a Tracking Store, fixes #1225 #1337
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
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
The long commit history should become much shorter once #1318 is merged in main. |
|
To review this PR, focus on the |
60c43ad to
60ad248
Compare
|
Re-based branch on main on top of mlmd removal. Ready for review. |
clients/modelregistry_plugin/modelregistry_plugin/api_client.py
Outdated
Show resolved
Hide resolved
jonburdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of my comments are just suggestions and could be done later. Overall lgtm
jonburdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
jonburdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There are some changes I would like to see us make in the future, but I think this looks good for now. If you were going to make one more change, I would suggest replacing builtins.open with model_registry_mlflow.auth.open and do same for the other patches. Even this should be okay for now though.
clients/model-registry-mlflow/src/model_registry_mlflow/auth.py
Outdated
Show resolved
Hide resolved
clients/model-registry-mlflow/src/model_registry_mlflow/auth.py
Outdated
Show resolved
Hide resolved
clients/model-registry-mlflow/src/model_registry_mlflow/auth.py
Outdated
Show resolved
Hide resolved
|
@jonburdo thanks for the detailed review. I have addressed all the comments. On using mtime for token file caching, I wrote a local benchmark script (using cursor) and it found that with a 95%+ hit rate, using mtime to avoid reading the file would be 4-5X faster. |
jonburdo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks for all the hard work on this! Looks great :)
Signed-off-by: Dhiraj Bokde <[email protected]>
…tests for mlflow plugin Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
… and runs search Signed-off-by: Dhiraj Bokde <[email protected]>
…Store class, add tests for new methods, replace print statements in tests Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Co-authored-by: Paul Boyd <[email protected]> Signed-off-by: Dhiraj Bokde <[email protected]>
Co-authored-by: Paul Boyd <[email protected]> Signed-off-by: Dhiraj Bokde <[email protected]>
Co-authored-by: Paul Boyd <[email protected]> Signed-off-by: Dhiraj Bokde <[email protected]>
…del_registry_mlflow to match pypi and python pkg naming conventions, removed redunant code in tests, general code cleanup Signed-off-by: Dhiraj Bokde <[email protected]>
…ild system Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
f75fd6d to
465cbce
Compare
|
New changes are detected. LGTM label has been removed. |
|
@tarilabs @Al-Pragliola please add an approval so we can close this. |
Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Dhiraj Bokde <[email protected]>
|
@dhirajsb please hold on this with merging; we did not have any conversations in the community on the direction; we need to establish what we are signing up here. Supporting MLflow this way is big commitment, especially given experiment tracking KEP was closed without resolution in the KFP recently. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it. |
Description
MLflow SDK supports integrations with non-mlflow Tracking stores by implementing a tracking.AbstractStore API.
This PR implements that API to allow MLflow SDK to directly call Model Registry APIs for storing Experiment and Run metadata.
See the feature request issue #1225 for motivation, design and implementation details.
This PR is based on the Experiment Tracking PR #1318 and must be merged after that PR.
Fixes #1225.
How Has This Been Tested?
Extensive unit and e2e tests have been provided. A local e2e test is provide to test against the go REST server, as well as an e2e test to run against a deployed model registry to verify RBAC support.
Merge criteria:
DCOcheck)ok-to-testhas been added to the PR.If you have UI changes