feat[dace][next] More Roboust Calling #2353

philip-paul-mueller · 2025-10-30T10:59:57Z

This PR changes how calls to the underlying CompiledSDFG objects are carried out.
Before, the implementation heavily relied on the internal data format of the DaCe class.
However a recent change in DaCe changed this internal data leading to errors, that had to be patched.
This PR introduces a more stable fix and builds upon a refactoring in DaCe that beside other things, exposes the tools that were needed by GT4Py to work independently of the internals.
For that reason this PR also updates the DaCe dependency to 2025.11.04.

The main change is, that the argument vector, i.e. the C representation of the arguments used for the call, are no longer managed by CompiledSDFG but instead by GT4Py's CompiledDaceProgram.

TODO:

ICON4Py CI must pass: DO NOT MERGE: Test New Calling Convention in GT4Py/DaCe C2SM/icon4py#925
DaCe PR#2206 must be included.

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py

…t_call_scheme

philip-paul-mueller · 2025-10-30T13:11:25Z

Please ignore changes in pyproject.toml they are just needed that the CI picks up the patched DaCe version.

…t_call_scheme

edopao

Very nice!

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py

edopao · 2025-10-31T15:13:42Z

pyproject.toml

 ]
 dace-next = [
-  'dace==2025.10.30'  # refined in [tool.uv.sources]
+  'dace==2025.05.35'  # refined in [tool.uv.sources] and I like that book.


What book? :)

It is a German children book and serves as a reminder for me that I have to undo these changes.
You see I needed a version/date that will never be selected for this hack.

…tion.py Co-authored-by: edopao <edoardo16@gmail.com>

egparedes

Just a few comments.

egparedes · 2025-11-03T11:45:19Z

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

+    #  `tuple` is empty then it is not initialized. The first element is used in the
+    #  normal call and we update it, the second element is used only for initialization
+    #  and we do not update it.
+    csdfg_args: Union[tuple[list[Any], Sequence[Any]], tuple[()]]


Why not just using None as usual for an uninitialized attribute?

Suggested change

csdfg_args: Union[tuple[list[Any], Sequence[Any]], tuple[()]]

csdfg_args: tuple[list[Any], Sequence[Any]] | None

I think changing it into a None is actually better.
Because in the first call it will raise a TypeError instead an IndexError.
And this might hide some other bugs in the update code.

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

egparedes · 2025-11-03T12:05:07Z

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

+            processed_csdfg_args = self.sdfg_program.construct_arguments(**kwargs)
+        assert isinstance(processed_csdfg_args, tuple) and len(processed_csdfg_args) == 2
+        # Note we only care about the first argument vector, that is used in normal call.
+        #  Since we update it, we ensure that it is a `list`.
+        self.csdfg_args = (list(processed_csdfg_args[0]), processed_csdfg_args[1])


Using explicit unpacking achieves the same effect as the assert and it makes the coder nicer to read.
I wouldn't store the second argument if it is not needed.

Suggested change

processed_csdfg_args = self.sdfg_program.construct_arguments(**kwargs)

assert isinstance(processed_csdfg_args, tuple) and len(processed_csdfg_args) == 2

# Note we only care about the first argument vector, that is used in normal call.

# Since we update it, we ensure that it is a `list`.

self.csdfg_args = (list(processed_csdfg_args[0]), processed_csdfg_args[1])

# We only care about the first value, which stores the arguments used in a normal call.

csdfg_call_args, _ = self.sdfg_program.construct_arguments(**kwargs)

self.csdfg_args =[*csdfg_call_args]

Okay there are two things here.
First, I told you that we do not need the initialization arguments, there I was wrong.
We only need it the first time we do the call and then never again, we do not even update it.
So we could technically remove it, but I think it would require some additional logic that makes the code harder to understand so I would propose to keep it.
If you really want it to be removed, I can do that.

The second thing is, that I fully agree with the unpacking.

egparedes · 2025-11-03T12:08:13Z

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

+        # If not called they yet, thus empty tuple.
+        self.csdfg_args = ()
+
+    def process_arguments(self, **kwargs: Any) -> None:


I find the name of this function not informative at all because it's too generic. What do you think about something like prepare_args_list ?

You are right the name is not that good, however, I am also not super happy about the prepare_args_list.
I would suggest prepare_arguments.

egparedes · 2025-11-03T12:08:42Z

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

    def fast_call(self) -> None:
-        result = self.sdfg_program.fast_call(*self.sdfg_program._lastargs)
+        """Perform a call to the compiled SDFG using the processed arguments, see `self.process_arguments()`."""
+        assert isinstance(self.csdfg_args, tuple) and len(self.csdfg_args) == 2


Suggested change

assert isinstance(self.csdfg_args, tuple) and len(self.csdfg_args) == 2

I would keep that because it tests if prepare_arguments() has been called or not.
I know you get an error in the unpacking, but I would prefer some more information about that.
However, I added a description to the assert.

egparedes · 2025-11-03T12:10:40Z

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py

-            #  so we just update it with the current call arguments.
-            update_sdfg_call_args(args, fun.sdfg_program._lastargs[0])
-            fun.fast_call()
+            # Not the first call. We will only update the first argument vector (normal call).


Suggested change

# Not the first call. We will only update the first argument vector (normal call).

# Not the first call. We will only update the first argument list (normal call).

As mentioned below.

egparedes · 2025-11-03T12:14:21Z

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

        None,
    ]

+    # Processed argument vectors that are passed to `CompiledSDFG.fast_call()`. If the


I don't think argument vectors is the best name for this. In Python arguments are tuples or any other sequence which can be unpacked, so arguments list or arguments sequence feels more appropriate and less awkward.

I understand your reasoning, however, I think that in this case vector is the better name, because we go to C and there we have agrv (okay we do not call main()).

egparedes · 2025-11-03T12:14:58Z

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py

-            fun.fast_call()
+            # Not the first call. We will only update the first argument vector (normal call).
+            # NOTE: If this is the first call then we will generate an exception because
+            #   `fun.csdfg_args` is still the empty tuple.


Suggested change

# `fun.csdfg_args` is still the empty tuple.

# `fun.csdfg_args` is still the empty

edopao

LGTM

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

edopao · 2025-11-04T08:20:56Z

pyproject.toml

+[[tool.mypy.overrides]]
+disallow_incomplete_defs = false
+disallow_untyped_defs = false
+module = 'gt4py.next.program_processors.runners.dace_iterator.*'
+


Remember to remove these lines.

@philip-paul-mueller did you actually remove these lines and the "and I like that book" comment of the dace-next version before merging?

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py

egparedes · 2025-11-04T09:33:20Z

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

+        assert isinstance(self.csdfg_args, tuple) and len(self.csdfg_args) == 2, (
+            "Argument vector was not set properly."
+        )
+        self.sdfg_program.fast_call(*self.csdfg_args)


Suggested change

self.sdfg_program.fast_call(*self.csdfg_args)

self.sdfg_program.fast_call(self.csdfg_argv, self.csdfg_init_argv)

egparedes

LGTM

edopao · 2025-11-04T10:36:02Z

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

            "Argument vector was not set properly."
        )
-        self.sdfg_program.fast_call(*self.csdfg_args)
+        self.sdfg_program.fast_call(self.csdfg_argv, self.csdfg_init_argv, do_gpu_check=False)


Are we really sure we want to pass do_gpu_check=False? Do you know what is its overhead? Maybe we could pass do_gpu_check=__debug__?

@philip-paul-mueller , any thoughts on this comment?

#2365

We should have added something here.

…t_call_scheme

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py

Started with more permanent solution.

78b8dce

philip-paul-mueller requested review from edopao and egparedes October 30, 2025 10:59

edopao reviewed Oct 30, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py Outdated Show resolved Hide resolved

edopao reviewed Oct 30, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py Outdated Show resolved Hide resolved

edopao reviewed Oct 30, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py Outdated Show resolved Hide resolved

philip-paul-mueller added 2 commits October 30, 2025 14:07

Lets see if the thing works with the new changes in DaCe.

e6133ce

Merge remote-tracking branch 'gt4py/main' into non_dace_dependent_fas…

366293b

…t_call_scheme

This was referenced Oct 30, 2025

[DO NOT MERGE]: Hotfix for SDFG Calling. GridTools/dace#9

Closed

Do Not Merge: Integration Branch for GT4Py Next GridTools/dace#1

Draft

philip-paul-mueller added 3 commits October 30, 2025 14:37

Modified the tests.

17be22e

Applied Edoaro's comments.

c228fc8

Update the dependency for proper testing, remember to remove afterwards.

dab085b

philip-paul-mueller mentioned this pull request Oct 31, 2025

DO NOT MERGE: Test New Calling Convention in GT4Py/DaCe C2SM/icon4py#925

Closed

philip-paul-mueller added 3 commits October 31, 2025 14:02

Fixed a test.

5280c3a

Merge remote-tracking branch 'gt4py/main' into non_dace_dependent_fas…

4ab5227

…t_call_scheme

Switched the dependency to the newest version of my PR.

efb562a

philip-paul-mueller marked this pull request as ready for review October 31, 2025 15:08

edopao reviewed Oct 31, 2025

View reviewed changes

Update src/gt4py/next/program_processors/runners/dace/workflow/decora…

82fae60

…tion.py Co-authored-by: edopao <edoardo16@gmail.com>

egparedes reviewed Nov 3, 2025

View reviewed changes

philip-paul-mueller added 2 commits November 4, 2025 08:33

Addressed Enrique's comments.

b945c28

Updated the tests.

969405a

philip-paul-mueller requested review from edopao and egparedes November 4, 2025 07:44

edopao reviewed Nov 4, 2025

View reviewed changes

egparedes reviewed Nov 4, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py Outdated Show resolved Hide resolved

egparedes reviewed Nov 4, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py Outdated Show resolved Hide resolved

egparedes reviewed Nov 4, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace/workflow/decoration.py Outdated Show resolved Hide resolved

egparedes reviewed Nov 4, 2025

View reviewed changes

egparedes approved these changes Nov 4, 2025

View reviewed changes

philip-paul-mueller added 3 commits November 4, 2025 11:06

Addressed next round of reviewes, tests not modified.

02e7375

Updated the tests.

94332f0

Updated DaCe dependency.

cec9e08

edopao reviewed Nov 4, 2025

View reviewed changes

Merge remote-tracking branch 'gt4py/main' into non_dace_dependent_fas…

45ecdb7

…t_call_scheme

philip-paul-mueller merged commit 08213ea into GridTools:main Nov 4, 2025
30 of 31 checks passed

egparedes reviewed Nov 6, 2025

View reviewed changes

src/gt4py/next/program_processors/runners/dace/workflow/compilation.py Show resolved Hide resolved

	csdfg_args: Union[tuple[list[Any], Sequence[Any]], tuple[()]]
	csdfg_args: tuple[list[Any], Sequence[Any]] \| None

	# Not the first call. We will only update the first argument vector (normal call).
	# Not the first call. We will only update the first argument list (normal call).

	# `fun.csdfg_args` is still the empty tuple.
	# `fun.csdfg_args` is still the empty

	self.sdfg_program.fast_call(*self.csdfg_args)
	self.sdfg_program.fast_call(self.csdfg_argv, self.csdfg_init_argv)

feat[dace][next] More Roboust Calling #2353

feat[dace][next] More Roboust Calling #2353

Uh oh!

Conversation

philip-paul-mueller commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philip-paul-mueller commented Oct 30, 2025

Uh oh!

edopao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philip-paul-mueller Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egparedes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egparedes Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edopao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egparedes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philip-paul-mueller Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

philip-paul-mueller commented Oct 30, 2025 •

edited

Loading

philip-paul-mueller Oct 31, 2025 •

edited

Loading

egparedes Nov 3, 2025 •

edited

Loading

philip-paul-mueller Nov 6, 2025 •

edited

Loading