Load a method whose constant name collides with another method's - #21959
Open
shoumikhin wants to merge 1 commit into
Open
Load a method whose constant name collides with another method's#21959shoumikhin 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/21959
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 6da3cd5 with merge base 3caaaca ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
shoumikhin
force-pushed
the
cuda-constant-fqn-collision
branch
from
August 20, 2026 03:40
591542c to
8251a71
Compare
shoumikhin
force-pushed
the
cuda-constant-fqn-collision
branch
from
August 20, 2026 03:43
8251a71 to
8b20c37
Compare
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 unrelated data. Sharing that pair points both methods at the same storage and one of them reads the wrong weights. Until now the first such collision failed the whole load, so a valid program with two same-named constants could not be loaded at all. Compare the cached tensor against the one this method expects, and when they differ leave this method's own constant in place rather than failing. The cached copy stays for whichever method it does match. The comparison happens where the weights blob is loaded, so it covers methods that have at least one constant the cache has not seen. A method whose names are all already cached has nothing to compare against without re-uploading the blob, which costs too much device memory to do on every method, so that case still shares on the name alone. The comment at the cache lookup says so. The comparison is on tensor metadata: dtype, rank, sizes, strides, device type and device index. It does not compare the data and cannot, because both handles point at device memory. Two constants agreeing on all of that while holding different values would still be shared.
shoumikhin
force-pushed
the
cuda-constant-fqn-collision
branch
from
August 20, 2026 03:56
8b20c37 to
6da3cd5
Compare
shoumikhin
added
the
release notes: runtime
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.
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_constant2holdingcompletely 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:
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-glimmerend to endjobs 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
one method has a constant the cache has not seen. Both methods load, and the log names
the constant that was not shared.
happens, so the change does not disable the optimisation it guards.
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.