Skip to content
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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

matthiasdiener
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@kaushikcfd kaushikcfd left a 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,
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in 5ed25a5

@@ -262,6 +262,8 @@ class BaseLazilyCompilingFunctionCaller:
program_cache: Dict["PMap[Tuple[Any, ...], AbstractInputDescriptor]",
"CompiledFunction"] = field(default_factory=lambda: {})

single_version_only: bool = False
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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'

Copy link
Collaborator

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?

arraycontext/context.py Show resolved Hide resolved
@matthiasdiener matthiasdiener self-assigned this Apr 26, 2023
@@ -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).
Copy link
Owner

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]
Copy link
Owner

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?

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