- Notifications
You must be signed in to change notification settings - Fork90
feat: add caching to GapicCallable#527
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This reverts commitc97a636.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
Assigning back to@daniel-sanche to resolve the presubmit failure |
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 |
parthea commentedFeb 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 inhttps://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. |
Sure, I opened an issue to track this here:#616 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 |
@vchudnov-g Please could you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor code comment, and an idea about tightening benchmarks.
tests/unit/gapic/test_method.py Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you mean to self-reference this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It was intentional, to give the context on this test. But on second thought,git blame should be enough. Removed
| lambda *a, **k: 1, retry=Retry(), timeout=1010, compression=False | ||
| ) | ||
| avg_time = timeit(lambda: gapic_callable(), number=10_000) | ||
| assert avg_time < 0.4 |
vchudnov-gMar 29, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 implementplatform_threshold now, and start with whatever your current machine is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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#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
d96eb5c intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
_GapicCallable currently does a lot of work on eachcall, 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
_apply_decoratorsand_is_not_none_or_falseto build the wrapped call more directly.Benchmark:
Before: 20.43s
After: 9.48s
BEGIN_COMMIT_OVERRIDE
chore: add caching to GapicCallable
END_COMMIT_OVERRIDE