Add CacheInputs class to simplify passing of expr + optional args/kwargs/key#583
Add CacheInputs class to simplify passing of expr + optional args/kwargs/key#583inducer merged 5 commits intoinducer:mainfrom
CacheInputs class to simplify passing of expr + optional args/kwargs/key#583Conversation
pytato/transform/__init__.py
Outdated
|
|
||
|
|
||
| class CachedMapperCache(Generic[CacheExprT, CacheResultT]): | ||
| class CacheInputs(Generic[CacheExprT, P]): |
There was a problem hiding this comment.
| class CacheInputs(Generic[CacheExprT, P]): | |
| class CacheInputWithKey(Generic[CacheExprT, P]): |
| class CacheInputs(Generic[CacheExprT, P]): | |
| class CacheKeyable(Generic[CacheExprT, P]): |
?
There was a problem hiding this comment.
Changed to CacheInputsWithKey.
inducer
left a comment
inducer
left a comment
There was a problem hiding this comment.
Forgot to submit these yesterday. This should be good to go once these are addressed.
pytato/transform/__init__.py
Outdated
|
|
||
|
|
||
| class CachedMapperCache(Generic[CacheExprT, CacheResultT]): | ||
| class CacheInputs(Generic[CacheExprT, P]): |
There was a problem hiding this comment.
I like how clean this is, but it's also throwing around a bunch more objects than before. Do you have a sense of the timing impact?
There was a problem hiding this comment.
The previous version had caused a bit of a slowdown, so I refactored it a bit. Now it performs roughly the same as the current main.
majosm
force-pushed
the
add-cache-inputs
branch
3 times, most recently
from
0ce18ff to
13d5ab0
Compare
|
Ready for a look after #585. |
| return self._cache.retrieve(inputs) | ||
| except KeyError: | ||
| s = super().rec(expr) | ||
| s = Mapper.rec(self, expr) |
| return (expr, *args, tuple(sorted(kwargs.items()))) | ||
| ) -> CacheKeyT: | ||
| if args or kwargs: | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
Explain that cache could be different based on by-kw or by-position passing.
| return (expr, *args, tuple(sorted(kwargs.items()))) | ||
| ) -> CacheKeyT: | ||
| if args or kwargs: | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
Explain that cache could be different based on by-kw or by-position passing.
majosm
force-pushed
the
add-cache-inputs
branch
from
13d5ab0 to
d5b5305
Compare
…nition_cache_key for extra args case ambiguous due to the fact that any arg can be specified with/without keyword
…n_definition_cache_key are not defined for general extra args/kwargs
majosm
force-pushed
the
add-cache-inputs
branch
from
d5b5305 to
81d300a
Compare
|
Thx! |
2 participants
Attempts to simplify the handling for mappers with/without extra arguments, and also for keys being computed up front and passed in.
Note: This PR also disables the default implementations of
get_cache_key/get_function_definition_cache_keyfor the extra args case; the previous implementations seem ambiguous, since a given argument can be passed with/without keyword and end up in different parts of the key tuple. I need to check if this works OK with mirgecom.Depends on #585.