Let a serialized fake-tensor state_dict be read back - #22111
Let a serialized fake-tensor state_dict be read back#22111ThomasJannaud wants to merge 1 commit into
Conversation
Summary: `serialize_torch_artifact` already supports fake tensors: it installs `_reduce_fake_tensor`, which writes a FakeTensor as its `TensorMeta` -- shape, dtype and stride, no storage. Two gaps stopped the matching read path from working, so an artifact whose `state_dict` holds fake tensors could be written and then not read back. Both are latent and independent of any caller; they are split out of the Helios stack that found them so they can be reviewed on their own. **1. `weights_only=True` refused ExecuTorch's own reconstructor.** `deserialize_torch_artifact` loads with `weights_only=True`, which rejects any global that is not allowlisted: WeightsUnpickler error: Unsupported global: GLOBAL executorch.exir.serde.export_serialize._reconstruct_fake_tensor was not an allowed global by default Fixed by allowlisting the reconstructor for the load with `torch.serialization.safe_globals`. That is narrower than `add_safe_globals`, which registers process-globally and permanently -- but narrower in *time* only, not per-call: the allowlist is a single process-global set, so a concurrent `torch.load(weights_only=True)` on another thread during the window accepts this global too. **Entering it is conditional, and that is not defensive coding.** `safe_globals` unions that set on entry and *subtracts* on exit, with no refcount. Entering it unconditionally therefore revokes the registration of any caller who has already added this global with `add_safe_globals` -- precisely the workaround people applied before this function handled it. Measured rather than argued: with the unconditional form, a probe reading the allowlist around one load reported `before=True after=False`. **2. Rebuilding a parameter raised on any integer dtype.** `_reconstruct_fake_tensor` restored `nn.Parameter` entries as `torch.nn.Parameter(fake_tensor)`, and that default of `requires_grad=True` *raises*: RuntimeError: only Tensors of floating point dtype can require gradients `nn.Parameter` accepts an integer tensor as long as `requires_grad` is False, and quantized weights -- uint8 codes and their zero points -- are exactly that. So this hit the whole class of quantized models, not an edge case. `requires_grad` is derived from the dtype. Recording the flag in the reducer instead was written first, then backed out: the reducer's return value is the on-disk format, and two other copies of this module in fbsource (`xplat/spacecraft/sample_apps`, `third-party/mediatek/release-0.7`) still have the two-argument `_reconstruct_fake_tensor`, so widening the payload would produce records they cannot read. The cost is fidelity, not correctness: a float parameter that had `requires_grad=False` comes back True. That was already the behaviour before integer parameters worked at all, and no reader of a serialized fake `state_dict` consumes the flag. `test_requires_grad_is_derived_from_the_dtype` pins it, so it stays a known trade-off rather than a surprise. The reasoning behind both choices is left as inline comments on this diff rather than in the source, which keeps only the two constraints a future reader needs in order not to undo them. Scope, so this is not over-read: `torch._export.serde.serialize` carries the same `requires_grad` defect and is untouched here. It is a different module path, so it can never be resolved for a record written by this one -- the same bug in another codebase, not a compatibility hazard for this one. Differential Revision: D115918112
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22111
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 1 Unrelated FailureAs of commit 1aaaded with merge base 4d3b46d ( NEW FAILURES - The following jobs have failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
@ThomasJannaud has exported this pull request. If you are a Meta employee, you can view the originating Diff in D115918112. |
This PR needs a
|
|
@claude review this code |
|
Claude finished @metascroy's task in 1m 14s —— View job Code Review
This is a careful, well-scoped fix. Both gaps are real: A few small notes, none blocking: 1. from contextlib import contextmanager, nullcontextWorth running 2. Thread-safety of the conditional guard is broader than the exit race ( 3. Correctness / tests
Non-code: the EasyCLA check is currently failing (commit not covered by a signed CLA) — that'll need resolving before merge, independent of the code. Overall LGTM — just the import-ordering nit is worth addressing before landing. |
Summary:
serialize_torch_artifactalready supports fake tensors: it installs_reduce_fake_tensor, which writes a FakeTensor as itsTensorMeta-- shape,dtype and stride, no storage. Two gaps stopped the matching read path from
working, so an artifact whose
state_dictholds fake tensors could be writtenand then not read back. Both are latent and independent of any caller; they are
split out of the Helios stack that found them so they can be reviewed on their
own.
1.
weights_only=Truerefused ExecuTorch's own reconstructor.deserialize_torch_artifactloads withweights_only=True, which rejects anyglobal that is not allowlisted:
WeightsUnpickler error: Unsupported global: GLOBAL
executorch.exir.serde.export_serialize._reconstruct_fake_tensor
was not an allowed global by default
Fixed by allowlisting the reconstructor for the load with
torch.serialization.safe_globals. That is narrower thanadd_safe_globals,which registers process-globally and permanently -- but narrower in time only,
not per-call: the allowlist is a single process-global set, so a concurrent
torch.load(weights_only=True)on another thread during the window accepts thisglobal too.
Entering it is conditional, and that is not defensive coding.
safe_globalsunions that set on entry and subtracts on exit, with no refcount. Entering it
unconditionally therefore revokes the registration of any caller who has already
added this global with
add_safe_globals-- precisely the workaround peopleapplied before this function handled it. Measured rather than argued: with the
unconditional form, a probe reading the allowlist around one load reported
before=True after=False.2. Rebuilding a parameter raised on any integer dtype.
_reconstruct_fake_tensorrestorednn.Parameterentries astorch.nn.Parameter(fake_tensor), and that default ofrequires_grad=Trueraises:
RuntimeError: only Tensors of floating point dtype can require gradients
nn.Parameteraccepts an integer tensor as long asrequires_gradis False,and quantized weights -- uint8 codes and their zero points -- are exactly that.
So this hit the whole class of quantized models, not an edge case.
requires_gradis derived from the dtype. Recording the flag in the reducerinstead was written first, then backed out: the reducer's return value is the
on-disk format, and two other copies of this module in fbsource
(
xplat/spacecraft/sample_apps,third-party/mediatek/release-0.7) still havethe two-argument
_reconstruct_fake_tensor, so widening the payload wouldproduce records they cannot read. The cost is fidelity, not correctness: a float
parameter that had
requires_grad=Falsecomes back True. That was already thebehaviour before integer parameters worked at all, and no reader of a serialized
fake
state_dictconsumes the flag.test_requires_grad_is_derived_from_the_dtypepins it, so it stays a knowntrade-off rather than a surprise.
The reasoning behind both choices is left as inline comments on this diff rather
than in the source, which keeps only the two constraints a future reader needs
in order not to undo them.
Scope, so this is not over-read:
torch._export.serde.serializecarries thesame
requires_graddefect and is untouched here. It is a different modulepath, so it can never be resolved for a record written by this one -- the same
bug in another codebase, not a compatibility hazard for this one.
Differential Revision: D115918112