Update Python SDK to use MLMD gRPC service #156

zhenghuiwang · 2019-11-04T23:21:25Z

What this PR does / why we need it:
Related to #155

This PR updates the python SDK.

New Store class is introduced for connecting to MLMD gRPC service, which returns ml_metadata.metadata_store.MetadataStore.
Old backend_url option for connecting to HTTP service is deprecated.
Change E2E test to test python SDK with gRPC service.

It is a breaking change, since the way to connect backend service changes. I keep the backend_url as optional parameter to ask users to update their code if the backend_url is set.

I will update the demo notebook and upload the package to pypi in next PR.

This change is

sdk/python/kubeflow/metadata/metadata.py

jinchihe · 2019-11-05T03:22:09Z

sdk/python/kubeflow/metadata/metadata.py

-    if backend_url_prefix is None or type(backend_url_prefix) != str:
-          raise ValueError("'backend_url_prefix' must be set and in string type.")
+    if backend_url_prefix:
+          raise ValueError("""'backend_url_prefix' is deprecated. Please set


is that better to WARNING here and remove from next release?

My original plan is to do what you suggested, but realized it would require to keep two ways of calling backends and the code would look messy.

Whoever want to use the previous SDK should ping down the version.

test/scripts/setup-services-run-tests.sh

zhenghuiwang · 2019-11-05T04:08:39Z

Thanks for the review @jinchihe

I updated the PR and it is ready for review @gaoning777

zhenghuiwang · 2019-11-07T19:43:54Z

/test kubeflow-metadata-presubmit

jinchihe · 2019-11-07T23:08:20Z

/lgtm

gaoning777 · 2019-11-08T21:19:44Z

sdk/python/kubeflow/metadata/metadata.py

+    config.host = grpc_host
+    config.port = grpc_port
+    if root_certificates or private_key or certificate_chain:
+      config.ssl_config = config.SSLConfig()


Should this be mlpb.MetadataStoreClientConfig.SSLConfig()? Maybe Protobuf support referencing the class through the instance.

It turns out this SubMessage ssl_config field is automatically initialized with properties with None value. So I removed this line.

But I found bug of MLMD when writing unit test:

File "/Users/zhenghui/kubeflow/metadata/.env/lib/python3.6/site-packages/ml_metadata/metadata_store/metadata_store.py", line 103, in _get_channel certificate_chain) File "/Users/zhenghui/kubeflow/metadata/.env/lib/python3.6/site-packages/grpc/__init__.py", line 1593, in ssl_channel_credentials certificate_chain)) File "src/python/grpcio/grpc/_cython/_cygrpc/credentials.pyx.pxi", line 133, in grpc._cython.cygrpc.SSLChannelCredentials.__cinit__ TypeError: expected certificate to be bytes, got <class 'str'>

gRPC requires all three fields to be bytes (https://grpc.github.io/grpc/python/grpc.html#grpc.ssl_channel_credentials)

But MLMD pass them in as string.

The fix is outside the scope of this PR.

Would you mind creating a bug for this?

google/ml-metadata#22

@gaoning777 , shall we address this along with the python client grpc server test?

sdk/python/kubeflow/metadata/metadata.py

zhenghuiwang · 2019-11-09T04:51:40Z

Thanks @gaoning777 @jinchihe for review!

@gaoning777 addressed your comments. PTAL

gaoning777 · 2019-11-11T23:48:18Z

/lgtm

zhenghuiwang · 2019-11-12T00:31:17Z

/approve

k8s-ci-robot · 2019-11-12T00:31:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhenghuiwang

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [zhenghuiwang]

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

* code complete; test passed * update e2e test * update comments * lint * lint & address comments * lint with yapf --style '{based_on_style: google, indent_width: 2}' * Adding Typing Hints; Rewrite docstrings * address comments * lint

…165) * Add metadata store protos * Add generated grpc protos for ml_metadata * Uses the version from Pipelines * Add generated grpc protos for ml_metadata * Bump `react-scripts` to 3.2.0 to support `EXTEND_ESLINT` environment variable, which is needed to `eslintIgnore` generated protos. * Add `api:metadata` script to regenerate grpc-web protos for ml_metadata. * Proxy requests to server to metadata-envoy-service * Move <rootDir>/__mocks__ into src to work around react-scripts@3.2.0 bug * facebook/create-react-app#7539 * Add `start-proxy.sh` to port forward to metadata-ui:3000 * Regenerate ArtifactList.test.tsx.snap - Old snapshots genreated with node 9.9.0 fail in node 12.13 used by docker image * Whitelist @jest/* packages in gen_licenses.js and point to third_party/jest/LICENSE * Switch ArtifactList getArtifactTypes to ml_metadata * Switch ArtifactList getArtifacts to ml_metadata * Use gRPC to fetch ArtifactDetails and ExecutionDetails * Mock grpc-web network calls * Use ml_metadata.Artifact in ModelInfo * Move repeated test model creation to TestUtils * Regenerate snapshot: ArtifactDetails.test.tsx.snap - Closing `}` were replaced with `)` in the commit to convert the snapshot to include a generated proto instead of a `Object` * Use ml_metadata.Exectuion in ExecutionList * Cleanup unused rpc in ArtifactList * Fix null check in ExecutionDetails * Regenerate ArtifactDetails snapshot after merging tabs with gRPC * Rewrite start-proxy.sh in node * Rewrite start-proxy.sh in node * Cosmetic changes to start-proxy.js * Fix brace styles * Rewrite gen_gprc_web_protos in node * Switch from @improbable-eng/grpc-web to grpc-web * Update grpc-web mock and fix failures in ArtifactDetails.test * Update Python SDK to use MLMD gRPC service (#156) * code complete; test passed * update e2e test * update comments * lint * lint & address comments * lint with yapf --style '{based_on_style: google, indent_width: 2}' * Adding Typing Hints; Rewrite docstrings * address comments * lint * [MLMD Lineage] Initial Lineage explorer draft implemented (#159) * Add metadata store protos * Add generated grpc protos for ml_metadata * Uses the version from Pipelines * Add generated grpc protos for ml_metadata * Bump `react-scripts` to 3.2.0 to support `EXTEND_ESLINT` environment variable, which is needed to `eslintIgnore` generated protos. * Add `api:metadata` script to regenerate grpc-web protos for ml_metadata. * Proxy requests to server to metadata-envoy-service * Move <rootDir>/__mocks__ into src to work around react-scripts@3.2.0 bug * facebook/create-react-app#7539 * Add `start-proxy.sh` to port forward to metadata-ui:3000 * Regenerate ArtifactList.test.tsx.snap - Old snapshots genreated with node 9.9.0 fail in node 12.13 used by docker image * Whitelist @jest/* packages in gen_licenses.js and point to third_party/jest/LICENSE * Switch ArtifactList getArtifactTypes to ml_metadata * Major changes 1.0 - Migrated half of lineage components - Rewrote all components in TS, updated and optimized to work in codebase - Css updated - **Erroneous, will fix** - Added gitignore - Added linter packages - Fixed spacing * Imported all elements * All of Lineage Explorer added: - All lineage components added - Card styles for target / execution cards - Rewrote the EdgeCavas renderer - Dyanmic updates and CSS updates to match live view - State diffing and rendering updates - Mocked data and presented on UI, via dynamic data - Z-indexing logic added to support reverse flipped connectors - State logic updated to match standard - Es6 Magic and refactor - Massive css updates (radio button revamp, et al) * Move changes to LineageView.tsx, which can then be used once Kwasi's changes are in * Resolved Kwasi's comments * Switch ArtifactList getArtifacts to ml_metadata * Rewrite start-proxy.sh in node * Switch from @improbable-eng/grpc-web to grpc-web * Fix package.json change introduced in rebase * Render LineageView in ArtifactDetails.LINEAGE_EXPLORER tab * Update ArtifactDetails snapshot * Update helper scripts - Fix extra parentheses in start-proxy.js and - Remove gen_grpc_web_protos.sh

zhenghuiwang added 2 commits November 4, 2019 14:40

code complete; test passed

3534f22

update e2e test

960085f

k8s-ci-robot added the do-not-merge/work-in-progress label Nov 4, 2019

k8s-ci-robot requested review from gaoning777 and jinchihe November 4, 2019 23:21

k8s-ci-robot added the size/L label Nov 4, 2019

zhenghuiwang added 2 commits November 4, 2019 15:28

update comments

b00299d

lint

a49d6cd

jinchihe reviewed Nov 5, 2019

View reviewed changes

lint & address comments

22f3363

zhenghuiwang removed the do-not-merge/work-in-progress label Nov 5, 2019

zhenghuiwang marked this pull request as ready for review November 5, 2019 04:08

lint with yapf --style '{based_on_style: google, indent_width: 2}'

66c47d3

k8s-ci-robot assigned jinchihe Nov 7, 2019

k8s-ci-robot added the lgtm label Nov 7, 2019

Adding Typing Hints; Rewrite docstrings

a32c08f

k8s-ci-robot added size/XL and removed lgtm size/L labels Nov 8, 2019

gaoning777 reviewed Nov 8, 2019

View reviewed changes

sdk/python/kubeflow/metadata/metadata.py Show resolved Hide resolved

address comments

17d9a95

lint

a47223a

zhenghuiwang mentioned this pull request Nov 11, 2019

Add reference docs for Metadata SDK kubeflow/website#1206

Closed

k8s-ci-robot assigned gaoning777 Nov 11, 2019

k8s-ci-robot added the lgtm label Nov 11, 2019

k8s-ci-robot added the approved label Nov 12, 2019

k8s-ci-robot merged commit a38effc into kubeflow:master Nov 12, 2019

zhenghuiwang mentioned this pull request Nov 15, 2019

sdk/python: docstring typo #132

Closed

zhenghuiwang mentioned this pull request Dec 13, 2019

Update the python SDK to talk to MLMD gRPC service #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Python SDK to use MLMD gRPC service #156

Update Python SDK to use MLMD gRPC service #156

zhenghuiwang commented Nov 4, 2019 •

edited by jlewi

jinchihe Nov 5, 2019

zhenghuiwang Nov 5, 2019

zhenghuiwang commented Nov 5, 2019

zhenghuiwang commented Nov 7, 2019

jinchihe commented Nov 7, 2019

gaoning777 Nov 8, 2019

zhenghuiwang Nov 9, 2019

gaoning777 Nov 11, 2019

zhenghuiwang Nov 12, 2019

gaoning777 Nov 12, 2019

hughmiao Nov 12, 2019

gaoning777 Nov 15, 2019

zhenghuiwang commented Nov 9, 2019

gaoning777 commented Nov 11, 2019

zhenghuiwang commented Nov 12, 2019

k8s-ci-robot commented Nov 12, 2019

Update Python SDK to use MLMD gRPC service #156

Update Python SDK to use MLMD gRPC service #156

Conversation

zhenghuiwang commented Nov 4, 2019 • edited by jlewi

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenghuiwang commented Nov 5, 2019

zhenghuiwang commented Nov 7, 2019

jinchihe commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenghuiwang commented Nov 9, 2019

gaoning777 commented Nov 11, 2019

zhenghuiwang commented Nov 12, 2019

k8s-ci-robot commented Nov 12, 2019

zhenghuiwang commented Nov 4, 2019 •

edited by jlewi