Python(feat): disk caching #346
Conversation
alexluck-sift
force-pushed
the
python/feat/disk-caching
branch
from
b3c21de to
15c302b
Compare
ian-sift
left a comment
ian-sift
left a comment
There was a problem hiding this comment.
not finished w/ full pass, so just left some optional nits.
Would be curious about instrumenting metrics on how often a cache was hit as part of evaluating this change
| grpc_client = GrpcClient(connection_config.get_grpc_config()) | ||
| grpc_config = connection_config.get_grpc_config() | ||
| # Override cache_config if provided directly to SiftClient | ||
| grpc_client = GrpcClient(grpc_config) |
There was a problem hiding this comment.
what is changing here? was there a missing step?
| mode: Cache behavior mode (enabled, disabled, clear_on_init). | ||
| ttl: Time-to-live for cached entries in seconds. Default is 1 week. | ||
| cache_folder: Path to the cache directory. Default is system temp directory. | ||
| size_limit: Maximum size of the cache in bytes. Default is 5GB. |
There was a problem hiding this comment.
declare these in the class for type hints?
|
|
||
| # Add cache config if enabled | ||
| if self.cache_config and self.cache_config.is_enabled: | ||
| config["cache_config"] = self.cache_config.to_sift_cache_config() |
There was a problem hiding this comment.
could move this directly into SiftChannelConfig
| for key, value in metadata: | ||
| metadata_dict[key] = value | ||
|
|
||
| use_cache = metadata_dict.get(METADATA_USE_CACHE, "false").lower() == "true" |
There was a problem hiding this comment.
could optionally bool(metdata_dict.get(METADATA_USE_CACHE))
| request, | ||
| use_cache=use_cache, | ||
| force_refresh=force_refresh, | ||
| ttl=cache_ttl, |
There was a problem hiding this comment.
could these just always be passed and defer the if use_cache or force_refresh: logic to the grpc interceptor? idk if it could be tagged onto get_stub or something
| grpc_url: The Sift gRPC API URL. | ||
| rest_url: The Sift REST API URL. | ||
| connection_config: A SiftConnectionConfig object to configure the connection behavior of the SiftClient. | ||
| connection_config: A SiftConnectionConfig object to configure the connection and cache behavior of the SiftClient. |
There was a problem hiding this comment.
would be helpful to add something here like the other examples you added in low_level_wrappers/base.py or caching.py since i'd imagine one of the first pages users would look at is https://sift-stack.github.io/sift/python/pr-346/reference/sift_client/client/
| self._cache_results = False | ||
|
|
||
| async def ping(self) -> str: | ||
| async def ping(self, _force_refresh: bool = False) -> str: |
There was a problem hiding this comment.
noticing none of this plumbing is exposed at the high level resource layer -- would the expectation be that people call the low level client directly?
|
Could you also add some background to on why diskcache vs something like functools @cache decorator on the low level wrapper functions? It seems like for the most part the intention of the low level wrappers is to be 1:1 to the actual GRPC calls so I'd imagine caching the result for the given args at the low level wrapper would provide similar results to caching the GRPC call but without the complexity of the interceptors |
Prototype of a gRPC disk cache