-
Notifications
You must be signed in to change notification settings - Fork 54
fix[next]: Fix different static args after with_backend
#2475
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
fix[next]: Fix different static args after with_backend
#2475
Conversation
egparedes
left a comment
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.
It looks good. Just some typos questions and code suggestions. It should be approved in the next round.
| #: If the user requests static params, they will be used later to initialize CompiledPrograms. | ||
| #: By default the set of static params is set when compiling for the first time, e.g. on call | ||
| #: when jitting is enabled, or on a call to `compiled`. | ||
| static_params: Sequence[str] | None = None |
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.
Question: wouldn't it be simpler to just default to an empty tuple?
| static_params: Sequence[str] | None = None | |
| static_params: Sequence[str] = () |
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 first changed it to be (), but I found it hard to understand under which circumstances it is ok for this value to be different than what is inside the compiled programs pool. I therefore went back to None here and set the value to () in _compiled_programs when the value is actually selected. I'll change it back when you like.
| else: | ||
| assert all(p is not None for p in static_params) | ||
| _static_params = typing.cast(tuple[str], static_params) | ||
| _static_params = typing.cast(tuple[str, ...], static_params) |
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.
Question: do you understand why the cast is used/needed here after the assert?
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.
Unclear:
error: Incompatible types in assignment (expression has type "tuple[str | None, ...]", variable has type "tuple[str, ...]")
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.
Ah, I get it. mypy does not understand the all() inside the assert. A custom typeguard would be needed here, but it can be done in a different PR.
src/gt4py/next/ffront/decorator.py
Outdated
| def with_connectivities( | ||
| self, | ||
| connectivities: common.OffsetProvider, # TODO(ricoh): replace with common.OffsetProviderType once the temporary pass doesn't require the runtime information | ||
| def with_compilation_option( |
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.
| def with_compilation_option( | |
| def with_compilation_options( |
or compile_options() I don't care
egparedes
left a comment
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
The following snippet broke after #2368
with the following error
This PR avoids using
object.__setattr__on theCompilationOptionsto pass information to the compiled program pool construction and instead explicitly constructs the pool oncompile.Note that the
enable_jitargument tocompilehas been removed and replaced by:as the same
object.__setattr__mechanism was used to pass the information.