-
Notifications
You must be signed in to change notification settings - Fork 49
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
Experimental GTFN Executor Caching #1197
Conversation
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.
Since there are no tests that allow us to say with any confidence that this caching has no problem with false positives, there should be at least a way to switch it off. This should probably then be the default case on CI, if not the default case overall.
Even if it looks obvious now that it should do the right thing, there is nothing in place that ensures we would notice if it gets broken down the line.
I fully agree, do you have a proposal on how to disable the caching without to much effort? We could make the caching an option in the |
Let's go with the options in GTFNExecutor for now. However, we seem to be accumulating them already so we shouldn't wait too long on a workflow based solution. |
src/gt4py/next/otf/workflow.py
Outdated
|
||
|
||
@dataclasses.dataclass(frozen=True) | ||
class CachedWorkflow(Generic[StartT, EndT]): |
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.
Why is this not a Step subclass? It lives on the same level, has the same interoperability etc...
Either case, the current naming is Workflow
for the concept and XyzStep
for all the concrete implementations (Any workflow can be a step in a super-workflow anyway). So naming this Concrete implementation CachedWorkflow makes it potentially confusing.
# TODO(tehrengruber): as the frontend types contain lists they are | ||
# not hashable. As a workaround we just use content_hash here. |
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.
If this comment only explains why content_hash
is used here, it does not need to be a TODO. Else it should contain something like "make frontent types hashable" or at least "verify this is the best solution".
looks like the doctests have gone out of sync, but once they are fixed, it's ready to merge |
src/gt4py/next/otf/workflow.py
Outdated
if hash_ in self._cache: | ||
return self._cache[hash_] | ||
return self._cache.setdefault(hash_, self.workflow(inp)) |
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.
if hash_ in self._cache: | |
return self._cache[hash_] | |
return self._cache.setdefault(hash_, self.workflow(inp)) | |
try: | |
result = self._cache[hash_] | |
except KeyError: | |
result = self._cache[hash_] = self.workflow(inp) | |
return result |
suggested by @egparedes
A small caching mechanism that makes repeated execution of stencils with the GTFN backend much faster. We should integrate this in a better way in the OTF pipeline, but this makes live much easier for now.
I have left the caching strategy asSESSION
for now, but for people that know what they are doing running withcache.Strategy.PERSISTENT
can make executing all tests a breeze (time-wise :-)).The caching of the GTFN executor can now be configured:
This feature is experimental!
Extracted from #965