-
Notifications
You must be signed in to change notification settings - Fork 90
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
Merged
Merged
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
5518645
feat: optimize _GapicCallable
daniel-sanche 0cc03bd
cleaned up metadata lines
daniel-sanche c97a636
chore: avoid type checks in error wrapper
daniel-sanche b453df4
Revert "chore: avoid type checks in error wrapper"
daniel-sanche 2f7acff
add default wrapped function
daniel-sanche 31f0b4e
fixed decorator order
daniel-sanche b92328c
fixed spacing
daniel-sanche 0831dbf
fixed comment typo
daniel-sanche a1563d2
fixed spacing
daniel-sanche fb1a372
Merge branch 'main' into optimize_gapic_callable
daniel-sanche 52ed5be
Merge branch 'main' into optimize_gapic_callable
daniel-sanche 85e2102
fixed spacing
daniel-sanche c76f51c
removed unneeded helpers
daniel-sanche f4a9021
use caching
daniel-sanche cacc73c
improved metadata parsing
daniel-sanche a30101d
improved docstring
daniel-sanche db9a9c4
fixed logic
daniel-sanche a555629
added benchmark test
daniel-sanche cfe5c7d
Merge branch 'main' into optimize_gapic_callable
daniel-sanche fbbaaca
update threshold
daniel-sanche 576bb0f
run benchmark in loop for testing
daniel-sanche 7c32a5d
use verbose logs
daniel-sanche 26bec79
Revert testing
daniel-sanche c25e0eb
used smaller value
daniel-sanche 49201ca
changed threshold
daniel-sanche 3d2c964
Merge branch 'main' into optimize_gapic_callable
daniel-sanche c9c1ff3
Merge branch 'main' into optimize_gapic_callable
daniel-sanche deca58c
removed link in comment
daniel-sanche File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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