fix(core): validate Host(is_numa_current=...) as a bool - #2542
Open
LeSingh1 wants to merge 1 commit into
Open
Conversation
`Host.__new__` validates `numa_id` carefully -- including an explicit bool
rejection whose comment spells out why:
# bool is an int subclass; reject explicitly so Host(True) doesn't
# alias Host(1) (and vice versa) in the singleton cache.
`is_numa_current` gets no validation at all, even though it lands in the
same process-wide singleton cache key and is stored verbatim as the
`is_numa_current` property. The same aliasing hazard applies from the other
side:
>>> Host(is_numa_current=1) # int, not True
>>> h = Host.numa_current()
>>> h.is_numa_current
1
>>> h.is_numa_current is True
False
`(None, 1)` and `(None, True)` are the same dict key, so whichever call runs
first wins for the lifetime of the process -- and `test_host.py` already
asserts `Host.numa_current().is_numa_current is True`, which that first call
turns into an order-dependent failure.
Non-int truthy values are worse: they mint a second singleton instead of
poisoning the first.
>>> dup = Host(is_numa_current="yes")
>>> repr(dup)
'Host.numa_current()'
>>> dup is Host.numa_current(), dup == Host.numa_current()
(False, False)
Both coerce to the same `_LocSpec(kind="host_numa_current")`, so they name
one physical location while breaking the documented "constructor calls with
the same arguments return the same instance" contract and the identity-based
set arithmetic in `ManagedBuffer.accessed_by`.
`is_numa_current=0` and `=None` are equally unhandled: they are falsy, so
they seed the *generic* `Host()` singleton with a non-bool
`is_numa_current`. And an unhashable value reaches the cache lookup and
raises `TypeError: unhashable type` out of the constructor.
Require a real bool, mirroring the numa_id check.
Contributor
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
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.
Problem
Host.__new__validatesnuma_idcarefully, and the existing test explains exactly why:is_numa_currentgets no validation at all, even though it goes into the same process-wide singleton cache key and is stored verbatim as theis_numa_currentproperty:The same aliasing hazard applies from the other side.
1.
1poisons thenuma_currentsingleton, process-wide.(None, 1)and(None, True)are the same dict key, so whichever call runs first wins for the rest of the process:test_host.py::TestHost::test_numa_currentalready assertsh.is_numa_current is True, so that first call turns a shipped assertion into an order-dependent failure — the classic shape of a cross-test flake.2. Any other truthy value mints a second singleton instead of poisoning the first:
Both coerce to the same
_LocSpec(kind="host_numa_current")in_memory/_managed_location.py, so they name one physical location while breaking the class's documented contract ("constructor calls with the same arguments return the same instance") and the identity-based set arithmetic inManagedBuffer.accessed_by.3. Falsy non-bools hit the generic host.
is_numa_current=0/=Noneseed theHost()singleton with a non-boolis_numa_current, soHost().is_numa_current is Falsestops holding too.4. Unhashable values escape as
TypeError.Host(is_numa_current=[])raisesTypeError: unhashable type: 'list'from the cache lookup rather than aValueErrorfrom the constructor.Fix
Require a real
bool, mirroring thenuma_idcheck, with the sameValueErrorshape:Nothing else changes: the mutual-exclusion check still runs first (so
Host(numa_id=0, is_numa_current=True)keeps its "mutually exclusive" message), and no in-tree caller passes a non-bool.Tests
Added to
cuda_core/tests/test_host.py, directly alongside thenuma_idbool test that documents the same hazard:test_is_numa_current_rejects_non_bool, parametrized over1,0,"yes",None,[].test_numa_current_singleton_survives_a_rejected_construction, which pins the point of the fix: a rejected construction must not leave anything in the cache, soHost.numa_current().is_numa_current is Truestill holds afterwards.Verification I could and could not do
_host.pyimports onlythreadingandtyping, so I loaded theupstream/mainand fixed versions side by side and ran both the new assertions and a replica of every existingtest_host.pyassertion against each.main:1,0,"yes",Noneraise nothing (thepytest.raisesblocks fail) and[]raisesTypeError, whichpytest.raises(ValueError)does not catch — all five parametrizations fail.ValueError.numa_id/numa_currentconstruction, singleton identity,numa_idrejection messages, mutual exclusion,__eq__/__hash__,repr, copy/pickle round-trip — is unchanged in both.ruff check/ruff format --checkclean on both changed files;python -m py_compileclean.pytest cuda_core/tests/test_host.pyitself. I have no environment wherecuda.coreis importable (no CUDA driver, no built extension modules), so the module-levelfrom cuda.core import Hostcould not be exercised locally. Please treat CI as the first real run.Not in this PR
Host._instancesis a singleClassVardict keyed withoutcls, so a subclass shares the base class's cache and can be handed a base-class instance (type(Sub()) is Host).__hash__hardcodes the literalHostin its tuple, so the class looks intended to be final. Nothing in-tree subclassesHost, and fixing it is a separate decision (key oncls, per-subclass caches, or forbid subclassing) — happy to file it separately if you want it addressed.