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

feat(proxy): introduce datastore abstraction #425

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Al-Pragliola
Copy link
Contributor

@Al-Pragliola Al-Pragliola commented Sep 24, 2024

Description

As proposed in issue #398 , I have introduced an abstraction to the underlying metadata storage service and changed the reference to the old --mlmd arguments, the next logical step should be to move the pkg/core to internal.

NOTE: This is a breaking change, the new manifests need the new model-registry image to work or else it wont recognize the new arguments --datastore-hostname, --datastore-port, --datastore-type

UPDATE: Old arguments --mlmd-hostname --mlmd-port have been reintroduced in order to prevent breaking changes in the manifests, but I have marked them as deprecated using cobra. This will show the following warning when an user run the proxy with them:

image

How Has This Been Tested?

  1. Tested with make test-nocache

  2. Tested the python client with make test-e2e

  3. Tested on a local kind cluster

kind create cluster

IMG_REGISTRY=localhost:{kind-registry-port} IMG_ORG=model-registry IMG_REPO=model-registry IMG_VERSION=local-1.0 make image/build

docker push localhost:{kind-registry-port}/model-registry/model-registry:local-1.0 (if needed --tls-verify=false)

kubectl create namespace kubeflow

kubectl apply -k "./manifests/kustomize/overlays/db"

kubectl set image -n kubeflow deployments/model-registry-deployment rest-container=localhost:{kind-registry-port}/model-registry/model-registry:local-1.0

kubectl port-forward svc/model-registry-service -n kubeflow 8081:8080 &

curl --request POST   --url http://localhost:8081/api/model_registry/v1alpha3/registered_models   --header 'content-type: application/json'   --data '{
  "name": "test",
  "description": "test"
}'

curl http://localhost:8081/api/model_registry/v1alpha3/registered_models

expected result:

{"items":[{"createTimeSinceEpoch":"1727181207919","customProperties":{},"description":"test","id":"1","lastUpdateTimeSinceEpoch":"1727181207919","name":"test"}],"nextPageToken":"","pageSize":0,"size":1}

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)

  • The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.

  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).

  • The developer has manually tested the changes and verified that the changes work.

  • Code changes follow the kubeflow contribution guidelines.

Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

thank you @Al-Pragliola especially for as well 15e3ec2

can you also reflect any changes required in the https://github.com/kubeflow/model-registry/blob/main/docs/mr_go_library.md dev doc, please?

overall
/lgtm

@Al-Pragliola
Copy link
Contributor Author

thank you @Al-Pragliola especially for as well 15e3ec2

can you also reflect any changes required in the https://github.com/kubeflow/model-registry/blob/main/docs/mr_go_library.md dev doc, please?

overall /lgtm

At the moment there are no changes to clients using pkg/core, the abstraction is for internal use only.

Copy link
Member

@tarilabs tarilabs 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 the clarification and the refactor for this factory @Al-Pragliola

overall
/lgtm

and as mentioned I'd
/hold
this until we have at least dry-run the release process to implement pinning so we efficiently update the manifests accordingly to KF/manifests

MLMDPort: 9090,
DatastoreHostname: "localhost",
DatastorePort: 9090,
DatastoreType: "mlmd",
Copy link
Contributor

Choose a reason for hiding this comment

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

should we have an enum for this?

Copy link

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.

@Al-Pragliola Al-Pragliola force-pushed the Al-Pragliola/introduce-datastore branch from 15e3ec2 to f391159 Compare December 25, 2024 18:18
@google-oss-prow google-oss-prow bot removed the lgtm label Dec 25, 2024
Copy link

New changes are detected. LGTM label has been removed.

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from tarilabs. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants