fix[next]: deterministic declaration order in gtfn codegen - #2779
Conversation
The gtfn lowering collected dimensions, cartesian offsets and shift offsets into
`set`s and then iterated them to build the generated source. Set iteration order
for these elements varies with `PYTHONHASHSEED`, so the same program lowered in
two processes could emit the same declarations in a different order.
The build cache is keyed on a fingerprint of the generated source, so this made
affected programs miss the cache on every fresh process.
Observed on the icon4py dycore and diffusion benchmarks: across three runs, 5-6
of 67 programs differed, always as a pure permutation of the dimension tag block
- using Edge_t = gtfn::unstructured::dim::horizontal;
+ using Vertex_t = gtfn::unstructured::dim::horizontal;
and one program produced three distinct fingerprints in three runs. Only programs
with two or more distinct horizontal domains are affected.
Derive the order from the IR instead, at all three collection sites:
- `_get_domains` drove the observed differences.
- `_collect_offset_definitions` feeds the same `offset_definitions` dict, so it
has the same defect on cartesian grids, which this workload never exercised.
- `_collect_offset_or_axis_node` returns the shift offsets its caller turns into
the generated `connectivities` list.
| ) | ||
| ) | ||
| return result | ||
| return list(result) |
There was a problem hiding this comment.
What about returning just an iterable (result.keys()) as in _get_domains()?
tehrengruber
left a comment
tehrengruber
left a comment
There was a problem hiding this comment.
I would just use OrderedSet which we have as a requirement anyway and is a drop-in replacement for set.
OrderedSet is already a requirement and is a drop-in replacement for set, so it says what is meant more directly than threading order through a dict. Returning the OrderedSet also lets `_collect_offset_or_axis_node` hand back an iterable rather than a materialized list, matching `_get_domains`.
|
Switched all three sites to @edopao on returning an iterable: Verified after the rewrite: the two regression tests still fail against |
|
Comments tightened to a single one on On whether this over-orders: I checked, and all three do reach the generated code.
Worth being explicit about the evidence, though: only So: correct as far as I can tell, but two thirds of it is reasoned rather than measured. |
An ordering assertion only fails when the unordered iteration happens to differ from the expected order, so it is flaky by construction and gives little confidence in exchange.
|
Dropped the tests. An ordering assertion only fails when the unordered iteration happens The PR is now source-only: |
|
The description is not correct:
The translation cache side-steps this. I'm noticing wrong descriptions quite a bit recently and I fear that this deteriorate human and agentic performance. At least I frequently resort to commit messages (let aside their importance for review). |
|
I usually cleanup the description before merge, but I am not sure what's better for review. The details (with maybe partially wrong reasoning, which might highlight also wrong implementation) or strip to the very concise high level description that I would usually write. Happy to discuss. (And with the last sentence you don't know who wrote this. ;) ) |
…k pass `_eliminate()` uses the `FindAccessNodes` result instead of scanning every state. The pass returns a `set` per state and its iteration order decides in which order the new AccessNodes are inserted, so the loop sorts by `state.node_id`, see GridTools#2779 and GridTools#2780. `_accesses_region()` uses the shared `maybe_intersecting()` and the removal of `T` is validated under the GT4Py debug flag rather than DaCe's. The class docstring described the requirement on `G` as a window between the definition of `T` and the write back. It has to hold from the definition onwards, which is also what serves a consumer that reads after the write back; that was enforced but left to be inferred. It is now stated as the simplification it is. The comment on the subset size was wrong: `Range.size()` does divide by the step. The check holds because the source subset equals `Range.from_array()` and so has unit steps, which the comment now says.
3 participants
The gtfn lowering collected dimensions, cartesian offsets and shift offsets into sets and iterated them while building the generated source.
The fix replaces the set by an OrderedSet at the three collectors. Only the one in _get_domains is backed by observed diffs; the other two are the same defect on inputs the test runs did not reach, namely several cartesian dimensions and more than one connectivity offset in a stencil.