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

Implement gRPC GetArtifactType #26

Merged

Conversation

tarilabs
Copy link
Member

resolves #25

Description

Implement gRPC GetArtifactType

How Has This Been Tested?

in one terminal:

make build
make run/server

in another terminal:

python test/python/test_mlmetadata.py 

then running the following python script:

from grpc import insecure_channel
from ml_metadata.proto import metadata_store_service_pb2
from ml_metadata.proto import metadata_store_service_pb2_grpc

def main():
    channel = insecure_channel('localhost:8080')
    store = metadata_store_service_pb2_grpc.MetadataStoreServiceStub(channel)

    request = metadata_store_service_pb2.GetArtifactTypeRequest()
    request.type_name = "SavedModel"

    response = store.GetArtifactType(request)
    print(response)

if __name__ == '__main__':
    main()

Outputs as expected:

$ python test/python/test_getartifacttype.py 
artifact_type {
  id: 2
  name: "SavedModel"
}

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or 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

Comment on lines +273 to +271
ctx, dbConn := Begin(ctx, g.dbConnection)
defer handleTransaction(ctx, &err)
Copy link
Member Author

Choose a reason for hiding this comment

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

I preferred to take "same approach" and place this on a tx context too, even if this is only a read, for consistency.

@tarilabs tarilabs force-pushed the tarilabs-20230925-GetArtifactType branch from e12affc to def917b Compare September 25, 2023 06:53
Copy link
Contributor

@dhirajsb dhirajsb left a comment

Choose a reason for hiding this comment

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

Please add the test client code to the current prototype integration test test/python/test_mlmetadata.py script. We can re-use it in a later testframework.

lampajr
lampajr previously approved these changes Sep 25, 2023
Copy link
Contributor

@dhirajsb dhirajsb left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@isinyaaa isinyaaa left a comment

Choose a reason for hiding this comment

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

lgtm!

@tarilabs
Copy link
Member Author

no vetos, no missing, all approval; proceeding to merge.

@tarilabs tarilabs merged commit 7293d51 into opendatahub-io:main Sep 27, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Implement gRPC GetArtifactType
4 participants