fix[next]: Deterministic dace auto optimize by tehrengruber · Pull Request #2568 · GridTools/gt4py

tehrengruber · 2026-04-09T19:52:03Z

This PR is a first vertical slice to make auto_optimize deterministic. The general idea is to use ordered datastructures everywhere where order matters (e.g. when iterating over elements of a set) and provide an easy to follow workflow to debug and resolve indeterminism (see here). In many places this just means replacing set with OrderedSet from https://github.com/rspeer/ordered-set and using deterministic naming schemes. In addition to the changes here GridTools/dace#17 is needed, which also introduces an id property in all dace nodes. The id property is computed from a deterministic counter incremented whenever a new node is constructed. Together with the reset_node_id_counter context manager each node gets a unique id, which is stable across runs. This was initially meant as a way to order output from networkx algorithms used in dace that return sets, but surprisingly this was not needed for the stencil used to test this PR. Since this can be different for other stencils and since the id value is also very useful to quickly recognize that order has changed this is kept for now.

Open discussion points:

Is OrderedSet the right package?
How should we test this? Using icon4py or a fuzzer?
Guidelines for symbol name generation

The program used to test this is PR:
icon4py @ 0918c3d with model/atmosphere/dycore/tests/dycore/stencil_tests/test_compute_advection_in_vertical_momentum_equation.py::TestFusedVelocityAdvectionStencilVMomentum::test_TestFusedVelocityAdvectionStencilVMomentum[compile_time_domain]

philip-paul-mueller

Generally looks okay, there are some improvements possible.

philip-paul-mueller · 2026-04-10T08:53:05Z

+  ```
+  TODO: introduce new config var that prints the hash instead of hard-coding it.
+- Execute the program in question twice and compare the output.
+- Set a conditonal breakpoint in beginning of the `apply` method of the first pass where the SDFG


apply() is something that is specific to the PatternTransformation.
Not all transformations use them (but the majority does).
The most general interface is given by Pass, whcih defines the apply_pass() method.
But as mentioned above, also functions can be transformations.

philip-paul-mueller · 2026-04-10T08:54:19Z

+with open(file) as f:
+    data = json.load(f)
+    sdfg = dace.SDFG.from_json(data)
+    print_sdfg_hash(sdfg)


There is also dace.SDFG.from_file().

philip-paul-mueller · 2026-04-10T09:02:38Z

I looked at this file and it looks okay, although the O(N) deletion is not nice.
One could rework the algorithm to get rid of them, however I would not do this.
There is PR#2531 which updates this file and also tries to avoid in determinism.
While it still uses sets it sorts the nodes ones in a deterministic way (depending on the node order upon input).

philip-paul-mueller · 2026-04-10T09:53:47Z

-    first_map_params = set(first_map.params)
-    second_map_params = set(second_map.params)
+    # TODO(tehrengruber): The structure here looks a little funky. We just use an ordered set for
+    #  now, but likely no sets are needed at all.
+    first_map_params = OrderedSet(first_map.params)
+    second_map_params = OrderedSet(second_map.params)
    if first_map_params != second_map_params:
        return None


sets are needed here, at least for the check that follows because a Map with parameters ["i", "j"] is the same as one with parameters ["j", "i"].
However, you can do something like:

first_map_params = sorted(first_map.params) second_map_params = sorted(second_map.params)

then you can also remove the sorted() calls bellow as well.

philip-paul-mueller · 2026-04-10T10:00:16Z

        # Now we will reroute the edges went through the inner map, through the
        #  inner access node instead.
-        for old_inner_edge in list(
+        for old_inner_edge in list(  # TODO(tehrengruber): Why all these list comprehensions everywhere?


It is needed because you replace the edges (removing the old and adding a new one).
Furthermore, the by_connector() gives you back an iterator thus modyfing the edges will change your iteration source, which leads to a race condition.

philip-paul-mueller · 2026-04-10T11:13:03Z

+
+- Enable printing each transformation step, e.g. using
+  ```
+  dace.Config.set("progress", value=True)


This will only give you like ~95% of the cases, as transformation can also run through other means that the patter matcher or can be simple functions that do things.

In order to avoid editing the code and adding this line, you can export an environment variable:
export DACE_progress=1

(note the the upper/lower case unfortunately matters)

philip-paul-mueller · 2026-04-10T11:15:14Z

+
+    sdfg.apply_transformations_repeated(
+        transformation(
+            ignore_upstream_blocks=False,


This should be True for the correct working.

philip-paul-mueller · 2026-04-10T12:48:06Z

                sdfg=copy.deepcopy(node.sdfg),
-                inputs=set(node.in_connectors.keys()),
-                outputs=set(node.out_connectors.keys()),
+                # TODO(tehrengruber): What is the performance optimization from Philip about?


What do you mean with my performance optimization?
What cold be a problem is the copying node.sdfg might also copy the surrounding SDFG, because nested SDFGs have a reference to their parent SDFG.

philip-paul-mueller · 2026-04-10T12:49:07Z

+                inputs={k: None for k in node.in_connectors.keys()},
+                outputs={k: None for k in node.out_connectors.keys()},


This should be equivelent since the data types should not change.

Suggested change

inputs={k: None for k in node.in_connectors.keys()},

outputs={k: None for k in node.out_connectors.keys()},

inputs=node.in_connectors.copy().

outputs=node.out_connectors.copy(),

It you want to play it safe ignore this suggestion.

philip-paul-mueller · 2026-04-10T13:28:51Z

        **kwargs: Any,
    ) -> dace.SDFG:
-        with gtx_wfdcommon.dace_context(device_type=self.device_type):
+        with gtx_wfdcommon.dace_context(device_type=self.device_type), dace.sdfg.nodes.reset_node_id_counter():


I kind of understand why it is used here, although I do not like it.

edopao · 2026-04-14T09:09:43Z

    # NOTE: Each thread maintains its own set of configuration, i.e. `dace.Config` is
    #   a thread local variable. This means it is safe to set values that are different
    #   for each thread.
+    dace.Config.set("progress", value=True)


This is not meant to be merged, right?

edopao · 2026-04-14T09:10:51Z

+
+- Enable printing each transformation step, e.g. using
+  ```
+  dace.Config.set("progress", value=True)


In order to avoid editing the code and adding this line, you can export an environment variable:
export DACE_progress=1

(note the the upper/lower case unfortunately matters)

tehrengruber added 3 commits April 9, 2026 21:38

Deterministic dace toolchain

b4d6cba

Update uv.lock

1f39ce5

Cleanup

f303b26

philip-paul-mueller reviewed Apr 10, 2026

View reviewed changes

edopao reviewed Apr 14, 2026

View reviewed changes

tehrengruber added 3 commits April 15, 2026 13:15

Use deterministic name in FuseHorizontalConditionBlocks

3965eec

Fix init

565113f

Fix typo

4da9a84

		inputs={k: None for k in node.in_connectors.keys()},
		outputs={k: None for k in node.out_connectors.keys()},

Conversation

tehrengruber commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philip-paul-mueller 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tehrengruber commented Apr 9, 2026 •

edited

Loading