Conversation
kaushikcfd
left a comment
kaushikcfd
left a comment
There was a problem hiding this comment.
LGTM, thanks! We might need to move this interface to ArrayContext.compile or might lead to illegal codes for certain array contexts.
| single_version_only: bool = False) -> Callable[..., Any]: | ||
| from .compile import LazilyPyOpenCLCompilingFunctionCaller | ||
| return LazilyPyOpenCLCompilingFunctionCaller(self, f) | ||
| return LazilyPyOpenCLCompilingFunctionCaller(self, |
There was a problem hiding this comment.
Maybe also handle the other sub-classes of BaseLazilyCompilingFunctionCaller?
arraycontext/impl/pytato/compile.py
Outdated
| program_cache: Dict["PMap[Tuple[Any, ...], AbstractInputDescriptor]", | ||
| "CompiledFunction"] = field(default_factory=lambda: {}) | ||
|
|
||
| single_version_only: bool = False |
There was a problem hiding this comment.
I'm worried this might lead to developer errors. I would prefer if we don't default and ensure that the current version is passed during instantiation.
There was a problem hiding this comment.
This fails in grudge now:
File "wave-mpi-lazy.py", line 201, in main
compiled_rhs = actx.compile(rhs)
^^^^^^^^^^^^^^^^^
File "/home/runner/work/arraycontext/arraycontext/mirgecom/src/grudge/grudge/array_context.py", line 416, in compile
return _DistributedLazilyPyOpenCLCompilingFunctionCaller(self, f)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: BaseLazilyCompilingFunctionCaller.__init__() missing 1 required positional argument: 'single_version_only'
There was a problem hiding this comment.
Hmm... Maybe then we introduce the default as you suggested, but have a deprecation period attached to it?
|
|
||
| :arg f: the function executing the computation. | ||
| :arg single_version_only: If *True*, raise an error if *f* is compiled | ||
| more than once (due to different input argument types). |
There was a problem hiding this comment.
- This needs to explain quite a bit more context, such as by explaining what triggers a recompile.
- I'm not sure I like "version" as a noun here.
arraycontext/impl/pytato/compile.py
Outdated
| @@ -324,7 +325,10 @@ def __call__(self, *args: Any, **kwargs: Any) -> Any: | |||
| try: | |||
| compiled_f = self.program_cache[arg_id_to_descr] | |||
There was a problem hiding this comment.
Can we save ourselves having to find arg_id_to_descr?
There was a problem hiding this comment.
If I remember correctly, this was the main goal of implementing single_version_only?
What do you think of the approach in d8071ea?
|
@matthiasdiener Could you measure the impact this has on launch overhead, for mirgecom-like kernels? This would help in deciding how to prioritize this. |
With the current version (as of b3c69c7), this has a measurable impact on the time step time for smaller examples: Sod, 1 rank, with
Unfortunately, I did not see any meaningful difference in time step time for large drivers, such as |
3 participants
No description provided.