test(lora): align LoRAValidationTests with the resolved-shape adapter contract - #1506
Conversation
… contract Nine LoRAValidationTests were red on master because they fed adapters a LAZY DenseLayer(outputSize) — every test even declares `int inputSize` and then never uses it, a leftover from before DenseLayer's lazy-init refactor. An unresolved base has no weight matrix, so: - the adapters fell back to the NON-authoritative outSize*2 input-dim heuristic (ParameterCount-readiness only, per review #1368 — never meant to size real adapters), making DoRA's Forward reject the [batch, inputSize] test input and VeRA reject its shared matrices ("required 20x4" vs the 5x4 the test initialized); - MergeToOriginalLayer indexed past the unmaterialized base layer's empty parameter vector (ArgumentOutOfRangeException); - DefaultLoRAConfiguration.ApplyLoRA correctly skipped the lazy layer (the #1345 guard), so the Creates/Throws tests saw an unchanged DenseLayer instead of the expected adapter type. Design decision, now pinned by a new test (DefaultLoRAConfiguration_LazyUnresolvedLayer_ReturnsUnchanged): shape-UNRESOLVED layers pass through ApplyLoRA untouched — wrapping from fabricated dims was explicitly rejected in review #1368, and the production pipeline (AiModelBuilder) resolves shapes via the warmup forward / TryDeclareShape oracle (#1370) before the LoRA pass. Fix: materialize each test's base layer to its declared inputSize via the public ResolveFromShape API (exactly what the production pipeline's warmup achieves), restoring the tests' original intent — the outputSize > inputSize merge/forward indexing coverage now actually exercises real weight matrices. All 23 LoRAValidationTests pass; no production code changes needed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 38 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This branch was successfully deployed
2 participants
Summary
Closes the 9 master-red
LoRAValidationTestsfailures by settling the lazy-shape design question and aligning the tests with the resolved-shape adapter contract. Test-only change — no production code is touched.The design question
Should
ApplyLoRA(and direct adapter construction) wrap a layer whose input shape is still unresolved?Decision: no — and that was already decided, in three pieces:
ApplyLoRAreturn lazy layers unchanged (wrapping at zero/unknown shape crashes adapter construction).outSize*2heuristic as "silent fabrication" — it survives only forParameterCount-readiness on unresolved bases, never as real adapter dims.AiModelBuilder) resolves shapes before the LoRA pass (warmup forward +TryDeclareShapeoracle, Shape Oracle: layer-side TryDeclareShape() to eliminate LoRA warmup forward (review #1368) #1370).The 9 failing tests predate the lazy-
DenseLayerrefactor: each declaresint inputSize = 5;and then never uses it, constructingnew DenseLayer<double>(outputSize)(lazy). The unresolved base made:outSize*2heuristic → DoRAForwardrejected the[batch, 5]test input ("Flat index is out of range"), VeRA rejected its shared matrices ("required 20×4" vs the 5×4 the test initialized);MergeToOriginalLayerindex past the unmaterialized base's empty parameter vector (ArgumentOutOfRangeException);DefaultLoRAConfigurationCreate/Throw tests see the guard's pass-through instead of the expected adapter type.Changes
inputSizevia the publicResolveFromShapeAPI (what the production warmup achieves) — restoring the tests' original intent. TheoutputSize > inputSizemerge/forward indexing coverage now genuinely exercises real weight matrices.DefaultLoRAConfiguration_LazyUnresolvedLayer_ReturnsUnchangedpins the design decision: a shape-unresolved layer passes throughApplyLoRAuntouched.Verification
LoRAValidationTests23/23 (was 14/23 on master).LoRAValidationTests+Bucket10_LoRA+ConvBatchNormFoldTestsrun: 28/28.Related: PR #1493 independently widens the
ApplyLoRAguard to theTryDeclareShapeoracle so ctor-determined layers (MultiHeadAttention) are admitted; that change and this one are compatible — lazyDenseLayeris skipped under both.🤖 Generated with Claude Code