perf(autoformer): compute the correlation spectrum with one matmul; revive 20 dead GPU tests#1916
Merged
Merged
Conversation
…evive 20 dead GPU tests
PERF — Autoformer's auto-correlation spectrum.
R[lag] = mean over (t < corrLen-lag, dim) of q[t,:]·k[t+lag,:] is, by definition, the mean
of the lag-th diagonal of M = Q·Kᵀ. Summing a diagonal is LINEAR in M, so the whole
spectrum is one constant operator applied to vec(M): R = A @ vec(Q·Kᵀ).
The previous formulation built the spectrum with a per-lag loop issuing corrLen x ~5
dispatches — about 120 at the default lookback of 24 — at EACH of four call sites per
forward (encoder self-attention per layer, decoder self, decoder cross). It now issues ~5
total, independent of corrLen. The operator is cached per (lq, lk, d).
MEASURED, same test and machine (Autoformer_trains_without_crossing_its_ffn_gradient_shapes:
dims 8/64/512, double, CPU engine, n=160, 2 epochs):
before: 103s test-duration (120s wall)
after: 66s test-duration ( 79s wall) ~36% faster
Note the honest gap: a ~40x reduction in spectrum dispatches yields ~1.5x wall-clock, so the
spectrum was one significant cost among several rather than the whole story. The rest is the
per-sample forward (see OPEN below).
NOT bit-identical: matmul reduction associates the sums differently than the per-lag
ReduceSum. Mathematically identical, same gradients (A is a constant, so the tape
differentiates through vec(M) unchanged). Asserted at 1e-9 against an independently written
reference in AutoformerBatchedEquivalenceTests.
TESTS — 20 tests that had NEVER executed.
Every test in DirectGpuCorrectnessTests (5), DirectGpuTests (14) and SenseVoiceTrainStepProfile
(1) carried [Fact(Timeout = N)] on a SYNCHRONOUS method. xUnit rejects that outright with
"Tests marked with Timeout are only supported for async tests", so they failed in ~1 ms
regardless of hardware and their assertions never ran. Converted to async Task +
await Task.Yield(), KEEPING the Timeout so the intended hang protection is preserved. The
fixture doc comment that taught the broken pattern is corrected too.
Running them found a real GPU bug: Power(x, y) returned NaN for every negative x, so squaring
a negative produced NaN on GPU while CPU was correct. Root cause is --use_fast_math lowering
powf to exp2(y*log2(x)); fixed separately in AiDotNet.Tensors (PR #827). Verified against that
kernel packed locally: DirectGpuCorrectnessTests 5/5, DirectGpuTests 18 passed / 1 skipped.
Also fixed a genuine test bug: AssertAllClose used a purely ABSOLUTE tolerance, which cannot
span these ops' magnitudes — Exp10 reaches ~1e5 where one float ULP is already ~0.0078, so a
fixed 1e-2 window failed on a GPU/CPU difference of 0.023 that is only ~2e-7 relative. Now
mixed absolute+relative.
ALSO LANDED, unused by the shipped path: CorrelationSpectrumBatched and MovingAverageBatched
([B,S,D]), groundwork for the deferred batching follow-up, both tested at 1e-12 (identical ops
one rank wider, so exact equality is the contract).
OPEN — deliberately not fixed here:
1. Autoformer can still crash in TrainCore's gradient accumulation with
"Tensor shapes must match. Got [2048, 512] and [512, 2048]" — the two FFN weights
(_ff1Weight [ffDim, embDim], _ff2Weight [embDim, ffDim]) receiving each other's gradient
shape. NOT reproduced by any synthetic configuration. Eliminated: EmbeddingDim 8/64/512,
double AND float, CPU engine, GPU engine, 3-way concurrency, and concurrency+GPU+float
together — all pass, and AutoformerTrainShapeReproTests documents each. It requires the real
data that surfaced it; AutoCorrelationEngine's data-dependent top-k selection is the leading
candidate, since it makes graph shape value-dependent, which matches why only SOME cells
crashed while others merely ran slow.
2. Autoformer still trains with a PER-SAMPLE forward (ForwardCore inside for bi) while
InformerModel and TemporalFusionTransformer use ForwardBatch. Full batching is deferred: the
primitives above are in place, but it costs ~32x tape memory at the default BatchSize of 32,
which is precisely why TrainCore chose per-sample tapes, and it would restructure the forward
of a model whose gradient crash is still unexplained.
Builds clean on net10.0, net8.0 and net471.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThe change removes ComputationNode gradient-checkpointing APIs, updates diffusion memory estimation, adds batched Autoformer decomposition and correlation operations, introduces Autoformer regression coverage, and converts GPU-related tests and profiling entry points to asynchronous Task methods. ChangesCheckpointing integration removal
Autoformer batched operations
Test execution and regression coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
AiDotNet.Autodiff.GradientCheckpointing<T> could never save memory. It wrapped its
forward in NoGradScope, whose comment claims "ops within NoGradScope are not recorded
to the active tape" — but NoGradScope suppresses the AiDotNet.Tensors GradientTape,
and ComputationNode does not use that tape. ComputationNode carries its own
Parents/BackwardFunction graph and never consults it, so the full graph was built
regardless while the CheckpointContext holding the recompute Function was pushed and
immediately popped with nothing retaining it. Net effect: zero activation memory saved,
plus a Clone() of every input and the output into a discarded context — strictly worse
than not calling it, under a docstring promising 40-50% savings.
It was also unreachable. The legacy graph has no backward driver in production: nothing
outside src/Autodiff/Testing ever invokes BackwardFunction, and its TopologicalSort is
private and uncalled. Its only consumers, DiffusionMemoryManager.Checkpoint and
.CheckpointSequence, had zero callers themselves.
The name collision is how this stayed invisible: an unqualified GradientCheckpointing<T>
binds to whichever namespace the using list reaches first, so a reader could not tell
this apart from the working package primitive.
Removed:
- src/Autodiff/GradientCheckpointing.cs (class, CheckpointContext, CheckpointingExtensions)
- DiffusionMemoryManager.Checkpoint / .CheckpointSequence
Retained (both real, both still used):
- AiDotNet.Tensors.Engines.Autodiff.GradientCheckpointing<T>.Checkpoint(blockFns, input,
segmentSize) — tape-based, used by Transformer, NeuralNetworkBase, PipelineParallelModel
and NoisePredictorBase, covered by Diffusion/CheckpointGradientEquivalenceTests
- DiffusionMemoryManager.ForwardWithCheckpointing — the ILayer-based path that genuinely
saves checkpoints and recomputes
EstimateMemorySavings was the deleted file's only live use, so its arithmetic is inlined
into DiffusionMemoryManager.EstimateMemory: A*(s + ceil(n/s)), minimised at s = sqrt(n)
to 2*A*sqrt(n). That formula is sound and independent of the broken class.
BREAKING CHANGE: removes public API (AiDotNet.Autodiff.GradientCheckpointing<T>,
CheckpointingExtensions, DiffusionMemoryManager.Checkpoint/.CheckpointSequence). No caller
existed in this repo, but downstream consumers referencing them will need to move to the
AiDotNet.Tensors primitive.
Verified: src/AiDotNet.csproj builds clean on all three TFMs (net10.0, net8.0, net471),
0 errors.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/AiDotNet.Tests/DirectGpuTests.cs (1)
1-1: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winBLOCKING: Multiple tests always pass — no meaningful assertions across 8 GPU tests.
All 8 tests share one root cause: they execute GPU code but never assert outcomes, so they always pass regardless of correctness. Per test quality guidelines, tests with no meaningful assertions are blocking.
tests/AiDotNet.Tests/DirectGpuTests.cs#L618-621:DirectGpuEngine_NewKernels_Correctness— logs PASS/FAIL at lines 694–701 but never callsAssert.FailwhenerrorCount > 0. A kernel producing wrong results still passes. AddAssert.Fail($"{name}: {errorCount} elements exceeded tolerance")in the else branch.tests/AiDotNet.Tests/DirectGpuTests.cs#L110-113:DirectGpuEngine_MatMul_Benchmark— pure benchmark with zero assertions. Add at minimum a sanity assertion thatresultis non-null and has expected length after the final MatMul.tests/AiDotNet.Tests/DirectGpuTests.cs#L160-163:DirectGpuEngine_MatMul_Benchmark_KernelOnly— same pattern. AssertbufferCwas written to (e.g., download and check non-zero).tests/AiDotNet.Tests/DirectGpuTests.cs#L240-243:DirectGpuEngine_MatMul_Benchmark_KernelComparison— same pattern. Assert at least one kernel produced non-zero output.tests/AiDotNet.Tests/DirectGpuTests.cs#L329-332:DirectGpuEngine_MatMul_Benchmark_AllKernelVariations— same pattern. Assert the results dictionary has non-zero GFLOPS entries.tests/AiDotNet.Tests/DirectGpuTests.cs#L715-718:DirectGpuEngine_NewKernels_Benchmark— same pattern. Assert at least one kernel achieved non-zero GFLOPS.tests/AiDotNet.Tests/DirectGpuTests.cs#L951-954:DirectGpuEngine_StaticVsDynamic_DiagnosticBenchmark— same pattern. Assert profiling produced non-zero measurements.tests/AiDotNet.Tests/DirectGpuTests.cs#L1298-1301:DirectGpuEngine_ABTestKernelVariants_OptimizationComparison— same pattern. Assert the A/B test result string is non-empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AiDotNet.Tests/DirectGpuTests.cs` at line 1, Update all eight named GPU tests in DirectGpuTests to include meaningful assertions: make DirectGpuEngine_NewKernels_Correctness fail when errorCount is nonzero; validate final MatMul result existence and expected length; verify kernel output buffers or outputs are non-zero; require non-zero GFLOPS or profiling measurements in the benchmark tests; and require a non-empty A/B test result string. Preserve the existing benchmark and logging behavior while ensuring each test fails when its GPU operation produces no valid outcome.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Diffusion/Memory/DiffusionMemoryManager.cs`:
- Around line 285-291: Update the checkpointing calculation in EstimateMemory to
normalize Config.CheckpointEveryNLayers before using it: prevent zero or invalid
intervals from reaching the division, and clamp the effective segment size to
numLayers so it cannot exceed the actual layer count. Use the normalized value
for both segmentSize and numSegments while preserving the existing
WithCheckpointing calculation.
In `@src/TimeSeries/AutoformerModel.cs`:
- Around line 605-629: Make the _diagOperatorCache in DiagonalMeanOperator
thread-safe for concurrent reads and writes, using the project’s established
synchronization or concurrent-cache pattern while preserving cached reuse.
Expand the cache key to include corrLen, and use that complete key consistently
for lookup and insertion so independently supplied correlation lengths cannot
reuse an incompatible operator.
- Around line 592-607: Move the XML documentation block describing the batched
correlation spectrum from above the _diagOperatorCache field to directly above
the CorrelationSpectrumBatched method. Leave the cache field without that
method-specific documentation and preserve the existing summary and remarks
content.
In `@tests/AiDotNet.Tests/DirectGpuTests.cs`:
- Around line 618-621: Update DirectGpuEngine_NewKernels_Correctness to assert
that errorCount is zero after kernel validation, causing the test to fail when
any incorrect result is detected. Keep the existing PASS/FAIL logging, but
ensure the test outcome is determined by the assertion rather than logs alone.
In
`@tests/AiDotNet.Tests/IntegrationTests/TimeSeries/AutoformerTrainShapeReproTests.cs`:
- Around line 178-226: Add the existing shared serial test-collection attribute
to AutoformerTrainShapeReproTests, matching other tests that mutate
AiDotNetEngine.Current or Logger. Do not alter the concurrent test body; ensure
the class runs in the collection that prevents parallel execution with other
engine-swapping tests.
---
Outside diff comments:
In `@tests/AiDotNet.Tests/DirectGpuTests.cs`:
- Line 1: Update all eight named GPU tests in DirectGpuTests to include
meaningful assertions: make DirectGpuEngine_NewKernels_Correctness fail when
errorCount is nonzero; validate final MatMul result existence and expected
length; verify kernel output buffers or outputs are non-zero; require non-zero
GFLOPS or profiling measurements in the benchmark tests; and require a non-empty
A/B test result string. Preserve the existing benchmark and logging behavior
while ensuring each test fails when its GPU operation produces no valid outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f92e5a6-9af1-4b26-89fd-750e76e26c88
📒 Files selected for processing (9)
src/Autodiff/GradientCheckpointing.cssrc/Diffusion/Memory/DiffusionMemoryManager.cssrc/TimeSeries/AutoformerModel.cstests/AiDotNet.Tests/DirectGpuCorrectnessTests.cstests/AiDotNet.Tests/DirectGpuTests.cstests/AiDotNet.Tests/Fixtures/NetworkFixture.cstests/AiDotNet.Tests/IntegrationTests/TimeSeries/AutoformerBatchedEquivalenceTests.cstests/AiDotNet.Tests/IntegrationTests/TimeSeries/AutoformerTrainShapeReproTests.cstests/AiDotNet.Tests/Performance/SenseVoiceTrainStepProfile.cs
💤 Files with no reviewable changes (1)
- src/Autodiff/GradientCheckpointing.cs
Contributor
Commit messages auto-fixedOne or more commit messages did not follow Conventional Commits, so they were rewritten to comply (subject case, header length ≤ 100, valid type). Each commit and its diff were preserved — no squashing. The branch was force-pushed with the corrected messages. If you have local work on this branch, run |
ooples
force-pushed
the
perf/autoformer-matmul-spectrum
branch
from
July 20, 2026 22:59
c852808 to
f564537
Compare
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.
Perf — Autoformer's correlation spectrum
R[lag] = mean over (t < corrLen-lag, dim) of q[t,:]·k[t+lag,:]is, by definition, the mean of the lag-th diagonal ofM = Q·Kᵀ. Summing a diagonal is linear inM, so the whole spectrum is one constant operator applied tovec(M):The old formulation built it with a per-lag loop issuing
corrLen × ~5dispatches — ~120 at the default lookback of 24 — at each of four call sites per forward (encoder self-attention per layer, decoder self, decoder cross). It now issues ~5 total, independent ofcorrLen. The operator is cached per(lq, lk, d).Measured — same test, same machine
Autoformer_trains_without_crossing_its_ffn_gradient_shapes(dims 8/64/512, double, CPU engine, n=160, 2 epochs):~36% faster. Worth stating the gap honestly: a ~40× reduction in spectrum dispatches produced ~1.5× wall-clock, so the spectrum was one significant cost among several — not the whole story. The remainder is the per-sample forward (see Open below).
Not bit-identical. Matmul reduction associates the sums differently than the per-lag
ReduceSum. Mathematically identical, and gradients are unchanged (Ais a constant, so the tape differentiates throughvec(M)as before). Asserted at1e-9against an independently written reference.Tests — 20 that had never executed
Every test in
DirectGpuCorrectnessTests(5),DirectGpuTests(14) andSenseVoiceTrainStepProfile(1) carried[Fact(Timeout = N)]on a synchronous method. xUnit rejects that outright:They failed in ~1 ms regardless of hardware, so their assertions never ran. Converted to
async Task+await Task.Yield(), keeping theTimeoutso the intended hang protection survives. The fixture doc comment that taught the broken pattern is corrected too.Running them found a real GPU bug:
Power(x, y)returned NaN for every negativex— squaring a negative gave NaN on GPU while CPU was correct. Root cause is--use_fast_mathloweringpowftoexp2(y·log2(x)). Fixed in ooples/AiDotNet.Tensors#827. Verified against that kernel packed locally:DirectGpuCorrectnessTests5/5,DirectGpuTests18 passed / 1 skipped.Also fixed a genuine test bug:
AssertAllCloseused a purely absolute tolerance, which can't span these ops' magnitudes —Exp10reaches ~1e5 where one float ULP is already ~0.0078, so a fixed 1e-2 window failed on a difference of 0.023 that is only ~2e-7 relative. Now mixed absolute + relative.Also landed (unused by the shipped path)
CorrelationSpectrumBatchedandMovingAverageBatched([B,S,D]) — groundwork for the deferred batching follow-up. Both tested at1e-12, since they are the identical ops one rank wider and exact equality is the contract there.Open — deliberately not fixed here
1. Autoformer can still crash in
TrainCore's gradient accumulation withTensor shapes must match. Got [2048, 512] and [512, 2048]— the two FFN weights (_ff1Weight [ffDim, embDim],_ff2Weight [embDim, ffDim]) receiving each other's gradient shape.Not reproduced by any synthetic configuration. All of these pass, and
AutoformerTrainShapeReproTestsdocuments each:It requires the real data that surfaced it.
AutoCorrelationEngine's data-dependent top-k is the leading candidate: it makes graph shape value-dependent, which matches why only some cells crashed while others merely ran slow.2. Autoformer still trains per-sample (
ForwardCoreinsidefor bi) whileInformerModelandTemporalFusionTransformeruseForwardBatch. Full batching is deferred: the primitives are in place, but it costs ~32× tape memory at the defaultBatchSizeof 32 — precisely whyTrainCorechose per-sample tapes — and it would restructure the forward of a model whose gradient crash is still unexplained.Build
Clean on net10.0, net8.0 and net471.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests