feat: add caching to GapicCallable - #527
Conversation
Sorry, something went wrong.
This reverts commit c97a636.
parthea
left a comment
There was a problem hiding this comment.
This seems to make it 50% faster
Thanks for fixing this!
Please could you add a simple benchmarking presubmit, similar to the test that you ran manually, to avoid future regressions in performance?
Sorry, something went wrong.
|
Assigning back to @daniel-sanche to resolve the presubmit failure |
Sorry, something went wrong.
|
Hmm good point, the benchmark result will be machine-specific, and I was doing my tests locally instead of with the CI workers. I guess I'll have to find an assertion value that works well for the CI nodes, and I'll add a comment explaining that it may be flake on slower hardware. Or let me know if you have other suggestions for how to approach this |
Sorry, something went wrong.
|
Can you set it high enough that we don't get flaky results, but low enough that we can detect performance regressions. Perhaps set the threshold to 0.4 for now and create an issue in https://github.com/googleapis/python-api-core/issues to add a proper benchmarking test ? I believe @ohmayr started looking into a benchmarking presubmit so please tag him on the issue. |
Sorry, something went wrong.
|
Sure, I opened an issue to track this here: googleapis/google-cloud-python#15058 I adjusted the value to 0.4. Feel free to merge it with that number, but I suspect we can find a lower value that still avoids flakiness. Let me know if you want me to do some investigation |
Sorry, something went wrong.
|
@vchudnov-g Please could you review? |
Sorry, something went wrong.
vchudnov-g
left a comment
There was a problem hiding this comment.
Minor code comment, and an idea about tightening benchmarks.
Sorry, something went wrong.
| Note: The threshold has been tuned for the CI workers. Test may flake on | ||
| slower hardware | ||
|
|
||
| https://github.com/googleapis/python-api-core/pull/527 |
There was a problem hiding this comment.
Do you mean to self-reference this PR?
Sorry, something went wrong.
There was a problem hiding this comment.
It was intentional, to give the context on this test. But on second thought, git blame should be enough. Removed
Sorry, something went wrong.
| lambda *a, **k: 1, retry=Retry(), timeout=1010, compression=False | ||
| ) | ||
| avg_time = timeit(lambda: gapic_callable(), number=10_000) | ||
| assert avg_time < 0.4 |
There was a problem hiding this comment.
Idea: If the assertion fails, print both the actual time it took and enough platform information so that in the future we can add the right threshold for the platform. The latter would be something like this
platform_threshold = { "foo": 0.2, "bar": 0.6 }
current_platform = ...
...
assert avg_time < platform_threshold.get(current_platform, 0.4)
In fact, you could implement platform_threshold now, and start with whatever your current machine is.
Sorry, something went wrong.
There was a problem hiding this comment.
That's an interesting idea, but it's not completely clear to me what we'd need to capture for the platform. Number of CPUs? Architecture? OS? Let me know if you have thoughts
We already have https://github.com/googleapis/python-api-core/issues/616 to track improving this though, so if it's alright with you, I'll merge this as-is and we can discuss follow-up there
Sorry, something went wrong.
_GapicCallable currently does a lot of work on each call, re-building each wrapped function, using a lot of calls to helper functions. This cost is added to every single rpc, so it can really add up
This PR does the following optimizations
Benchmark:
Before: 20.43s
After: 9.48s
BEGIN_COMMIT_OVERRIDE
chore: add caching to GapicCallable
END_COMMIT_OVERRIDE