Add grpcio-status extension package #17490

lidizheng · 2018-12-13T01:19:31Z

The new package has 2 API from_call and to_status
Utilize the experimental API abort_with_status
Add 5 unit test cases

grpc-testing · 2018-12-13T01:24:35Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-13T01:36:28Z

[trickle] No significant performance differences

grpc-testing · 2018-12-13T02:07:38Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,020,870      Total (=)      2,020,870

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,177,127      Total (>)     11,177,121

 No significant differences in binary sizes

grpc-testing · 2018-12-13T02:17:16Z

[microbenchmarks] No significant performance differences

* The new package has 2 API `from_call` and `to_status` * Utilize the experimental API `abort_with_status` * Add 5 unit test cases

grpc-testing · 2018-12-13T18:23:39Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-13T18:31:24Z

[trickle] No significant performance differences

grpc-testing · 2018-12-13T19:08:21Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,311,965      Total (<)     11,311,966

 No significant differences in binary sizes

grpc-testing · 2018-12-13T19:15:17Z

[microbenchmarks] No significant performance differences

src/python/grpcio_tests/tests/status/_grpc_status_test.py

gnossen · 2018-12-13T19:20:20Z

tools/run_tests/helper_scripts/build_python.sh

 # Install testing
 pip_install_dir "$ROOT/src/python/grpcio_testing"

 # Build/install tests
 $VENV_PYTHON -m pip install coverage==4.4 oauth2client==4.1.0 \
-                            google-auth==1.0.0 requests==2.14.2
+                            google-auth==1.0.0 requests==2.14.2 \


Is there a reason we're pinning to specific versions of these libraries?

Thanks for reviewing. I don't have the context about these libraries' version. But the build_python.sh is only used in testing environment. Pin the packages to a working version can protect us from flakiness/failures introduced by other packages' update.

grpc-testing · 2018-12-14T01:11:15Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-14T01:21:20Z

[trickle] No significant performance differences

grpc-testing · 2018-12-14T01:23:56Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-14T01:31:44Z

[trickle] No significant performance differences

grpc-testing · 2018-12-14T01:54:04Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,324,730      Total (>)     11,324,723

 No significant differences in binary sizes

grpc-testing · 2018-12-14T02:04:01Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-12-14T02:07:55Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,324,729      Total (>)     11,324,722

 No significant differences in binary sizes

grpc-testing · 2018-12-14T02:11:57Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-14T02:17:28Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-12-14T02:21:39Z

[trickle] No significant performance differences

grpc-testing · 2018-12-14T02:24:42Z

[trickle] No significant performance differences

grpc-testing · 2018-12-14T02:54:58Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,324,721      Total (<)     11,324,725

 No significant differences in binary sizes

grpc-testing · 2018-12-14T02:57:49Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,324,730      Total (>)     11,324,725

 No significant differences in binary sizes

grpc-testing · 2018-12-14T03:03:26Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-12-14T03:06:10Z

[microbenchmarks] No significant performance differences

ericgribkoff

A couple suggestions for the docstrings on the new API, otherwise LGTM

Should the APIs in the new package be explicitly marked as experimental, or is this already implied by some other part of our distribution process?

src/python/grpcio_status/grpc_status/rpc_status.py

lidizheng · 2018-12-14T18:13:35Z

@ericgribkoff Thanks for reviewing. I have adopted your advice and updated the docstring:

Mark those APIs as experimental
Rephrase the raise condition
Add more detail to the returned object

ericgribkoff · 2018-12-14T18:18:04Z

src/python/grpcio_status/grpc_status/rpc_status.py

    Args:
      call: A grpc.Call instance.

    Returns:
      A google.rpc.status.Status message representing the status of the RPC.

    Raises:
-      ValueError: If the status code, status message is inconsistent with the rich status
-        inside of the google.rpc.status.Status.
+      ValueError: If the gRPC call's code or details are inconsistent with the status code and message inside of the google.rpc.status.Status.


Line length?

grpc-testing · 2018-12-14T18:18:10Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-14T18:25:06Z

[trickle] No significant performance differences

* Mark API as experimental * Rephrase the raise condition * Add more detail to the returned object

grpc-testing · 2018-12-14T18:34:18Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-14T18:42:59Z

[trickle] No significant performance differences

grpc-testing · 2018-12-14T19:01:34Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,324,728      Total (>)     11,324,727

 No significant differences in binary sizes

grpc-testing · 2018-12-14T19:10:29Z

[microbenchmarks] No significant performance differences

lidizheng · 2018-12-14T19:25:07Z

This PR implements the extension package of grpc/proposal#118

grpc-testing · 2018-12-14T19:26:36Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-12-14T19:28:44Z

****************************************************************

libgrpc.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-12-14T19:36:03Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,324,732      Total (>)     11,324,724

 No significant differences in binary sizes

grpc-testing · 2018-12-14T19:39:21Z

[trickle] No significant performance differences

grpc-testing · 2018-12-14T20:20:31Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-12-14T20:39:53Z

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,026,842      Total (=)      2,026,842

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,324,723      Total (<)     11,324,731

 No significant differences in binary sizes

lidizheng added kind/enhancement lang/Python release notes: yes Indicates if PR needs to be in release notes labels Dec 13, 2018

lidizheng mentioned this pull request Dec 13, 2018

Rich status transport implementation #17384

Closed

Add grpcio-status extension package

45b3230

* The new package has 2 API `from_call` and `to_status` * Utilize the experimental API `abort_with_status` * Add 5 unit test cases

lidizheng force-pushed the py-status-3 branch from c8b2f0c to 45b3230 Compare December 13, 2018 18:19

lidizheng requested review from ericgribkoff and gnossen December 13, 2018 18:19

lidizheng assigned gnossen and ericgribkoff Dec 13, 2018

gnossen approved these changes Dec 13, 2018

View reviewed changes

Fix typo && remove unecessary except

0e2f406

Include LICENSE in artifact

b9cb245

lidizheng force-pushed the py-status-3 branch from 3d97b11 to b9cb245 Compare December 14, 2018 02:08

ericgribkoff approved these changes Dec 14, 2018

View reviewed changes

src/python/grpcio_status/grpc_status/rpc_status.py Outdated Show resolved Hide resolved

src/python/grpcio_status/grpc_status/rpc_status.py Outdated Show resolved Hide resolved

ericgribkoff reviewed Dec 14, 2018

View reviewed changes

ericgribkoff approved these changes Dec 14, 2018

View reviewed changes

Update docstring to make it more clear

40b8ca9

* Mark API as experimental * Rephrase the raise condition * Add more detail to the returned object

lidizheng force-pushed the py-status-3 branch from 9ad3d4a to 40b8ca9 Compare December 14, 2018 18:29

Assign noop to build_package_protos for backward compatibility

8621bd4

lidizheng merged commit d64fd75 into grpc:master Dec 14, 2018

AspirinSJL mentioned this pull request Dec 17, 2018

Revert "Add grpcio-status extension package" #17527

Closed

lock bot locked as resolved and limited conversation to collaborators Mar 14, 2019

Add grpcio-status extension package #17490

Add grpcio-status extension package #17490

Conversation

lidizheng commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

grpc-testing commented Dec 13, 2018

gnossen Dec 13, 2018

Choose a reason for hiding this comment

lidizheng Dec 14, 2018

Choose a reason for hiding this comment

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

ericgribkoff left a comment

Choose a reason for hiding this comment

lidizheng commented Dec 14, 2018

ericgribkoff Dec 14, 2018

Choose a reason for hiding this comment

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

lidizheng commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018

grpc-testing commented Dec 14, 2018