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

GT4Py programs as DaCe SDFGConvertibles #1527

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

kotsaloscv
Copy link
Contributor

This feature is needed for implementing DaCe orchestration, e.g. in ICON4Py.

@kotsaloscv kotsaloscv self-assigned this Apr 12, 2024
@havogt
Copy link
Contributor

havogt commented Apr 12, 2024

Just a general comment, I am wondering if DaCe offers alternatively the option to register the conversion functions, instead of having to make them part of the object. (E.g. like jax pytrees work) If only the current option is available I am wondering if we should go with a monkey patching approach to remove the dace import from unrelated modules. But this is for @egparedes to answer. (And we could think of the other option as a contribution to productice DaCe.)

@kotsaloscv
Copy link
Contributor Author

My first impression is that @havogt is right and the best option would be to enhance DaCe with support for registering SDFG convertible functions defined outside of the object. It would improve both the Gt4Py and DaCe codebases and would make this PR much cleaner. Another option, even simpler to implement in the DaCe side, would be to remove the requirement to inherit from SDFGConvertible (which is not even an ABC) and implement it as a typing Protocol using runtime_checkable or a custom typeguard for isinstance() checks.

If we want to merge this PR in the short term before waiting for changes in DaCe, I think it'd be better to add the required DaCe methods and bases conditionally and keep DaCe as an optional requirement, as @edopao and @havogt suggested. For example:

class Program:
    ....

if dace:
    class Program(Program, SDFGConvertible):
        def __sdfg__(self, *args, **kwargs) -> Optional[Any]:
             ....

For now, I would go with the short term approach since we are in the testing/development phase. In the near future, I could create a PR in DaCe to address this topic.

cc: @havogt @edopao

Copy link
Contributor

@havogt havogt left a comment

Choose a reason for hiding this comment

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

I think we are getting closer. I am still trying to understand how things interact and where stuff belongs. Maybe we could have a chat and you could teach me the interface.

src/gt4py/next/ffront/decorator.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/decorator.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/decorator.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/decorator.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/decorator.py Outdated Show resolved Hide resolved
src/gt4py/next/embedded/nd_array_field.py Outdated Show resolved Hide resolved
@kotsaloscv kotsaloscv requested a review from havogt April 29, 2024 07:17
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.

None yet

4 participants