Fix PyTorch autograd hook dropped after no-grad call (include requires_grad in call-data cache signature) - #1054
Fix PyTorch autograd hook dropped after no-grad call (include requires_grad in call-data cache signature)#1054nv-slang-bot[bot] wants to merge 7 commits into
Conversation
|
Thanks for the review, @ccummingsNV — appreciated. Status: approved on the current HEAD ( This PR is intentionally held as a draft under our automated-contributor policy — the bot does not self-promote to ready-for-review or self-merge. Whenever you're happy for it to land, a maintainer can mark it ready and squash-merge (it One note: 🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify. |
af81600 to
d2896f7
Compare
|
Reworked and re-pushed. The earlier approval no longer covers this code, so this needs a fresh look rather than a re-confirmation. Why it changed. #1082 ("Torch tensors supply vector dims") landed on What to look at:
Verified on an L40S: 206 passed / 0 skipped, the regression green on Staying in draft — promotion is a maintainer call. 🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify. |
| *p++ = shape_compatibility_char(extent); | ||
| *p++ = ','; | ||
| *p++ = 'G'; | ||
| p = fast_itoa(p, requires_grad); |
There was a problem hiding this comment.
couldn't this just be *p++ = tensor.requires_grad ? '1' : '0'
There was a problem hiding this comment.
Yes — done. fast_itoa was pointless for a value that is always one character; it is now *p++ = tensor.requires_grad() ? '1' : '0';, and the requires_grad local is gone since the ternary reads the tensor directly.
🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.
| @@ -142,6 +147,9 @@ extern "C" int tensor_bridge_get_signature(void* py_obj, char* buffer, size_t bu | |||
| *p++ = 'V'; | |||
| for (int64_t extent : sizes) | |||
There was a problem hiding this comment.
I'd prefer for the none-variable-length signature parts (inc this new one) to be before the variable length part
There was a problem hiding this comment.
Done — format is now [Dn,Sm,Gk,V...], so every fixed-width field precedes the variable-length run.
Worth noting your reasoning is better than what I had. I originally proposed exactly this ordering, then talked myself out of it on the grounds that nothing parses the signature (it is an opaque cache key), so I put G last to keep main's [Dn,Sm,V...] prefix byte-identical and minimise the diff. That optimised for the wrong thing: "all fixed-width parts before the variable-length part" is a property worth holding regardless of whether a parser exists today, because it is what keeps the format cheap to extend.
Applied in the native emitter, the Python fallback (bridge_fallback.py), and all 13 signature expectations across three test files.
🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.
| // TENSOR_BRIDGE_SIGNATURE_BASE_SIZE + tensor rank. | ||
| // Returns: TENSOR_BRIDGE_SUCCESS (0) on success, or a negative TensorBridgeResult on error | ||
| // Format: "[Dn,Sm,V...]" where n=ndim, m=scalar_type, and V contains one | ||
| // Format: "[Dn,Sm,V...,Gk]" where n=ndim, m=scalar_type, V contains one |
There was a problem hiding this comment.
see below - let's make constant length parts before the variable length part of the sig
There was a problem hiding this comment.
Updated to "[Dn,Sm,Gk,V...]" with the invariant stated explicitly — every fixed-width field precedes the variable-length V run, which is always last. The fallback docstring in bridge_fallback.py carries the same wording so the two cannot drift.
🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.
| # Restore original state | ||
| slangpy.set_torch_bridge_python_fallback(original) | ||
|
|
||
| def test_native_bridge_version_matches(self): |
There was a problem hiding this comment.
I don't think we need more of these bridge tests - they're testing the same stuff we already test for in different ways
There was a problem hiding this comment.
Removed test_native_bridge_version_matches — you are right that it largely duplicates test_is_torch_bridge_available.
One assertion in it was not covered elsewhere, so I folded that single line into the existing test rather than keep a whole new one: is_torch_bridge_using_fallback() is False when slangpy_torch is importable. Without it a version-incompatible bridge falls back silently and the suite stays green with the native path dead — which is the failure mode that made the 8→9 bump necessary in the first place. Three lines inside a test that already exists, instead of a separate one.
test_stale_bridge_version_is_rejected I have kept: it is the only test that exercises the incompatible path deliberately, by shimming a bridge with a stale api_version and the correct info_struct_size so only the version differs. Happy to drop it too if you would rather, but it is the guard against a future format change forgetting the version bump.
🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.
The torch call-data cache signature keys on rank, dtype and per-dimension shape compatibility but not on requires_grad. Because the autograd hook is gated on a flag frozen into the CallData when it is built, a no-grad call followed by a grad call of the same rank/dtype/shape reuses the non-autograd entry and silently drops the hook: grad_fn is None and backward() never reaches the input. Adds the ordering regression in both native and Python-fallback bridge modes, plus signature-level coverage asserting the grad bit coexists with the shape-compatibility classifiers from #1082 rather than displacing them. Expectations are updated at all four signature assertion sites, and test_nn_parameter_signature now compares nn.Parameter against a tensor with matching grad-ness so it keeps asserting type-handling parity.
The autograd hook is decided once, when a CallData is built, and cached. The torch tensor signature keyed rank, dtype and per-dimension shape compatibility, so a no-grad call and a later grad call of the same rank/dtype/shape collapsed onto one cache entry. Whichever call arrived first fixed the autograd decision for both, and a no-grad-first ordering left the grad call with grad_fn=None. Appends a grad bit after the shape-compatibility section, giving '[Dn,Sm,V...,Gk]'. It is appended rather than substituted so the vector-dims keying from #1082 keeps its meaning; both are part of the routing identity. The emitter and the Python fallback are updated together, since divergence between them makes the two bridge modes key the cache differently. TENSOR_BRIDGE_API_VERSION 8->9: the compat gate can only observe api_version and info_struct_size, so an output-format change is invisible to it unless the version moves. The existing BASE_SIZE + rank buffer contract is unchanged -- the added ',Gk' fits the fixed allowance, which retains 33 bytes of headroom in the worst case. Also refreshes the signature format documented in the torch integration instructions, which still described the pre-#1082 '[Dn,Sm]' form.
The compat gate discriminates only on api_version + info_struct_size, and TensorBridgeInfo's layout is unchanged, so api_version is the sole signal that can distinguish two wire formats. Without this test a forgotten version bump lets a stale bridge pass the gate and silently emit the old format. Restored verbatim from af81600; it reads API_VERSION dynamically (real - 1), so it fails closed against 9 with no adaptation.
) The positive counterpart to test_stale_bridge_version_is_rejected: it asserts a matched native/ext pair is NOT reported incompatible and that the native path is the active one. Without it a bump that breaks the matched pair would only surface as a silent fallback to the Python bridge. Restored verbatim from af81600.
The compat-gate rationale documents a live invariant and stays; the 6->7/7->8/8->9 list is changelog narration that belongs in the PR body.
…gth run Signature format is now [Dn,Sm,Gk,V...] so every fixed-width field comes before the per-dimension shape run, keeping the format cheap to extend. The grad bit is written directly rather than through fast_itoa, which was pointless for a single character. Drops test_native_bridge_version_matches as redundant with test_is_torch_bridge_available, keeping only its unique assertion — that an importable native bridge is the active path, since an incompatible one falls back silently.
9fd422c to
a9dca29
Compare
Motivation
The torch-tensor call-data cache signature did not include grad-ness.
requires_gradwas read into the tensor-info struct but never written to the cache key. The autograd hook is gated at dispatch on thetorch_autogradflag frozen into the cachedCallDataat build time (slangpyfunction.cpp→is_torch_autograd()), whichdetect_torch_tensorssets once, on the first (cache-miss) build.So a no-grad-first build cached
torch_autograd=False; a later same-rank/dtype/shape grad call hit that entry, theTorchAutoGradHookwas bypassed, and the output came back withgrad_fn=None. That exactly explains the reporter's asymmetry — no-grad→grad broken, grad→grad fine, grad→no-grad fine.Fixes #1052
Proposed solution
Append a per-tensor grad bit to the signature, composing it onto the existing shape-compatibility run:
This is a composition, not a replacement. #1082 (
d0ee6b1, "Torch tensors supply vector dims") landed theVrun onmainafter this branch was cut, and it rewrote the same emitter. Taking either side wholesale would regress the other: keeping onlyGdrops #1082's vector-dims keying, keeping onlyVdrops the #1052 grad fix. Both are cache-collision bugs, just on different axes — so the format carries both fields.The bit is per-tensor rather than folded across arguments: a single call-level flag would collide grad
(True, False)with(False, True), which is the same class of collision being fixed.Implemented in both bridges — native
tensor_bridge_get_signature(src/slangpy_torch/torch_bridge_impl.cpp) and the Pythonget_signaturefallback (slangpy/torchintegration/bridge_fallback.py) — so the two paths cannot key the cache differently.Change summary
9 files changed, +197 / −28. Most of that is test updates: the format change invalidates every hardcoded signature expectation, and there are nineteen of them across three files.
src/slangpy_torch/torch_bridge_impl.cpp,G<bit>before the shape-compat runsrc/slangpy_torch/tensor_bridge_api.hTENSOR_BRIDGE_API_VERSION8 → 9; format contract doc updatedslangpy/torchintegration/bridge_fallback.pysrc/slangpy_ext/utils/torch_bridge.cppslangpy/tests/slangpy_tests/test_pytorch.pyslangpy/tests/utils/test_torch_bridge.pyslangpy/tests/slangpy_tests/test_torchintegration.pyslangpy/tests/slangpy_tests/test_bridge_fallback_gaps.pyConcepts and vocabulary
slangpy.cppappends it verbatim to aSignatureBuffer. Field order is therefore not a parsing constraint. Per review, every fixed-width field precedes the variable-lengthVrun, which is always last — a property worth holding regardless of whether a parser exists today, because it keeps the format cheap to extend.src/slangpy_ext/utils/torch_bridge.h) — decides whether the native bridge is used or the Python fallback. It discriminates onapi_versionandinfo_struct_sizeonly.Why
API_VERSIONhad to move 8 → 9mainand this branch both defined version8, for different wire formats:main's8is[Dn,Sm,V...](#1082), this branch's was[Dn,Sm,Gk]. BecauseTensorBridgeInfo's layout is unchanged,info_struct_sizecannot discriminate them — soapi_versionis the sole signal. Leaving both at8would let a stale bridge pass the gate and silently emit the wrong format: no error, just a mis-keyed cache. The unlanded side (this one) renumbers to9.test_stale_bridge_version_is_rejectedpins this: it shims a bridge withapi_version = real - 1and the correctinfo_struct_size, so only the version differs, and asserts the gate reportsincompatible. That is the guard against a future format change forgetting the bump.The positive direction is covered inside
test_is_torch_bridge_available, which asserts that whenslangpy_torchis importable it is also the active path — an incompatible bridge falls back silently, so without that assertion the suite would stay green with the native path dead.Buffer sizing
The native guard is unchanged:
required_size = TENSOR_BRIDGE_SIGNATURE_BASE_SIZE + ndim. It is not padded for the extra,G<bit>, becauseBASE_SIZE(64) is a deliberate fixed over-allowance and the composed format still fits inside it:Stated assumption:
fast_itoatakes a plainintwith no clamp or range check, so budgeting 10 decimal digits each forndimandscalar_typeis a true bound over representable inputs rather than an assumption about typical values. (Bound derivation credit: reviewer analysis on the tracking thread.)Padding the guard would also have broken
test_native_signature_buffer_size_contract, which pins the bound exactly —required_size - 1must fail andrequired_sizemust succeed.Bounds behaviour differs between the two paths (pre-existing, tracked separately)
Format is in lockstep; bounds are not. The native path enforces a contractual bound (
BASE_SIZE + ndim) while the fallback enforces an actual-length bound (sig.size() + 1). This PR changes neither —src/slangpy_ext/utils/torch_bridge.his not in its diff and the native guard is untouched.Tracked as #1091 (P2), out of scope here. Documented in this PR only so the asymmetry is not mistaken for something this change introduced — not as a claim that it is harmless.
Verification
Built and run on an NVIDIA L40S (torch 2.13.0+cu126,
slangpy_torch.API_VERSION = 9).test_torch_bridge.py,test_bridge_fallback_gaps.py,test_torchintegration.py, and the regression (86 pre-existing skips for unavailable device types).[native-vulkan],[native-cuda],[fallback-vulkan],[fallback-cuda].api_version 8with the correctinfo_struct_size 72against compiled9yieldsfallback_reason='incompatible',using_fallback=True.test_signature_distinguishes_requires_grad[fallback]fail with[D2,S6,V44]against the expected[D2,S6,G0,V44]; restoring it passes. The test discriminates rather than merely passing.pre-commit run --all-filesclean.Not verified locally:
ninjaexits non-zero on 89.pyistub-generation commands — a pre-existing environment issue (the stub step invokes/usr/bin/python3, which lacks numpy, instead of the venv). Zero C++ compile or link errors; both extensions import. Left untouched as unrelated to this change.Notes for reviewers
test_bridge_fallback_gaps.pynever conflicted because this branch had not touched it, so git did not flag it — but it hardcodes signature literals and would have failed post-merge. Five more expectations are@pytest.mark.parametrizerows asserted ~15 lines from the table.torch.empty(())has an emptyVrun, so the signature is[D0,S6,G0,V].,Gis written unconditionally before the extent loop, so an empty run needs no special case.requires_gradbitfield inTensorBridgeInfopredates this work (50c4656, PyTorch optimizations #759) and is unrelated to the cache key — its presence onmainis not evidence of a grad fix.NativeTorchTensorDiffPair::read_signatureis deliberately untouched: that path is unreachable pre-existing due to a Eliminate heap allocations from cached dispatch hot path #872 regression (documented attest_torchintegration.py:1286-1291, which explicitly does not cover it), so there is no second live signature path to keep in lockstep. The asymmetry is intentional, not an oversight.🤖 Generated by an automated SlangPy coworker — may be inaccurate. A human maintainer should verify.