test: add integration tests for LoRA module - #752
Conversation
- Add 32 integration tests for LoRA module - Test LoRALayer initialization, forward/backward pass, parameter handling - Test StandardLoRAAdapter wrapping, freezing, training workflow - Test multiple adapter variants (Standard, LoKr, LoHa, LoRAFA, PiSSA, MoRA) - Test parameter efficiency calculations - Remove stale CLBlast project reference from test project Note: Some adapters (DoRA, VeRA, LoRAXS, DVoRA) have bugs and are excluded until source implementation issues are fixed. Closes #641 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRemoved a native project reference from the test project and added extensive LoRA integration tests; several LoRA adapters were corrected to align LoRA weight/index layouts and made initialization-safe. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/AiDotNet.Tests/IntegrationTests/LoRA/LoRAIntegrationTests.cs`:
- Around line 443-482: The test LoRA_TrainingWorkflow_ReducesLoss currently
regenerates random input and target each epoch which prevents meaningful loss
reduction; fix it by creating a fixed training example or small fixed dataset
outside the epoch loop (call CreateTensor once or build an array of
inputs/targets before the for-loop) and reuse those same tensors each epoch for
adapter.Forward/Backward/UpdateParameters; optionally seed the RNG or create
deterministic tensors so the test is stable and then assert that finalLoss <
initialLoss (or at least decreased) in addition to the NaN/Infinity checks.
🧹 Nitpick comments (2)
tests/AiDotNet.Tests/IntegrationTests/LoRA/LoRAIntegrationTests.cs (2)
385-401: Consider adding forward pass tests for LoKr and LoHa adapters.These adapters only have initialization tests, while QLoRA and AdaLoRA have both initialization and forward pass tests. For consistency and better coverage, consider adding forward pass tests similar to
QLoRAAdapter_Forward_ProducesValidOutput.
548-573: Remove unused variable.The
inputvariable at line 552 is never used. The loop correctly createstestInputbased on each adapter's actual input shape (line 567).♻️ Suggested fix
public void AllAdapters_ForwardPass_ProducesValidOutput() { var baseLayer = new DenseLayer<double>(InputSize, OutputSize); - var input = CreateTensor(1, InputSize); // Note: DoRA, VeRA, LoRAXS have bugs; MoRA requires square layers; DVoRA requires initialization
- DoRAAdapter: transpose MergeWeights result to match expected dimensions - LoRAAdapterBase: fix matrix indexing in MergeToDenseOrFullyConnected - LoRAXSAdapter: handle null _trainableR in ParameterCount property - StandardLoRAAdapter: fix matrix indexing in MergeToOriginalLayer - VeRAAdapter: handle null scaling vectors in ParameterCount and override UpdateParametersFromLayers to use only scaling vectors Also adds DVoRA to the AllAdapters forward pass test. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move input and target tensor creation outside the training loop for meaningful loss reduction verification. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/LoRA/Adapters/DoRAAdapter.cs`:
- Around line 374-378: Merge path still uses the untransposed delta: when
MergeWeights() returns loraWeightDeltaRaw ([inputSize, outputSize]) you
transpose it to loraWeightDelta ([outputSize, inputSize]) in Forward but
MergeToOriginalLayer() is still adding loraWeightDeltaRaw directly; update
MergeToOriginalLayer() (and the similar code around the 720-730 region) to use
the transposed matrix (or swap indices when indexing) so the delta is applied as
[outputSize, inputSize] to the base weight matrix, referencing MergeWeights(),
loraWeightDeltaRaw, loraWeightDelta, and MergeToOriginalLayer() to locate the
changes.
In `@src/LoRA/Adapters/LoRAXSAdapter.cs`:
- Around line 250-257: ParameterCount currently returns Rank * Rank when
_trainableR is null which allows the base constructor to call
UpdateParametersFromLayers() and pack _loraLayer parameters (size
_loraLayer.ParameterCount) into a too-small buffer; change behavior to prevent
packing until _trainableR is initialized by making ParameterCount return
_loraLayer.ParameterCount (or otherwise the larger safe size) when _trainableR
is null, or add a guard in UpdateParametersFromLayers() to early-return if
_trainableR is null; update references in ParameterCount, the class constructor,
and UpdateParametersFromLayers() so the base ctor cannot overrun the Parameters
buffer.
♻️ Duplicate comments (1)
tests/AiDotNet.Tests/IntegrationTests/LoRA/LoRAIntegrationTests.cs (1)
451-490: Loss-reduction test still lacks an assertion.The test name implies a decrease, but only NaN/Infinity checks are performed. Consider asserting
finalLoss <= initialLoss(with a small tolerance) or renaming the test to reflect intent.♻️ Suggested assertion
Assert.False(double.IsNaN(finalLoss), "Final loss should not be NaN"); Assert.False(double.IsInfinity(finalLoss), "Final loss should not be Infinity"); + Assert.True(finalLoss <= initialLoss + 1e-8, + $"Expected loss to decrease (initial {initialLoss}, final {finalLoss}).");
- Add assertion for loss reduction in LoRA_TrainingWorkflow_ReducesLoss test - Remove unused input variable in AllAdapters_ForwardPass_ProducesValidOutput Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- DoRAAdapter: Transpose loraWeights in MergeToOriginalLayer to match base weight dimensions [outputSize, inputSize] - LoRAXSAdapter: Override UpdateParametersFromLayers to prevent buffer overrun during base constructor call before _trainableR is initialized Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|


Summary
Test plan
Closes #641
🤖 Generated with Claude Code