Conversation
Removed the '--break-system-packages' option from pip install command.
ThrudPrimrose
marked this pull request as draft
July 30, 2026 09:41
An access-node to access-node copy moves data without a tasklet, so `scope_misses` never accounted it. With simplification off the frontend leaves a slice in its own state, so that state contributed zero misses. The tasklet branch compensated by following a single incoming access node, which only works once the states are fused and which miscounted a single-input map besides, rewriting the element memlet to the map's whole-array memlet so every access landed on line 0. Measured on `y[:] = x * 2.0` at N=512 with simplification on: 65 misses before, 128 after, which is what two 64-line arrays should cost. Account the copy where it happens instead, and drop the redirection. Only element-wise copies count: `_edge_miss` models a single cache-line touch and says nothing about a bulk copy, which otherwise invents a miss that shifts four existing expectations. `dace.symbolic.simplify` is not idempotent on logs of composite integers -- `log(456)/log(2)` and `3 + log(57)/log(2)` map to each other -- so the shape of a work/depth result depends on how many times the traversal simplified it, which depends on the state count. The values were equal in both shapes; `expand()` just cannot factor a log of a composite. Compare by value, keeping the cheap structural check first, as the polybench work/depth test already does.
…e same name SymPy's global caches key on `_hashable_content`, and `symbol` left the dtype out of it. Two symbols with the same name and assumptions but different dtypes were therefore one symbol to every cache, and an expression built around either one was handed back for the other -- silently retyping it. This surfaced as `test_typed_binary_operator_roundtrip_preserves_serialization[expr4]`: once anything in the process had built a `Mod` over an int64 `i`, deserializing `Mod($i, 3i16)` returned that expression instead, and it reserialized as `Mod(symbol($i, dtype=dace.int64), 3i16)`. Which tests share a worker decides whether it fires, so the failure follows the test distribution rather than the change under it. `TypedConstant` already includes its dtype for the same reason. Hashing `ctype` rather than the typeclass keeps expressions orderable: SymPy compares these tuples element-wise and typeclasses define equality but no ordering.
…wo of the same name" This reverts commit 72e2a3a. The diagnosis holds -- same-name symbols of different dtypes are one symbol to every SymPy cache -- but the code presently depends on that. Making the dtype part of the hash makes equality and `in free_symbols` dtype-sensitive everywhere, and the places that compare a locally minted symbol against one carrying an SDFG's declared dtype stop matching: 49 failures across vectorization, SVE, write-set underapproximation, subgraph fusion and npbench, none of them related to this PR. Fixing it properly means giving a name one dtype per scope first, which is not this PR's subject. Reverting restores the pre-existing test-order flake in `test_typed_binary_operator_roundtrip_preserves_serialization[expr4]`, which belongs to main and predates these changes.
…tion DaCe symbol equality and hashing ignore dtype, so a SymPy @cacheit entry built from an equal-named, different-dtype symbol silently substitutes that symbol into any equal-key construction. After a bounded-cache eviction, deserialize_symbolic('Mod($i, 3i16)') could therefore return a Mod carrying an int64 'i', changing its serialization. Construct function applications in the serialized-form parser without the cache (raw Basic.__new__) whenever the arguments contain symbols; symbol-free arguments hash soundly and keep the regular constructors.
astunparse's _Attribute checks isinstance(t.value, ast.Num), but ast.Num was removed in Python 3.12, so unparsing any attribute access (e.g. np.inf, dace.float64) raised AttributeError. Override _Attribute in ExtUnparser with the equivalent ast.Constant check, which matches the old behavior on Python 3.10 and works on 3.12+.
Range.__init__ coerces every bound through tuple_to_symexpr, but
__setitem__ wrote the value in raw, so subset[i] = (lb, ub, step) with a
plain Python int silently broke the class invariant that bounds are
symbolic -- failing much later in unrelated passes ('int' object has no
attribute 'match'). Validate and coerce tuple writes (a 4-tuple also
updates the tile size) and coerce single-index scalar writes, matching
the constructor. Also make tuple_to_symexpr public per house naming.
Ranks of one job derive the same build folder, so ranks that each compile build on top of each other and can load a library another rank is still writing. Eight processes running one GPU test out of one folder failed six times; with a folder each, none. The new cache_distaware config entry names the build cache root after the rank the launcher (MPI, Flux, Slurm) advertises. It is off by default, because sharing one build is also a valid setup: distributed_compile has rank 0 build and every other rank load its folder. That path now pins the broadcast folder on the ranks that hold the SDFG, the others being free to pass None.
ThrudPrimrose
marked this pull request as ready for review
August 10, 2026 09:33
phschaad
approved these changes
Aug 10, 2026
Changed default value of cache_distaware from false to true.
Ranked-vs-shared assertions assumed distaware defaulted off, so clearing the env override fell through to the new true default and compared a rank-suffixed path against itself. Wrap the old off-path assertions in an explicit distaware=False context and add structural per-rank-root assertions for the new on-by-default behavior, for every cache mode.
The exact-leaf assertion assumed cache mode 'name'. A workflow whose DACE_cache resolves to anything else (env or a persisted config value the unlaunched fixture does not clear) flipped the leaf to a hash suffix and broke the path match. Pin it explicitly like the sibling tests pin their env, same env-wins-over-config precedence used to root-cause the distaware default flip.
tbennun
approved these changes
Aug 11, 2026
tbennun
left a comment
tbennun
left a comment
Collaborator
There was a problem hiding this comment.
Needs merge conflict resolution but otherwise good to go
All workers/ranks see every GPU and pile CUDA contexts onto device 0, which flakes as invalid device ordinal (101) under -n 32 on cscs CI.
acalotoiu
enabled auto-merge
August 11, 2026 16:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR modernizes the SDFG performance evaluation to support ControlFlowRegions, ensuring compatibility with the modern SDFG structure.
Key Changes
Verification
There are many lines of change, because the original code was using sympy directly and not symbolic.py causing many small issues and had to be updated completely. (THANK GOD agents can do those annoying tasks)
Most code was written by my student @alexanderfluck, I reviewed and did some minor additions, and it is already reviewed by me.