Add basic benchmark test for Python by lidizheng · Pull Request #17793 · grpc/grpc

lidizheng · 2019-01-23T01:42:58Z

Running a single performance benchmark scenario for Python takes 15 minutes. The scenario here named "python_protobuf_sync_unary_ping_pong".

$ time tools/run_tests/run_performance_tests.py --language python -r='^python_protobuf_sync_unary_ping_pong$'
# Skip outputs
real	4m24.825s
user	15m16.941s
sys	1m48.508s

To boost our productivity, we have to speed up the benchmark test. The whole benchmark mechanism is designed for CI, and it is mixing the control flow with data flow. The related code amount is huge (thousands of lines across languages & folders). IMO it is better to utilize existing code as much as I can without changing its behavior.

The first change is building Python C extension incrementally. To achieve that, this PR added a Bazel BUILD file to instruct Bazel how to build Python benchmark related code. Then make it depends on our core package //src/python/grpcio/grpc:grpcio.

The second change is adding a basic_benchmark_test. It runs two scenario that could cover major use cases of gRPC Python. What is does is that it spins up two Python qps_worker to serve as client and server, and then spins up a qps_json_driver to control the two worker to run benchmarks. The result will be print to stdout. And this PR includes a simple doc to make it easier to use.

The time usage reduces dramatically (and it is running two scenarios):

$ time bazel test --test_output=streamed src/python/grpcio_tests/tests/qps:basic_benchmark_test
# Skip outputs
real	0m26.429s
user	0m0.039s
sys	0m0.050s

PS: If I use a py_test, the sanity check will complain that I didn't add it to the tests.json. Then, I need to disable it at two or three places...

gnossen

Just some nits. Looking really nice.

gnossen · 2019-01-23T17:26:20Z

src/python/grpcio_tests/tests/qps/basic_benchmark_test.sh

+function join { local IFS="$1"; shift; echo "$*"; }
+
+if [[ -e "${SCENARIOS_FILE}" ]]; then
+    echo "Running against scenarios.json:"


Maybe this could be echo "Running against ${SCENARIOS_FILE}:"?

That's better!

gnossen · 2019-01-23T17:31:56Z

src/python/grpcio_tests/tests/qps/README.md

+
+## Why keep the scenario file if it can be generated?
+
+Well... The `tools/run_tests/performance/scenario_config.py` is 1274 lines long. The intention of building these benchmark tools is reducing the complexity of existing infrastructure code. Depending on something that is 


Unfinished sentence?

Thanks for pointing out. The updated version:

Well... The tools/run_tests/performance/scenario_config.py is 1274 lines long. The intention of building these benchmark tools is reducing the complexity of existing infrastructure code. So, instead of calling layers of abstraction to generate the scenario file, keeping a valid static copy is more preferable.

jtattermusch · 2019-01-25T08:00:05Z

A few insights:

AFAIK the only reason why tools/run_tests/run_performance_tests.py is slow is that python build is also slow. The script doesn't do much beside building and then running the scenario (each scenario takes ~35 secs to run). Perhaps we should focus on making python build faster (and make sure the right args are used in

grpc/tools/run_tests/performance/build_performance.sh

Line 73 in f7a4d1e

python tools/run_tests/run_tests.py -l "$language" -c "$CONFIG" --compiler python2.7 --build_only -j 8

)
there is a kokoro job that allows you to run the performance benchmark in a single-machine setup:
https://fusion.corp.google.com/projectanalysis/current/KOKORO/prod%3Agrpc%2Fcore%2Fmaster%2Flinux%2Fgrpc_e2e_performance_singlevm
before submitting please doublecheck that the performance benchmark scenarios continue to work as they are a bit fragile (there is a way to run an adhoc run with multimachine setup, I can help with that once I have a bit of free time).
please do not submit any changes to performance benchmarks without me reviewing first.

jtattermusch · 2019-01-25T08:06:34Z

I scanned through the code quickly and looks like this is only adding a bazel test that runs a few basic python benchmarks locally. If that's the case and there are no changes to the run_performance_tests.py suite, I probably don't need to review.

(but I still don't quite understand why running the run_performance_tests.py script is so slow for python - we have accelerated the python build recently, haven't we?).

lidizheng · 2019-01-25T19:23:20Z

There are two major performance issue with setuptools which is the build tooling gRPC Python is using. The first is that it builds C files one-by-one which solved by your PR. And the second is that it doesn't do incremental builds as I wrote in the description... In comparison, Bazel is doing really well to produce a development build, it is fast and stable.

The purpose of this PR is adding a handy tool to tune performance locally with frequent code changes. However, we still need the build_python.sh to depend on setuptools since only setuptools is able to generate distribution packages which is the version actually used by our users.

ericgribkoff

LGTM

Regarding build speed of python: the recent changes did make it faster, but it's still on the order of minutes when building core as well. It seems that setuptools has little (afaict, no) support for incremental compilation: we found https://stackoverflow.com/questions/47539538/setup-py-build-that-doesnt-reinvoke-compiler-on-every-c-source-file-increment, which suggests an approach like plugging in bazel to do the c/cython step within setuptools may be a viable solution, but that's a longer term approach.

ericgribkoff · 2019-01-25T19:41:57Z

src/python/grpcio_tests/tests/qps/README.md

+bazel test --test_output=streamed src/python/grpcio_tests/tests/qps:basic_benchmark_test
+```
+
+## How is the output look like?


s/How is/What does/

ericgribkoff · 2019-01-25T19:44:39Z

src/python/grpcio_tests/tests/qps/README.md

+
+## Why keep the scenario file if it can be generated?
+
+Well... The `tools/run_tests/performance/scenario_config.py` is 1274 lines long. The intention of building these benchmark tools is reducing the complexity of existing infrastructure code. So, instead of calling layers of abstraction to generate the scenario file, keeping a valid static copy is more preferable.


s/more preferable/preferable/

Optional: Add a "future-proofing" sentence like - "If the use case for this tool grows beyond these two scenarios, we can incorporate automatic generation and selection of scenarios into the tool."

Added:

Also, if the use case for this tool grows beyond simple static scenarios, we can incorporate automatic generation and selection of scenarios into the tool.

ericgribkoff · 2019-01-25T19:45:36Z

src/python/grpcio_tests/tests/qps/README.md

+* python_protobuf_sync_streaming_qps_unconstrained
+* python_protobuf_sync_unary_ping_pong_1MB
+
+Here I picked the top 2 most representative scenarios of them, and reduce their benchmark duration from 30 seconds to 10 seconds:


Is "most representative" backed by an objective measurement? If not its fine, perhaps just reword to "I [we? I think we usually use we in the readme files] picked a small but representative subset"

Add basic benchmark test for Python

4f451c7

lidizheng added area/tools lang/Python area/performance/benchmarking release notes: no Indicates if PR should not be in release notes labels Jan 23, 2019

Add copyright to BUILD file

95d4120

gnossen approved these changes Jan 23, 2019

View reviewed changes

Adopte reviewer's suggestion

b60c5cd

lidizheng force-pushed the py-bm branch from e1bfe92 to b60c5cd Compare January 23, 2019 20:27

lidizheng changed the title ~~[WIP] Add basic benchmark test for Python~~ Add basic benchmark test for Python Jan 23, 2019

lidizheng requested a review from ericgribkoff January 23, 2019 20:40

lidizheng assigned gnossen and ericgribkoff Jan 23, 2019

jtattermusch self-requested a review January 25, 2019 07:50

ericgribkoff approved these changes Jan 25, 2019

View reviewed changes

Update the README.md

f817d49

lidizheng merged commit 16b8279 into grpc:master Jan 28, 2019

lidizheng mentioned this pull request Jan 29, 2019

Revert "Add basic benchmark test for Python" #17854

Merged

lock bot locked as resolved and limited conversation to collaborators Apr 28, 2019

lidizheng added the area/infra label Feb 19, 2020


		## Why keep the scenario file if it can be generated?

		Well... The `tools/run_tests/performance/scenario_config.py` is 1274 lines long. The intention of building these benchmark tools is reducing the complexity of existing infrastructure code. Depending on something that is

Conversation

lidizheng commented Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnossen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtattermusch commented Jan 25, 2019

Uh oh!

jtattermusch commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lidizheng commented Jan 25, 2019

Uh oh!

ericgribkoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lidizheng commented Jan 23, 2019 •

edited

Loading

jtattermusch commented Jan 25, 2019 •

edited

Loading