Fix C-cache bug related to input order of nominal variables #1673
Conversation
ricardoV94
force-pushed
the
fix_ccache_bug
branch
3 times, most recently
from
e3668de to
593038b
Compare
ricardoV94
force-pushed
the
fix_ccache_bug
branch
from
593038b to
3b3c110
Compare
|
PS I don't love the shared baseclass of Constant and NominalVariables as "atomics". They are more like root variables (always required) than constants. The only difference between nominal and generic root variables is that they evaluate equal based on the "position / type" instead of id. from pytensor.graph.basic import NominalVariable
from pytensor.tensor.type import TensorType
ScalarType = TensorType(dtype="float64", shape=())
n1 = NominalVariable(0, ScalarType)
n2 = NominalVariable(0, ScalarType)
n3 = NominalVariable(1, ScalarType)
t1 = ScalarType()
t2 = ScalarType()
assert n1 == n2 != n3 and t1 != t2That's however tangential to the bug here. It could be emulated just with regular variables and constants, as in the first test |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1673 +/- ##
=======================================
Coverage 81.63% 81.63%
=======================================
Files 242 242
Lines 53569 53569
Branches 9451 9451
=======================================
Hits 43732 43732
- Misses 7360 7362 +2
+ Partials 2477 2475 -2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a C-cache bug related to the input order of nominal variables in PyTensor's C linker. The issue occurred when compiling variants of the same graph with different orders of atomic inputs, which led to incorrect cache key generation and potential runtime errors.
Key changes:
- Modified the
in_sigfunction inCLinker.cmodule_key_to properly handle nominal variables that appear in the inputs list - Added comprehensive test cases covering both constant and nominal variable scenarios with different input orderings
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pytensor/link/c/basic.py | Fixed cache key generation logic to correctly identify and handle nominal/constant variables in input lists |
| tests/link/c/test_basic.py | Added regression tests for input order variations and C/CVM linker compatibility with nominal variables |
| # While the CVMLinker will call the CLinker on its one Op with all inputs (required and constants) | ||
| # This difference in input signature used to be ignored by the cache key, | ||
| # but the generated code cared about the number of explicit inputs. | ||
| # Changing the order of inputs is a smoke thest to make sure we pay attention to the input signature. |
There was a problem hiding this comment.
Corrected spelling of 'thest' to 'test'.
| # Changing the order of inputs is a smoke thest to make sure we pay attention to the input signature. | |
| # Changing the order of inputs is a smoke test to make sure we pay attention to the input signature. |
Copilot uses AI. Check for mistakes.
| ) | ||
| @pytest.mark.parametrize("atomic_type", ["constant", "nominal"]) | ||
| def test_clinker_atomic_inputs(linker, atomic_type): | ||
| """Test that compiling variants of the same graph with different order of atomic inputs |
There was a problem hiding this comment.
Sentence fragment. That that compiling variants does what?
2 participants
Closes #1670
📚 Documentation preview 📚: https://pytensor--1673.org.readthedocs.build/en/1673/