Skip to content

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

Merged
merged 28 commits into from
Jun 10, 2025

Conversation

edopao
Copy link
Contributor

@edopao edopao commented May 22, 2025

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.

@edopao edopao changed the title feat[next]: Fast hash for offset provider feat[next]: Hash function for offset provider May 23, 2025
tehrengruber
tehrengruber previously approved these changes May 23, 2025
@edopao
Copy link
Contributor Author

edopao commented May 27, 2025

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.

@edopao edopao closed this May 27, 2025
@edopao edopao reopened this May 27, 2025
@edopao edopao changed the title feat[next]: Hash function for offset provider fix[next]: Hash function for offset provider May 27, 2025
@edopao edopao marked this pull request as ready for review May 28, 2025 07:23
@edopao edopao requested a review from egparedes May 28, 2025 07:23
@edopao edopao changed the title fix[next]: Hash function for offset provider fix[next]: Dict-based cache for offset provider in program call May 30, 2025
@edopao edopao changed the title fix[next]: Dict-based cache for offset provider in program call fix[next]: Custom map for caching of offset provider in program call May 30, 2025
@edopao edopao requested a review from havogt June 3, 2025 11:00
@edopao edopao changed the title fix[next]: Custom map for caching of offset provider in program call fix[next]: Use CustomMapping for caching of offset provider in program call Jun 4, 2025
@edopao
Copy link
Contributor Author

edopao commented Jun 5, 2025

cscs-ci run default

Copy link
Contributor

@egparedes egparedes left a 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.

try:
op_extended = self._offset_provider_extended_cache[offset_provider]
except KeyError:
op_extended = {**self._implicit_offset_provider, **offset_provider}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
op_extended = {**self._implicit_offset_provider, **offset_provider}
op_extended = self._implicit_offset_provider |
offset_provider

Copy link
Contributor Author

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]

@edopao edopao requested a review from egparedes June 5, 2025 21:14
@havogt havogt dismissed tehrengruber’s stale review June 6, 2025 06:50

review was for a different implementation

Copy link
Contributor

@egparedes egparedes left a 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.

@edopao
Copy link
Contributor Author

edopao commented Jun 6, 2025

cscs-ci run default

@egparedes egparedes merged commit 00bffa5 into GridTools:main Jun 10, 2025
30 of 31 checks passed
@edopao edopao deleted the gtir-offset_provider-hash branch June 10, 2025 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants