-
Notifications
You must be signed in to change notification settings - Fork 92
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 27 commits
5518645
0cc03bd
c97a636
b453df4
2f7acff
31f0b4e
b92328c
0831dbf
a1563d2
fb1a372
52ed5be
85e2102
c76f51c
f4a9021
cacc73c
a30101d
db9a9c4
a555629
cfe5c7d
fbbaaca
576bb0f
7c32a5d
26bec79
c25e0eb
49201ca
3d2c964
c9c1ff3
deca58c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -222,3 +222,26 @@ def test_wrap_method_with_call_not_supported(): | |
with pytest.raises(ValueError) as exc_info: | ||
google.api_core.gapic_v1.method.wrap_method(method, with_call=True) | ||
assert "with_call=True is only supported for unary calls" in str(exc_info.value) | ||
|
||
|
||
def test_benchmark_gapic_call(): | ||
""" | ||
Ensure the __call__ method performance does not regress from expectations | ||
|
||
__call__ builds a new wrapped function using passed-in timeout and retry, but | ||
subsequent calls are cached | ||
|
||
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 | ||
""" | ||
from google.api_core.gapic_v1.method import _GapicCallable | ||
from google.api_core.retry import Retry | ||
from timeit import timeit | ||
|
||
gapic_callable = _GapicCallable( | ||
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 commentThe 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
In fact, you could implement There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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