Use a class for CachedMapper-derived mappers instead of a dict
#549
+260
−120
Conversation
This file contains 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
majosm
force-pushed
the
add-cache-class
branch
3 times, most recently
from
a34936a to
378d439
Compare
This was referenced Sep 24, 2024
majosm
force-pushed
the
add-cache-class
branch
3 times, most recently
from
1cf2454 to
df7db39
Compare
majosm
force-pushed
the
add-cache-class
branch
4 times, most recently
from
d40153c to
e9b4ac5
Compare
majosm
force-pushed
the
add-cache-class
branch
2 times, most recently
from
c25dce2 to
7a556ad
Compare
majosm
commented
Jan 16, 2025
pytato/transform/__init__.py
Outdated
Comment on lines
321
to
325
| # FIXME: Can this be inlined? | ||
| def get_key( | ||
| self, expr: CacheExprT, *args: P.args, **kwargs: P.kwargs) -> CacheKeyT: | ||
| """Compute the key for an input expression.""" | ||
| return self._key_func(expr, *args, **kwargs) |
There was a problem hiding this comment.
Is it possibly worth inlining this to avoid the Python call overhead? How would I go about doing that?
There was a problem hiding this comment.
Comment on lines
+331
to
+333
| # FIXME: Figure out the right way to type annotate these | ||
| | tuple[CacheExprT, tuple[Any, ...], dict[str, Any]], |
There was a problem hiding this comment.
How should I annotate the args tuple and kwargs dict here so that it matches *args: P.args, **kwargs: P.kwargs?
There was a problem hiding this comment.
pytato/transform/__init__.py
Outdated
Comment on lines
887
to
888
| # FIXME: Can this just inherit from CachedMapper? | ||
| class CombineMapper(Mapper[ResultT, FunctionResultT, []]): |
There was a problem hiding this comment.
Was there a reason that CombineMapper doesn't inherit from CachedMapper? Seems like we could plausibly do that now, I just wanted to ask before I take a stab at it.
majosm
force-pushed
the
add-cache-class
branch
2 times, most recently
from
a60b0f3 to
387bafb
Compare
inducer
reviewed
Jan 21, 2025
pytato/transform/__init__.py
Outdated
| :arg key_func: Function to compute a hashable cache key from an input | ||
| expression and any extra arguments. | ||
| """ | ||
| self._key_func = key_func |
There was a problem hiding this comment.
Suggested change
| self._key_func = key_func | |
| self.get_key = key_func |
pytato/transform/__init__.py
Outdated
Comment on lines
321
to
325
| # FIXME: Can this be inlined? | ||
| def get_key( | ||
| self, expr: CacheExprT, *args: P.args, **kwargs: P.kwargs) -> CacheKeyT: | ||
| """Compute the key for an input expression.""" | ||
| return self._key_func(expr, *args, **kwargs) |
pytato/transform/__init__.py
Outdated
Comment on lines
321
to
325
| # FIXME: Can this be inlined? | ||
| def get_key( | ||
| self, expr: CacheExprT, *args: P.args, **kwargs: P.kwargs) -> CacheKeyT: | ||
| """Compute the key for an input expression.""" | ||
| return self._key_func(expr, *args, **kwargs) |
There was a problem hiding this comment.
Comment on lines
+331
to
+333
| # FIXME: Figure out the right way to type annotate these | ||
| | tuple[CacheExprT, tuple[Any, ...], dict[str, Any]], |
There was a problem hiding this comment.
| """Compute the key for an input expression.""" | ||
| return self._key_func(expr, *args, **kwargs) | ||
|
|
||
| def add( |
There was a problem hiding this comment.
Alternative to add/retrieve: "just give me the thing from the cache and OBTW here's a function to compute it if you don't have it"
(No strong preference from my end)
pytato/transform/__init__.py
Outdated
| # Arrays are cached separately for each call stack frame, but | ||
| # functions are cached globally | ||
| _function_cache: dict[Hashable, FunctionResultT] | None = None | ||
| _cache: CachedMapper._CacheT[ResultT, P] | None = None, |
There was a problem hiding this comment.
Pyright doesn't seem to like this. As yet I'm unclear why.
pip install pyright
Access to generic instance variable through class is ambiguous
pytato/transform/__init__.py
Outdated
| @@ -768,40 +884,53 @@ def map_named_call_result(self, expr: NamedCallResult, | |||
|
|
|||
| # {{{ CombineMapper | |||
|
|
|||
| # FIXME: Can this just inherit from CachedMapper? | |||
There was a problem hiding this comment.
From my POV: no reason why it shouldn't.
majosm
force-pushed
the
add-cache-class
branch
2 times, most recently
from
40b2a64 to
9cbe7f4
Compare
majosm
force-pushed
the
add-cache-class
branch
from
9cbe7f4 to
8546893
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds classes to represent the caches of
CachedMapper-derived mappers, which will be useful for adding array duplication checks and result deduplication (#550). Both of these features add additional cache dictionaries and cache retrieval/addition logic; this change minimizes the amount of logic that must be duplicated when mappers overriderec, as well as minimizes the extra arguments that need to be passed around for function caches when cloning new mappers.Depends on #531(merged).