-
Notifications
You must be signed in to change notification settings - Fork 11
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
single_version_only for actx.compile #228
base: main
Are you sure you want to change the base?
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.
LGTM, thanks! We might need to move this interface to ArrayContext.compile
or might lead to illegal codes for certain array contexts.
from .compile import LazilyPyOpenCLCompilingFunctionCaller | ||
return LazilyPyOpenCLCompilingFunctionCaller(self, f) | ||
return LazilyPyOpenCLCompilingFunctionCaller(self, |
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.
Maybe also handle the other sub-classes of BaseLazilyCompilingFunctionCaller?
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.
Added in 5ed25a5
arraycontext/impl/pytato/compile.py
Outdated
@@ -262,6 +262,8 @@ class BaseLazilyCompilingFunctionCaller: | |||
program_cache: Dict["PMap[Tuple[Any, ...], AbstractInputDescriptor]", | |||
"CompiledFunction"] = field(default_factory=lambda: {}) | |||
|
|||
single_version_only: bool = False |
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the default in 5ed25a5
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Maybe then we introduce the default as you suggested, but have a deprecation period attached to it?
@@ -520,6 +521,8 @@ def compile(self, f: Callable[..., Any]) -> Callable[..., Any]: | |||
it may be called only once (or a few times). | |||
|
|||
: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we save ourselves having to find arg_id_to_descr
?
No description provided.