fix(#1405): moe default optimizer overshoots — use amsgrad adam(1e-4) - #1409
Conversation
…v deserialize fallback PR #1290 CI Cluster 1: 25 of 25 DCGANTests failing post-master with one of two errors: 1. Most (23 tests): "Invalid layer configuration: The last layer's output shape [3, -1, -1] must match the architecture output size (12288)." 2. Clone tests (2): "Input spatial dims after padding (1+2*1, 1+2*1) must be >= kernelSize (4)" raised inside DeserializationHelper's pre-resolve of the discriminator's first conv layer. Plus 1 SparseNN test (intermittent mode-collapse) that re-runs pass without code change — flaky, not a regression target. ## Root causes (1) NeuralNetworkBase.IsLastLayerShapeCompatible: PR #1329 (commit 969977d) added a `outputShape.Any(d => d < 0)` early-return so the validator defers the flat-OutputSize check when any output-shape dim is deferred — DCGAN's last transposed-conv emits [3, -1, -1] until its first Forward resolves H/W. That guard was inadvertently deleted by the grafprint PR (c8cac23, May 16) one day later. Restoring it unblocks all 23 validator-rejection cases at once. (2) DeserializationHelper conv path: when the saved layer record's inputShape carries -1 sentinels (a lazy conv layer serialized before its first Forward — DCGAN's discriminator on a Predict-only probe sees only the generator), the pre-existing code coerced all -1 dims to 1 and called conv.ResolveShapesOnly(...). For DCGAN's first conv (kernel=4, padding=1) this fails OnFirstForward's kernel-size check (1 + 2 < 4). Coercing to Math.Max(1, KernelSize) fixes that specific check, but locks InputDepth at 1 — then the real Forward with the [3, 64, 64] RGB image throws "Expected input depth 1, but got 3". The correct fix is to skip pre-resolve entirely when InputDepth is deferred — ConvolutionalLayer.SetParameters has its own auto-resolve fallback at line ~1598 that derives InputDepth from the saved parameter vector's length, and uses KernelSize as the spatial placeholder. Pre-resolve still runs (and uses Math.Max(1, KernelSize) for any deferred spatial dim) when InputDepth is concrete — that's the original PR #1329 contract for the auto-resolve-disambiguation case. ## Verification $ dotnet test --filter "FullyQualifiedName~DCGANTests|FullyQualifiedName~SparseNeuralNetworkTests" --framework net10.0 Failed! - Failed: 2, Passed: 44, Skipped: 0, Total: 46 26 → 2 failures. The remaining two are NOT cluster-1 shape-contract issues: - DCGANTests.MoreData_ShouldNotDegrade — `Test execution timed out after 120000 milliseconds`. Pre-existing GAN training-path perf gap; the deep deconv+conv chain in tape mode is ~5-10× slower than PyTorch CPU baseline. Substep profile (Release): Generator.Predict 19 ms, Discriminator.Train 187 ms, Generator adversarial 313 ms — 519 ms/step × 250 iters = 130 s vs 120 s timeout. Filed separately so this PR ships the actual cluster-1 root causes (validator + conv-deserialize) without bundling a multi-week perf project. - SparseNeuralNetworkTests.DifferentInputs_AfterTraining_ShouldProduceDifferentOutputs — intermittent mode-collapse, passes on re-runs. Separate flaky-test issue, not a shape-contract bug. Closes #1309 partially (cluster-1 shape-contract root causes). The MoreData_ShouldNotDegrade timeout + SparseNN mode-collapse flakiness are tracked separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…alformed Conv inputShape rank
…ses DCGAN MoreData timeout
Previously GenerativeAdversarialNetwork.Train ran the generator forward TWICE
per training step:
1. Generator.Predict(input) (eval mode, NoGradScope) → detached fake images
for the combined real+fake discriminator step.
2. ForwardForTraining(input) (train mode, on tape) inside
TrainWithCustomLoss — duplicate of the same forward, just for the
gen-adversarial backward.
On the DCGAN MoreData fixture (250 iters, double-precision, batch=2, 64×64
RGB) this duplicate forward contributed ~19 ms of the 519 ms / step
profiled in #1390 — pushing the test 10 s over its 120 s budget.
Refactor:
- Open a single GradientTape at the start of the step.
- Run ForwardForTraining(input) ONCE on that tape → fakeTapeTracked.
- Take a value-copy detached snapshot (fakeImages) for the disc step;
fresh Tensor<T> with no GradNode chain so disc.Train (which opens
its own nested tape) can not leak gradients back into the generator.
- Walk the discriminator layer-by-layer on the existing gen tape for
the adversarial loss (unchanged from the prior closure semantics).
- Drive the gen optimizer step via the new
NeuralNetworkBase.BackwardAndStepOnPrecomputedLoss helper, which
reuses the open tape instead of TrainWithCustomLoss opening a fresh
one + re-running ForwardForTraining.
Behavior note: the disc step now sees train-mode generator output
(batch BN stats) instead of eval-mode (running BN stats). This matches
PyTorch's standard DCGAN training pattern (fake = G(z); fake_detached =
fake.detach()) and the existing gen step's own train-mode forward.
DCGAN has no Dropout, so the only distribution shift is BN stats, which
is the conventional adversarial behavior.
Verified locally with the canonical Tensors 0.81.3 dependency:
- DCGANTests.MoreData_ShouldNotDegrade: 1 m 47 s (was timing out at
> 120 s) — closes the test's perf gap.
- Full DCGANTests class: 25 / 25 passing.
- ConditionalGANTests + InfoGANTests (other GAN.Train consumers):
50 / 50 passing.
- Full SparseNeuralNetworkTests: 21 / 21 passing (previously
"intermittent mode-collapse" in PR #1389 description — appears
stable now, may have been transient).
Closes #1390.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
addresses three coderabbit comments on backwardandsteponprecomputed loss in pr #1389: 1. visibility narrowed public -> internal. the codebase contract is "users should only interact with aimodelbuilder / aimodelresult" and this helper is training plumbing for in-assembly callers (currently generativeadversarialnetwork.train); no reason for it to live on the public surface. only caller is in same assembly. 2. added using var __reentrancyguard = acquiretrainsentinel() at the top, mirroring trainwithtape's sentinel discipline. without it, concurrent callers on the same model race on lastloss + optimizer internal state. 3. trainableparams now concats getextratrainabletensors() with the layer params, matching trainwithtape's parameter set. without this models that expose raw tensors via getextratrainabletensors (rather than layer-resident params) silently skipped updates on the precomputed-loss path -- divergent semantics between the two training entry points. build passes.
vanilla adam default at the recommended learning rate diverges on the moredata long-training regression for mixtureofexpertsneuralnetwork (short=ok, long=overshoot) — same signature as the densenet fix in #1393. two root causes: 1. lr too aggressive for the moe gating + per-expert summed-gradient path, where multiple experts touch each parameter per step. 2. vanilla adam's v denominator drifts late in training, so step size grows even after loss has bottomed out. switching the built-in default to amsgrad-mode adam at 1e-4 keeps the running max of v (reddi et al. 2018), which removes the late-training denominator decay and matches the recipe already proven on densenet. users passing an explicit optimizer are unaffected. Closes #1405
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesDefault AMSGrad optimizer configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary
MixtureOfExpertsNeuralNetworkTests.MoreData_ShouldNotDegradewas failing because the network's built-in default optimizer (vanillaAdamOptimizer<T,Tensor,Tensor>(this)) diverges in the long-training regime: short training fits, but loss climbs again as training proceeds.This is the same signature as the DenseNet divergence fixed in #1393, and the same recipe applies.
Root cause
Two reinforcing problems with the previous default:
v̂denominator drifts late in training (Reddi et al. 2018, On the Convergence of Adam and Beyond). AMSGrad fixes this by keeping the running max ofvinstead of the EMA, which prevents the denominator from shrinking and the step from blowing up.Fix
Switch the built-in default in
MixtureOfExpertsNeuralNetworkto AMSGrad-mode Adam atlr = 1e-4:Users who pass an explicit optimizer are unaffected.
Test plan
MixtureOfExpertsNeuralNetworkTests.MoreData_ShouldNotDegradepasses on net10.0Closes #1405
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes