-
Notifications
You must be signed in to change notification settings - Fork 90
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add caching to GapicCallable #527
Conversation
This reverts commit c97a636.
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 |
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. |
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 |
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 implement platform_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
_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
_apply_decorators
and_is_not_none_or_false
to build the wrapped call more directly.Benchmark:
Before: 20.43s
After: 9.48s
BEGIN_COMMIT_OVERRIDE
chore: add caching to GapicCallable
END_COMMIT_OVERRIDE