Load a method whose constant name collides with another method's - #21706
Closed
shoumikhin wants to merge 1 commit into
Closed
Load a method whose constant name collides with another method's#21706shoumikhin wants to merge 1 commit into
shoumikhin wants to merge 1 commit into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21706
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
shoumikhin
force-pushed
the
cuda-constant-fqn-collision
branch
from
August 20, 2026 00:10
f321153 to
a9a7930
Compare
The mismatch check only ran when at least one name was a cache miss, because it sat inside the `if (!uncached_fqns.empty())` branch. A method whose constant names all coincide with an earlier method's has no misses, so it took the other branch and shared every constant on the strength of the names alone. That is the total-collision case this check exists for. AOTInductor names a lifted constant by position within one compiled module, so two methods can each own a `_tensor_constant2` holding unrelated data. Sharing that pair aliases mismatched storage. Load the blob and extract in that branch too, then run the same comparison. When the blob or the extraction entry points are unavailable there is no second opinion to be had, so keep the previous behaviour and log that sharing rests on names only, rather than failing a load that used to work.
shoumikhin
force-pushed
the
cuda-constant-fqn-collision
branch
from
August 20, 2026 00:21
a9a7930 to
144ce1b
Compare
shoumikhin
requested review from
JacobSzwejbka,
abhinaykukkadapu,
digantdesai,
kirklandsign,
larryliu0820,
psiddh,
rascani and
robert-kalmar
as code owners
August 20, 2026 00:21
Contributor
Author
|
Superseded by #21959. This PR's branch history was rewritten by accident: a commit was amended in a shallow #21959 carries the same change on the same branch, rebuilt on top of No review comments here were addressed by that rebuild; the code is the same as this PR |
shoumikhin
added a commit
that referenced
this pull request
Aug 20, 2026
) Replaces #21706, whose branch history was rewritten by accident and which GitHub then closed. Same change, rebuilt on `main`. ## The problem A program can hold several methods, for example one that fills a cache from a prompt and one that runs a single step. The CUDA delegate can share a constant between them so one copy of a weight serves every method that uses it. Sharing is keyed on the constant's name. That key is not reliable on its own. AOTInductor names a lifted constant by its position within one compiled module, so two methods can each own a `_tensor_constant2` holding completely unrelated data. Sharing that pair points both methods at the same storage, and one of them then reads the wrong weights. Until now the first such collision failed the whole load: ``` Constant '_tensor_constant2' in method 'decode' is incompatible with the cached version from a previous method. Refusing to share. ``` So a valid program, two methods that happen to have same-named constants, could not be loaded at all. ## The change Compare the cached tensor against the one the current method expects. When they differ, leave this method's own constant in place rather than failing the load, and keep the cached copy for whichever method it does match. ## What is covered, and what is not The comparison happens where the weights blob is loaded, so it covers any method that has at least one constant the cache has not seen yet. That is the common case. **A method whose constant names are all already cached is not covered.** Checking it would mean re-uploading the weights blob to compare against, and that has to happen for every such method, which raises peak device memory enough to fail a large model. An earlier version of this change did exactly that and three of the four `muse-glimmer` end to end jobs died with `CUDA error: out of memory`. So that case still shares on the name alone, as it did before, and the comment at the cache lookup says so. Fixing it properly needs a way to read the cached constants' metadata without a blob round trip. The comparison is also on tensor metadata only: dtype, rank, sizes, strides, device type and device index. It does not compare the data, and it cannot, because both handles point at device memory. Two constants that agree on all of that while holding different values would still be shared. This narrows the problem rather than eliminating it, and it turns a hard failure into a working load for the case it does cover. ## Test plan - Built the delegate and loaded a two-method program whose constant names collide where one method has a constant the cache has not seen. Both methods load, and the log names the constant that was not shared. - Loaded a two-method program with genuinely shared constants and confirmed sharing still happens, so the change does not disable the optimisation it guards. - Ran the existing CUDA runtime unit tests. There is no automated regression test for the collision case. Building one needs a two-method program with colliding constant names and a GPU to run it on, which the unit test targets in this directory do not have. Reverting this change would not fail any test in the repository today.
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.
The problem
A program can hold several methods, for example one that fills a cache from a prompt and one
that runs a single step. The CUDA delegate can share a constant between them so one copy of a
weight serves every method that uses it. Sharing is keyed on the constant's name.
That key is not unique. A constant lifted out of a graph is named by its position within one
compiled module, so the first method's third constant and the second method's third constant are
both called
_tensor_constant2, and they usually hold different data. Loading such a programfails:
The program is fine. Each method carries its own copy of the constant and can run with it. Only
the attempt to treat two unrelated tensors as one fails.
The change
Refusing to alias mismatched storage is correct, so that check stays. What changes is the
response: decline to share that one constant and let the method use its own copy, rather than
failing the load for the whole program. Constants that do match go on being shared exactly as
before, so a program with no collisions behaves identically.
One detail matters for correctness. The cache lookup runs before the tensors can be compared, so
a name match is only provisional at that point. A constant rejected after extraction has to be
left out of the user-managed pairs too, otherwise the method is pointed at a tensor it did not
ask for.
Scope
This fixes the load failure and nothing else. On the program used to test it, the run is not yet
deterministic across repeats, and that is also true before this change when sharing is turned
off, so it is a separate pre-existing issue rather than something this introduces or resolves.
Testing
Built the CUDA delegate from source on Linux x86_64 and ran a three-method program whose methods
have colliding constant names, comparing the same program and runner against an unmodified
delegate:
The first row is the failure above. The second shows the program itself is loadable, which is
what makes failing the load the wrong response.
cc @digantdesai @freddan80 @per @zingo @oscarandersson8218 @mansnils @Sebastian-Larsson @robell @rascani