-
Notifications
You must be signed in to change notification settings - Fork 52
fix[next]: Use CustomMapping for caching of offset provider in program call #2044
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
Conversation
This PR does not solve the problem. I still see all cache misses, probably because some of the values themselves are new objects at each program call. |
cscs-ci run default |
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.
Looks good but I have some suggestions to make it slightly cleaner.
src/gt4py/next/ffront/decorator.py
Outdated
try: | ||
op_extended = self._offset_provider_extended_cache[offset_provider] | ||
except KeyError: | ||
op_extended = {**self._implicit_offset_provider, **offset_provider} |
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.
op_extended = {**self._implicit_offset_provider, **offset_provider} | |
op_extended = self._implicit_offset_provider | | |
offset_provider |
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.
I get a type error:
error: No overload variant of "__or__" of "dict" matches argument type "Mapping[str, Dimension | NeighborConnectivity]" [operator]
review was for a different implementation
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.
LGTM. There are two more suggestions but they are so simple that don't need another round of review, so I'm approving now the PR.
cscs-ci run default |
Fix the cache key for the offset provider. We use the
eve.utils.CustomMapping
class introduced by #2062 to cache the offset providers in a dictionary and ensure that the key is computed in the same way at compile time and at runtime. This solution is used in two places: the cache for the full offset provider, that contains user-defined and implicit offset providers; and the cache for offset provider type.