Repository navigation
Graph capture fix - #17
Merged
Merged
Conversation
…memory. Graph capture on ConvPCNetwork/SimpleConvPCNetwork/GaussSeidelPCNetwork/ DiscriminativePCNetwork surfaced two pre-existing CUDA backend bugs that had never been reachable before (nothing used capture on this path): 1. ComputePrecisionWeightedErrorAndEnergy/ComputeErrorAndEnergy both do a blocking cudaStreamSynchronize() to read energy back to a host float, illegal while a stream is mid-capture. FullPCLayer/SimplePCLayer already had a needEnergy bool to skip this; ConvPCLayer/ SimpleConvPCLayer/GaussSeidelPCLayer/DiscriminativePCLayer never did. 2. ConvPCLayer/DiscriminativePCLayer's input-layer branch read log_p[i] directly from host code -- illegal once log_p is a GPU device pointer (DeviceMemoryArena), independent of capture entirely. Added the missing ComputePrecisionWeightedError (no-energy, no-sync) and Sum (host-safe reduction) backend primitives to CPUBackend/CUDABackend, threaded needEnergy through all 4 affected layers/networks mirroring SimplePCLayer's existing pattern, and fixed two nanobind bindings that became ambiguous once CalculateState had two overloads. Verified: full CPU rebuild clean, ctest passing, and all 4 TrainStep/ TrainStepWithProjection sanity tests produce bit-for-bit identical energy trajectories to before this fix -- zero CPU behavioral change, GPU-path correctness only. The new CUDA kernels themselves are unverified by a compiler (no GPU on this machine); they closely mirror the existing, working kernels in the same file.
…e fix). Reported symptom: on real GPU hardware, the first train_step() call after switching inference_steps (triggering a graph re-capture) returned the EXACT same energy as the previous call, as if nothing executed; the next call then showed roughly double the expected delta, as if the "missing" update got applied together with the following one. No CUDA error was printed anywhere in the capture/instantiate/launch sequence. cudaGraphLaunch() only enqueues the replay -- it returns once the launch is submitted, not once it completes. Same-stream ordering should already guarantee the subsequent energy readout waits for it, but added an explicit IComputeBackend::Synchronize() (CPUBackend: no-op; CUDABackend: cudaStreamSynchronize()) right after every ReplayGraph() call across all 12 call sites (7 networks) to close any doubt, since this costs nothing extra on the path that was already about to do its own blocking sync for the energy value anyway. Verified: CPU rebuild clean, ctest passing, zero behavioral change on CPU (Synchronize() is a no-op there). GPU-side effect unverified -- no GPU on this machine -- this is a hypothesis-driven fix pending retest.
GPU branch, to directly observe graphCaptured/capturedInferenceSteps state transitions -- diagnosing a reproducible bug where the first train_step() call after switching inference_steps (triggering recapture) produces an unchanged energy value, and the NEXT call's delta is consistently ~2x a full old-step-count delta, not the new one (observed across two independent runs with different random weights). Sanitizer (synccheck) reports 0 errors, so this isn't a CUDA API misuse bug -- the recapture logic itself needs to be observed directly. Temporary, to be removed once the root cause is found.
…) call Distinguishes two remaining hypotheses for the GraphCaptureFix bug: whether the first replay after a fresh cudaGraphInstantiate() genuinely performs no GPU work, or whether the GPU work happens but the energy readout reported by the existing [DIAG] lines is stale/host-cached. Reads layers.front()'s first weight directly via CopyToHost, bypassing CalculateState() entirely, before and after the recapture branch's ReplayGraph()+Synchronize(). If W[0] changes across that call, the GPU executed real work and the bug is in how energy is read out; if it doesn't, the first launch truly is a no-op and the bug is in graph execution itself. Temporary, to be removed once the root cause is found.
…resh energy read The W[0] diagnostic from the last run proved the GPU is NOT idle on the first replay after a fresh cudaGraphInstantiate(): weights move by a normal, in-trend amount on every single call, capture or not. So the bug isn't graph execution -- it's specifically that the energy value returned by TrainStep() is stale (bit-identical to the previous call's) on the one call that just did a (re)capture. Root cause: ConvPCLayer::ComputeMuOnly() caches mu for clamped layers (isClamped && muCacheValid) to avoid recomputing an input that hasn't changed. ClampState() normally resets muCacheValid=false at the top of every TrainStep() call, which is what forces the final, uncaptured CalculateState() read (the one that produces the returned/printed energy) to recompute mu from this call's POST-weight-update state. But a (re)capture runs the whole settling loop as real C++ while *recording* the graph -- that loop's last ComputeMuOnly() call leaves muCacheValid=true as a side effect, and nothing clears it again before ReplayGraph() runs and the fresh energy read happens. That read then wrongly takes the cached branch, returning mu as it was captured (from this call's PRE-update weights) instead of the fresh value ReplayGraph() just produced. Fix: explicitly invalidate every layer's mu-cache right after a successful EndGraphCapture(), mirroring what a fresh ClampState() call would have done. Diagnostic instrumentation (std::cerr) left in place for this round of testing; to be removed once confirmed.
… is fixed and confirmed temp2.py's recapture test now reports RESULT: OK on the GPU node. The InvalidateMuCache() fix from the previous commit stands on its own.
…capturing network ConvPCNetwork's confirmed fix (commit 663262a) generalizes: every network that pairs CUDA graph capture with a clamped-layer mu-cache (isClamped && muCacheValid, used to skip recomputing an input that hasn't changed) has the identical latent bug. A (re)capture runs its settling loop as real C++ while recording the graph, which leaves muCacheValid=true behind on whichever clamped layer it last touched; nothing clears that before ReplayGraph() runs and the following fresh, uncaptured energy read happens, so that read wrongly reuses mu from before this call's own weight update instead of the value the replay just produced. Covered now, all capture sites (including TrainStepWithProjection() variants where present): - ConvPCNetwork (TrainStepWithProjection(), missed in the previous commit) - SimplePCNetwork - SimpleConvPCNetwork (both TrainStep() and TrainStepWithProjection()) - DiscriminativePCNetwork (both TrainStep() and TrainStepWithProjection()) - DirectKPPCNetwork - FullPCNetwork, FullConvPCNetwork: InvalidateMuCache() already existed here (added earlier for an unrelated reason -- iPC's weights changing mid-settle), but was only called when useIPC is on; added the same unconditional call made everywhere else so the non-iPC path is covered too. GaussSeidelPCNetwork is NOT affected: GaussSeidelPCLayer has no mu-cache at all, so there's nothing for a recapture to leave stale. Added InvalidateMuCache() to SimplePCLayer, SimpleConvPCLayer, DiscriminativePCLayer, and DirectKPPCLayer (ConvPCLayer already has it from the previous commit; FullPCLayer/FullConvPCLayer already had it for the iPC reason above). Builds clean and passes ctest locally (CPU-only, no GPU on this machine); the GPU-side fix still needs verifying on the cluster the same way ConvPCNetwork's was, since none of this has been exercised against real graph capture yet.
2 tasks done
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
1 participant
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.
Description
Closes CUTLASS integration.
Type of change
Testing
python build.py Releasebuilds andDeepityTestspassespython build.py Debugbuilds andDeepityTestspasses (required for anything correctness-sensitive)pyrightpasses (if you touchedbindings/,pydeepity/,examples/, orexperiments/)Platform(s) and compiler(s) tested:
Performance impact
Appears to yield <1s speedup. Testing not done, but allocation is safe.
Checklist
[Unreleased](for any user-facing change)..clang-formatstyle.Notes for reviewers
This is the test run result: