-
Notifications
You must be signed in to change notification settings - Fork 158
Fix lru_cache type-conflation in dace.symbolic.simplify
#2404
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
Open
Spielopoly
wants to merge
6
commits into
spcl:main
Choose a base branch
from
Spielopoly:symbolic_lru_cache_fix
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+111
−1
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
09c6c9c
Fix lru_cache type-conflation in symbolic.simplify
Spielopoly e03f031
Add test for previous fix
Spielopoly 0ce4c1d
[fortran] Fix fparser circular import on Python 3.14
ThrudPrimrose 9c53f42
Merge branch 'fix-fparser' into symbolic_lru_cache_fix
Spielopoly 8d39ae1
make tests more lenient so they cover both sympy and python types ins…
Spielopoly 5361ce5
Merge branch 'main' into symbolic_lru_cache_fix
Spielopoly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| # Copyright 2019-2026 ETH Zurich and the DaCe authors. All rights reserved. | ||
| """ | ||
| Regressions tests to ensure LRU cache is type-aware | ||
| """ | ||
|
|
||
| import sympy | ||
| from sympy.logic.boolalg import Boolean | ||
|
|
||
| import dace | ||
|
|
||
|
|
||
| def _fresh_cache() -> None: | ||
| """Clear the ``simplify`` cache so each test is order independent.""" | ||
| dace.symbolic.simplify.cache_clear() | ||
|
|
||
|
|
||
| def test_simplify_bool_does_not_poison_integer_one(): | ||
| """``simplify(True)`` first must not turn ``simplify(Integer(1))`` boolean.""" | ||
| _fresh_cache() | ||
| # Caches sympy.true; on an untyped cache this poisons the key ``1``. | ||
| dace.symbolic.simplify(True) | ||
|
|
||
| result = dace.symbolic.simplify(sympy.Integer(1)) | ||
| assert isinstance(result, (sympy.Integer, int)), \ | ||
| f"expected sympy.Integer, got {type(result).__name__}: {result!r}" | ||
| assert not isinstance(result, (Boolean, bool)) | ||
| assert result == 1 | ||
|
Spielopoly marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def test_simplify_integer_one_does_not_poison_bool(): | ||
| """The reverse order: a cached ``Integer(1)`` must not leak into ``simplify(True)``.""" | ||
| _fresh_cache() | ||
| dace.symbolic.simplify(sympy.Integer(1)) | ||
|
|
||
| result = dace.symbolic.simplify(True) | ||
| assert isinstance(result, (Boolean, bool)), \ | ||
| f"expected a sympy boolean, got {type(result).__name__}: {result!r}" | ||
| assert bool(result) is True | ||
|
Spielopoly marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def test_simplify_bool_does_not_poison_integer_zero(): | ||
| """Same conflation exists for ``False``/``0`` -- integer direction stays typed.""" | ||
| _fresh_cache() | ||
| dace.symbolic.simplify(False) | ||
|
|
||
| result = dace.symbolic.simplify(sympy.Integer(0)) | ||
| assert isinstance(result, (sympy.Integer, int)), \ | ||
| f"expected sympy.Integer, got {type(result).__name__}: {result!r}" | ||
| assert not isinstance(result, (Boolean, bool)) | ||
| assert result == 0 | ||
|
|
||
|
|
||
| def test_simplify_integer_zero_does_not_poison_bool(): | ||
| """``False``/``0`` -- boolean direction stays boolean.""" | ||
| _fresh_cache() | ||
| dace.symbolic.simplify(sympy.Integer(0)) | ||
|
|
||
| result = dace.symbolic.simplify(False) | ||
| assert isinstance(result, (Boolean, bool)), \ | ||
| f"expected a sympy boolean, got {type(result).__name__}: {result!r}" | ||
| assert bool(result) is False | ||
|
Spielopoly marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def test_simplify_symbolic_expressions_still_cache(): | ||
| """Normal symbolic inputs are unaffected and are still served from the cache.""" | ||
| _fresh_cache() | ||
| n = sympy.Symbol('N') | ||
| assert dace.symbolic.simplify(n + 1 - n) == 1 | ||
| assert dace.symbolic.simplify((n**2 - 1) / (n - 1)) == n + 1 | ||
| # Same expression again: served from the cache with the same result. | ||
| assert dace.symbolic.simplify(n + 1 - n) == 1 | ||
| assert dace.symbolic.simplify.cache_info().hits >= 1 | ||
|
|
||
|
|
||
| def test_volume_propagation_after_bool_simplify(): | ||
| """End-to-end: a ``bool`` fed to ``simplify`` must not break memlet-volume | ||
| propagation on an unrelated SDFG (the original order-dependent crash).""" | ||
| from dace.sdfg.propagation import propagate_memlets_sdfg | ||
|
|
||
| _fresh_cache() | ||
| # The poisoning call pattern: a concrete shape comparison yields a Python | ||
| # ``bool``, which simplifies to ``sympy.true``. | ||
| poison = dace.symbolic.simplify(sympy.Integer(1) == 1) | ||
| assert bool(poison) is True | ||
|
|
||
| sdfg = dace.SDFG('simplify_cache_volume_repro') | ||
| sdfg.add_array('A', [4], dace.float64) | ||
| sdfg.add_array('B', [4], dace.float64) | ||
| state = sdfg.add_state() | ||
| # A single-iteration map: the propagated volume is ``sympy.Integer(1)`` -- | ||
| # exactly the cache key that a cached ``simplify(True)`` poisons. | ||
| state.add_mapped_tasklet('copy', | ||
| dict(i='0:1'), | ||
| dict(inp=dace.Memlet('A[i]')), | ||
| 'out = inp', | ||
| dict(out=dace.Memlet('B[i]')), | ||
| external_edges=True) | ||
| # Without the fix this raised: TypeError: Property volume must be a literal | ||
| # or symbolic expression (the propagated volume was a BooleanTrue). | ||
| propagate_memlets_sdfg(sdfg) | ||
| sdfg.validate() | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| test_simplify_bool_does_not_poison_integer_one() | ||
| test_simplify_integer_one_does_not_poison_bool() | ||
| test_simplify_bool_does_not_poison_integer_zero() | ||
| test_simplify_integer_zero_does_not_poison_bool() | ||
| test_simplify_symbolic_expressions_still_cache() | ||
| test_volume_propagation_after_bool_simplify() | ||
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.
Uh oh!
There was an error while loading. Please reload this page.