feat(#1211): ONNX symbolic-axis end-to-end with ONNX Runtime - #1269
Merged
Merged
Conversation
Closes the remaining #1211 spec item: prove the symbolic-axis wire format actually binds dynamically at inference time, not just at the protobuf-encoding layer. Test: - New `OnnxSymbolicAxisRuntimeTests.Graph_WithSymbolicAxes_RunsAtMultipleSpatialSizes_WithoutReExport` builds a minimal ONNX graph (input → ReLU → output) with batch / H / W marked symbolic, then runs it through `Microsoft.ML.OnnxRuntime` at THREE different shapes through the SAME `InferenceSession`: 1. 1×3×224×224 (paper-canonical ImageNet size) 2. 1×3×320×320 (different H/W; the #1211 contract) 3. 2×3×256×192 (different batch AND non-square) - Per-shape assertion validates the ReLU op actually ran (negative inputs → 0, non-negative → identity), so a graph that's binding the wrong op or returning input as-is is caught. - Note in the file's XML doc explains why we use ReLU (a supported ExportLayer op) instead of Conv: the exporter doesn't yet have Conv/Pool ONNX ops — that's separate scope. The symbolic-axis contract being validated is identical regardless of body op. Bug fix: - `OnnxExporter.HasDynamicSpatialAxes` was reading the model's `Architecture` via `GetType().GetProperty("Architecture")`, but `NeuralNetworkBase.Architecture` is declared as a public FIELD (not a property). Reflection returned null for every NN model, so the symbolic-axis emission silently no-op'd whenever a real model was exported. Probe `GetField` first, then fall back to `GetProperty`. Verification: - `dotnet build AiDotNet.sln -c Release --no-incremental`: 0 errors on net10 + net471. - Test passes — ORT binds the symbolic axes dynamically across all 3 different shapes through a single exported graph. Closes #1211. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/concepts/projects/project-configuration |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReflection-backed ONNX exporter now probes architectures via field-or-property and converts reflected tensors (rank‑1/2) to floats; dense export looks up GetWeights/GetBiases fallbacks. Tests validate symbolic ONNX dim_param binding and runtime shape-preservation. Transformer defaults changed to Vaswani Adam + Noam schedule; deterministic per-layer seeding (optional) and per-batch optimizer OnBatchEnd wiring added. ChangesONNX symbolic-axis export & runtime validation
Transformer training defaults, deterministic init, and wiring
Sequence Diagram(s)sequenceDiagram
participant Trainer as Trainer/TrainWithTape
participant Model as Transformer (NeuralNetworkBase)
participant Optimizer as AdamOptimizer
participant Scheduler as NoamSchedule
Trainer->>Model: Forward pass -> compute loss
Trainer->>Model: Backward pass -> compute gradients
Trainer->>Optimizer: Apply gradients (Update parameters)
Trainer->>Model: After updates -> call batch boundary hook
Model->>Optimizer: OnBatchEnd()
Optimizer->>Scheduler: Scheduler.Step() (SchedulerStepMode=StepPerBatch)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes issue #1211’s end-to-end validation by proving that ONNX graphs emitted with symbolic axes (dim_param) actually bind dynamically at inference time in ONNX Runtime, and fixes a reflection bug that previously prevented dynamic-axis emission for real neural-network models.
Changes:
- Add a new ONNX Runtime integration test that runs a single exported model at multiple
(batch, H, W)shapes without re-exporting. - Fix
OnnxExporter.HasDynamicSpatialAxesto correctly readNeuralNetworkBase.Architecturewhen it is a public field (not a property), restoring dynamic H/W axis emission.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs | New end-to-end ONNX Runtime test validating symbolic batch/H/W axes work across multiple shapes using a single session. |
| src/Onnx/OnnxExporter.cs | Bug fix: reflectively retrieves Architecture via GetField (with GetProperty fallback) so HasDynamicSpatialDims is detected correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 1
🤖 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 `@tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs`:
- Around line 45-57: Add a new runtime test that exercises the exporter path
instead of constructing the graph directly: create a small model class with an
Architecture field, build the model using existing builder logic, call
OnnxExporter.ExportToBytes(...) to produce the ONNX bytes (referencing
OnnxExporter.ExportToBytes), then create a single InferenceSession and reuse it
to run inference with multiple input shapes to verify symbolic axes are
preserved; ensure the test uses the same input axis specs as the original
(OnnxAxisSpec.Symbolic("batch"), OnnxAxisSpec.Fixed(3),
OnnxAxisSpec.Symbolic("H"), OnnxAxisSpec.Symbolic("W")) and asserts outputs for
at least two different H/W shapes to catch regressions in symbolic axis metadata
emission.
🪄 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: 3ea7ae11-a2fe-40e0-b8ce-c4bcf275a1c8
📒 Files selected for processing (2)
src/Onnx/OnnxExporter.cstests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs
…nit — closes 3 bugs Three coupled bugs surfaced during V=256 batched flake investigation on PR #1265 follow-up. All three are required to make Transformer training deterministic AND paper-faithful AND robust on small-budget tasks. BUG 1: Vaswani β₂=0.98 / ε=1e-9 require a paired warmup+inverse-sqrt LR schedule (Vaswani 2017 §5.3). PR #1265 added the β₂/ε then had to revert them because the framework didn't apply the schedule. This commit ships the schedule alongside the hyperparameters so they travel together as the "Vaswani recipe": - New `NoamSchedule` class implementing lr(t) = factor · d_model^(-0.5) · min(t^(-0.5), t · warmup^(-1.5)) - Transformer<T> default optimizer now constructs Adam with β₂=0.98, ε=1e-9, and a NoamSchedule(d_model, warmupSteps) attached. - `TransformerArchitecture.WarmupSteps` (default 4000, paper-faithful) exposed as a constructor parameter so consumers with shorter training budgets can pass a smaller value. BUG 2: `OnBatchEnd` (the public entry point that advances the optimizer's LR scheduler) was never called from any training path. `grep -rn "OnBatchEnd\b" src/` returned only the definition, no callers. Schedulers attached to optimizers via either the new default-recipe path OR a user-supplied `LearningRateScheduler =` config were silently inert: the LR stayed pinned at its initial value forever and `StepScheduler()` was never invoked. Fix: NeuralNetworkBase.TrainWithTape now calls GradientBasedOptimizerBase.OnBatchEnd() at the end of each training step, immediately after `opt.Step(context)` and the network-level extras update. This is the canonical per-batch boundary that the OnBatchEnd contract documents ("called at the end of each training batch"). Optimizers that don't derive from GradientBasedOptimizerBase fall through unchanged. BUG 3: V=256 batched 100-step memorization test showed 12-28% top-1 accuracy spread across runs (init-seed flake). Root cause: `InitializationStrategyBase` sources Xavier draws from `RandomHelper.ThreadSafeRandom` which is non-deterministic across process instances; every `new Transformer(arch, ...)` got different initial weights and small-budget convergence varied accordingly. Fix: `TransformerArchitecture.RandomSeed` (default null, preserves pre-fix behaviour) lets consumers request reproducible weight init. When set, `LayerHelper.CreateDefaultTransformerLayers` wires every weight-bearing layer's `InitializationStrategy` to a shared `EagerInitializationStrategy(new Random(seed))`. Same seed -> same weights every run, so unit tests, multi-seed experiment harnesses, and CI all see deterministic accuracy. `EagerInitializationStrategy` and `InitializationStrategyBase` gained a `Random?` constructor parameter to thread the seeded RNG through (default behaviour unchanged). Test results — all 8 transformer integration tests pass deterministically across 5 consecutive runs: Constructor_DefaultOptimizer_IsAdamNotGradientDescent ✓ Train_SingleSample_V4_MemorisesAfter5000Steps ✓ Train_SingleSample_V16_MemorisesAfter1000Steps ✓ TrainBatched_V256_LearnsBatchAfter100Steps ✓ (was flaky 12-28%) Train_LossDecreasesByAtLeastHalfOnMemorizationTask ✓ ExplicitAdamMatchesDefaultBehavior ✓ SetBaseTrainOptimizer_OverridesCtorDefault_OnTrainCall ✓ Facade_Predict_MatchesDirectModelPredict_AfterBuildAsync ✓ The SetBaseTrainOptimizer test was updated to use explicit non-default optimizers on both sides — without that change the Vaswani+Noam default would converge in 50 steps just like the "high-LR" override and the test couldn't differentiate them. The test's ACTUAL semantic (verify SetBaseTrainOptimizer takes effect) is now isolated from the default-optimizer recipe. The integration-test helper `MakeArch` opts in to deterministic init (`randomSeed: 42`) and a small warmup (`warmupSteps: 10`) because tests run on tiny budgets where the 4000-step paper warmup would never exit; production users still get the paper-faithful 4000 default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The original runtime test built the onnx graph manually via onnxmodelbuilder, which doesn't go through onnxexporter — so the hasdynamicspatialaxes reflection probe (architecture is a public field on neuralnetworkbase, not a property) wasn't actually verified at runtime even though pr #1259 fixed the silent no-op there. This commit: - adds tinyvisionnet test stub (lazy conv + maxpool, dynamic-spatial architecture) that runs through onnxexporter.exporttobytes and asserts symbolic axis names appear in the produced bytes — proving the field-aware reflection probe fired - guards the existing inferencesession-using test with #if !net471 since the ort native binding crashes the test host with av on full framework (test passes on net10.0 where ort is stable) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/NeuralNetworks/TransformerArchitecture.cs (1)
393-414:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking: validate
warmupStepsinput before storing it.Line 413 accepts any value, including
0/negative, which can break default scheduler math downstream. Add a guard in the constructor.💡 Suggested fix
public TransformerArchitecture( InputType inputType, NeuralNetworkTaskType taskType, int numEncoderLayers, int numDecoderLayers, int numHeads, int modelDimension, int feedForwardDimension, NetworkComplexity complexity = NetworkComplexity.Medium, int inputSize = 0, int outputSize = 0, double dropoutRate = 0.1, int maxSequenceLength = 512, int vocabularySize = 0, bool usePositionalEncoding = true, double temperature = 1.0, SequencePoolingMode? sequencePooling = null, List<ILayer<T>>? layers = null, int warmupSteps = 4000, int? randomSeed = null) : base( inputType: inputType, taskType: taskType, complexity: complexity, inputSize: inputSize, outputSize: outputSize, layers: layers) { + if (warmupSteps <= 0) + throw new ArgumentOutOfRangeException(nameof(warmupSteps), "Warmup steps must be positive."); + NumEncoderLayers = numEncoderLayers; NumDecoderLayers = numDecoderLayers; NumHeads = numHeads; ModelDimension = modelDimension; FeedForwardDimension = feedForwardDimension; DropoutRate = dropoutRate; MaxSequenceLength = maxSequenceLength; VocabularySize = vocabularySize; UsePositionalEncoding = usePositionalEncoding; Temperature = temperature; WarmupSteps = warmupSteps; RandomSeed = randomSeed;As per coding guidelines: "Production Readiness (CRITICAL - Flag as BLOCKING): ... missing validation of external inputs".
🤖 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 `@src/NeuralNetworks/TransformerArchitecture.cs` around lines 393 - 414, The constructor for TransformerArchitecture is currently assigning the warmupSteps parameter directly to WarmupSteps; add input validation in the constructor (e.g., in the parameter-handling block where WarmupSteps is set) to ensure warmupSteps is a positive non-zero integer—if invalid either throw an ArgumentOutOfRangeException with a clear message or fallback to the default (4000) before assigning to the WarmupSteps property; update any unit tests or callers as needed to expect the exception or new behavior.src/NeuralNetworks/Transformer.cs (1)
649-660:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the active optimizer in one place.
Train()now resolves through the base optimizer slot, butGetModelMetadata(),SerializeNetworkSpecificData(), andCreateNewInstance()still read_optimizer. AfterAiModelBuilder.ConfigureOptimizerorSetBaseTrainOptimizer(...), the model can train with one optimizer and then report/serialize/clone a different one. Please either source those paths fromGetOrCreateBaseOptimizer()or keep_optimizersynchronized whenever the base optimizer changes.🤖 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 `@src/NeuralNetworks/Transformer.cs` around lines 649 - 660, Train() now uses GetOrCreateBaseOptimizer() but GetModelMetadata(), SerializeNetworkSpecificData(), and CreateNewInstance() still read the private field _optimizer, causing inconsistent reporting/serialization/cloning after SetBaseTrainOptimizer/AiModelBuilder.ConfigureOptimizer changes; fix by either (A) have those methods obtain the optimizer via GetOrCreateBaseOptimizer() instead of reading _optimizer, or (B) ensure any change through SetBaseTrainOptimizer/GetOrCreateBaseOptimizer synchronizes the backing field _optimizer (update its value whenever the base optimizer is set) so all code paths see the same optimizer instance (reference methods: Train, GetModelMetadata, SerializeNetworkSpecificData, CreateNewInstance, GetOrCreateBaseOptimizer, SetBaseTrainOptimizer, and field _optimizer).tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs (1)
413-436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t hardcode warmup in the shared test helper.
MakeArch()is also used byExplicitAdamMatchesDefaultBehavior(). WithwarmupSteps: 10, that test’s default-construction path now includesNoamSchedule, while its “explicit Adam” path is still flat Adam. The test no longer isolates constructor branching; it compares two different optimizers. Make warmup configurable per caller, or update that test to attach the same scheduler explicitly.As per coding guidelines, tests must be production-quality and verify actual behavior.
🤖 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/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs` around lines 413 - 436, MakeArch currently hardcodes warmupSteps:10 which changes optimizer scheduling for callers like ExplicitAdamMatchesDefaultBehavior; change MakeArch signature to accept an int warmupSteps (with default 10) and pass that value into the TransformerArchitecture constructor (warmupSteps: warmupSteps), then update the caller ExplicitAdamMatchesDefaultBehavior to call MakeArch with the warmup value that keeps both paths equivalent (e.g., warmupSteps:0 or the same schedule used in the explicit-Adam path) so the test compares identical optimizer schedules.
♻️ Duplicate comments (1)
tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs (1)
55-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe exporter fix still isn’t exercised end-to-end through ONNX Runtime.
Line 56 validates runtime behavior from a graph built directly with
OnnxModelBuilder, while Line 156 only byte-scans the result ofOnnxExporter.ExportToBytes. That split still leaves the repaired reflection path unproven at runtime: the exporter could emit bytes containing the symbolic names yet still fail when oneInferenceSessionis reused across multiple shapes. Please add one exporter-based runtime test that exports a real model throughOnnxExporter, then runs the same session at multiple spatial sizes.Also applies to: 155-185
🤖 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/Onnx/OnnxSymbolicAxisRuntimeTests.cs` around lines 55 - 99, Add a new test that exercises the exporter end-to-end: build the same symbolic-axes relu model but obtain bytes via OnnxExporter.ExportToBytes (call ExportToBytes on the model/export path used elsewhere), create an InferenceSession from those exported bytes, and then invoke AssertReluRoundtrip on that single session at the three spatial shapes (1×3×224×224, 1×3×320×320, 2×3×256×192) to ensure reuse across shapes; reference the existing test Graph_WithSymbolicAxes_RunsAtMultipleSpatialSizes_WithoutReExport for shape choices and use the same AssertReluRoundtrip helper and the InferenceSession constructor to validate runtime behavior.
🤖 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/Helpers/LayerHelper.cs`:
- Around line 1704-1716: The MultiHeadAttentionLayer currently ignores the
seeded strategy because InitializeParameters() creates a new SimdRandom instead
of using the layer's InitializationStrategy; update
MultiHeadAttentionLayer.InitializeParameters() to call
InitializationStrategy?.InitializeWeights(...) (or the equivalent Initialize
method used by DenseLayer) for _queryWeights, _keyWeights, _valueWeights and
_outputWeights, preserving the same shapes and scale logic, and only fall back
to creating a SimdRandom when InitializationStrategy is null to keep
backward-compatible behavior; this will allow the Wire(ILayer<T> layer) logic
that assigns sharedInit to LayerBase<T>.InitializationStrategy to actually
produce reproducible parameters.
In `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 392-394: Update the XML documentation for the
TransformerArchitecture constructor to match the current signature: add <param>
entries for sequencePooling, warmupSteps, and randomSeed and update or remove
any stale params (e.g., old pooling or warmup names) so the <param> list exactly
mirrors the constructor parameters (sequencePooling, layers, warmupSteps,
randomSeed, etc.) in the TransformerArchitecture class/constructor declaration.
In `@tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs`:
- Around line 77-81: The test currently converts onnxBytes to a UTF-8 string and
searches for substrings ("batch", "H", "W"), which is unsafe; instead parse
onnxBytes into an ONNX ModelProto (using Google.Protobuf/Onnx parser) and assert
the model graph's input/output tensor shapes by checking each
TensorShapeProto.Dim[].DimParam equals the expected symbolic names ("batch",
"H", "W") for the relevant inputs/outputs (replace the
Encoding.UTF8.GetString/Assert.Contains checks that reference onnxBytes); apply
the same replacement for the other occurrence that also scans the raw bytes.
---
Outside diff comments:
In `@src/NeuralNetworks/Transformer.cs`:
- Around line 649-660: Train() now uses GetOrCreateBaseOptimizer() but
GetModelMetadata(), SerializeNetworkSpecificData(), and CreateNewInstance()
still read the private field _optimizer, causing inconsistent
reporting/serialization/cloning after
SetBaseTrainOptimizer/AiModelBuilder.ConfigureOptimizer changes; fix by either
(A) have those methods obtain the optimizer via GetOrCreateBaseOptimizer()
instead of reading _optimizer, or (B) ensure any change through
SetBaseTrainOptimizer/GetOrCreateBaseOptimizer synchronizes the backing field
_optimizer (update its value whenever the base optimizer is set) so all code
paths see the same optimizer instance (reference methods: Train,
GetModelMetadata, SerializeNetworkSpecificData, CreateNewInstance,
GetOrCreateBaseOptimizer, SetBaseTrainOptimizer, and field _optimizer).
In `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 393-414: The constructor for TransformerArchitecture is currently
assigning the warmupSteps parameter directly to WarmupSteps; add input
validation in the constructor (e.g., in the parameter-handling block where
WarmupSteps is set) to ensure warmupSteps is a positive non-zero integer—if
invalid either throw an ArgumentOutOfRangeException with a clear message or
fallback to the default (4000) before assigning to the WarmupSteps property;
update any unit tests or callers as needed to expect the exception or new
behavior.
In
`@tests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cs`:
- Around line 413-436: MakeArch currently hardcodes warmupSteps:10 which changes
optimizer scheduling for callers like ExplicitAdamMatchesDefaultBehavior; change
MakeArch signature to accept an int warmupSteps (with default 10) and pass that
value into the TransformerArchitecture constructor (warmupSteps: warmupSteps),
then update the caller ExplicitAdamMatchesDefaultBehavior to call MakeArch with
the warmup value that keeps both paths equivalent (e.g., warmupSteps:0 or the
same schedule used in the explicit-Adam path) so the test compares identical
optimizer schedules.
---
Duplicate comments:
In `@tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs`:
- Around line 55-99: Add a new test that exercises the exporter end-to-end:
build the same symbolic-axes relu model but obtain bytes via
OnnxExporter.ExportToBytes (call ExportToBytes on the model/export path used
elsewhere), create an InferenceSession from those exported bytes, and then
invoke AssertReluRoundtrip on that single session at the three spatial shapes
(1×3×224×224, 1×3×320×320, 2×3×256×192) to ensure reuse across shapes; reference
the existing test
Graph_WithSymbolicAxes_RunsAtMultipleSpatialSizes_WithoutReExport for shape
choices and use the same AssertReluRoundtrip helper and the InferenceSession
constructor to validate runtime behavior.
🪄 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: 4ab83edc-de49-4587-a7e9-320b3e553005
📒 Files selected for processing (9)
src/Helpers/LayerHelper.cssrc/Initialization/EagerInitializationStrategy.cssrc/Initialization/InitializationStrategyBase.cssrc/LearningRateSchedulers/NoamSchedule.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/NeuralNetworks/Transformer.cssrc/NeuralNetworks/TransformerArchitecture.cstests/AiDotNet.Tests/IntegrationTests/NeuralNetworks/TransformerEndToEndIntegrationTests.cstests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs
addresses 3 unresolved review comments on pr #1269: transformerarchitecture.cs (pqdj): - xml param docs added for sequencepooling, warmupsteps, randomseed. the constructor signature was already including these params but the <param> doc list hadn't been updated when they were introduced. doc copy mirrors the equivalent fix shipped on pr #1270 for the same ctor's vaswani-side doc gap onnxsymbolicaxisruntimetests.cs (pqdo): - replace naive utf-8 substring scan with protobuf length-prefixed-string byte-pattern search. the previous Assert.Contains("H") / Assert.Contains("W") could match any single 'H' or 'W' byte in the protobuf binary by chance (e.g., as part of a protobuf field tag, or random data in a non-shape-related field). new helper assertlengthprefixedstringinbytes searches for the exact `<varint-length-byte> <utf-8-bytes>` sequence that protobuf's length-delimited encoding produces for a dim_param — random binary is essentially impossible to produce that pattern by chance, especially for the 30+ char sentinels the first test now uses (DYNAMIC_BATCH_AXIS_SENTINEL_TEST etc.). the second test (which uses framework defaults "batch"/"H"/"W") gets the same byte-pattern strengthening so even single-letter axis names are matched only when they appear with the protobuf length prefix in front of them. tests still pass layerhelper.cs (pqdz): - replace the single shared eagerinitializationstrategy across all transformer layers with per-layer seeded strategies: build a seed-rng from the architecture seed, derive a fresh int per layer, construct that layer's strategy backed by its own private system.random. preserves determinism (same architecture seed -> same layer seeds -> same weights) and eliminates the data race between concurrent lazy-init paths sharing one non-thread-safe rng. mirrors the same fix on the vaswani branch for the same code path builds cleanly, onnx tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs (1)
222-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen
dim_paramwire checks to avoid false-positive passes.
AssertLengthPrefixedStringInBytescurrently matches only<len><bytes>, which can appear in unrelated protobuf payloads (notably for one-character names like"H"/"W"). Match thedim_paramfield tag (0x12) as part of the pattern so this fails reliably if symbolic-axis emission regresses.Proposed minimal fix
private static void AssertLengthPrefixedStringInBytes(byte[] haystack, string needle) { + const byte DimParamFieldTag = 0x12; // field 2, length-delimited var nameBytes = System.Text.Encoding.UTF8.GetBytes(needle); if (nameBytes.Length >= 0x80) { // Multi-byte varint length encoding — not used by any current // test sentinel. Add support if a future test needs it. throw new System.NotSupportedException( $"Test sentinel '{needle}' is {nameBytes.Length} bytes; the assertion " + "helper currently only supports single-byte varint length prefixes. " + "Either shorten the sentinel or extend the helper for multi-byte varints."); } byte lengthPrefix = (byte)nameBytes.Length; - // Slide a window of length (1 + nameBytes.Length) across the bytes - // looking for `lengthPrefix nameBytes[0] nameBytes[1] ...`. - for (int start = 0; start <= haystack.Length - 1 - nameBytes.Length; start++) + // Slide a window of length (2 + nameBytes.Length) across the bytes + // looking for `0x12 lengthPrefix nameBytes[0] nameBytes[1] ...`. + for (int start = 0; start <= haystack.Length - 2 - nameBytes.Length; start++) { - if (haystack[start] != lengthPrefix) continue; + if (haystack[start] != DimParamFieldTag || haystack[start + 1] != lengthPrefix) continue; bool match = true; for (int j = 0; j < nameBytes.Length; j++) { - if (haystack[start + 1 + j] != nameBytes[j]) { match = false; break; } + if (haystack[start + 2 + j] != nameBytes[j]) { match = false; break; } } if (match) return; } Assert.Fail( - $"Could not find protobuf length-prefixed encoding of '{needle}' " + - $"({1 + nameBytes.Length} bytes: 0x{lengthPrefix:X2} followed by the UTF-8 " + + $"Could not find protobuf dim_param encoding of '{needle}' " + + $"({2 + nameBytes.Length} bytes: tag 0x12, 0x{lengthPrefix:X2}, then UTF-8 " + $"of '{needle}') anywhere in the {haystack.Length}-byte ONNX graph. " + $"This means the symbolic axis was NOT emitted as a dim_param in the " + $"wire format — the axis is being written as a fixed dim_value instead, " + $"contradicting the issue `#1211` contract."); }As per coding guidelines, tests must assert specific expected values and avoid weak assertions that can pass for unrelated data.
🤖 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/Onnx/OnnxSymbolicAxisRuntimeTests.cs` around lines 222 - 255, AssertLengthPrefixedStringInBytes currently searches for only the length-prefixed UTF‑8 bytes and can match unrelated protobuf fields; update the search to require the ONNX dim_param field tag (0x12) immediately before the length prefix. In the AssertLengthPrefixedStringInBytes helper, change the sliding-window pattern to look for the three-part sequence [0x12, lengthPrefix, nameBytes...] (adjust the window bounds to account for the extra tag byte) and update the failure message to mention the dim_param field tag so the test will only pass when a real dim_param wire encoding is present; reference the AssertLengthPrefixedStringInBytes method and the 0x12 dim_param field tag in your changes.
🤖 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/Helpers/LayerHelper.cs`:
- Around line 1689-1706: The comment claiming "exact same weights every run" is
too strong because System.Random does not guarantee cross-framework identical
sequences; edit the block around Random? seedRng = architecture.RandomSeed... to
remove or narrow that claim (e.g., state "deterministic within the same .NET
runtime/version" or "deterministic for a given runtime implementation"), or
replace the comment and implement a stable PRNG abstraction (e.g., a
Xoshiro256-based PRNG) used to derive per-layer seeds if cross-framework
reproducibility is required; ensure the updated comment references the symbol
Random? seedRng and architecture.RandomSeed so future readers know where the
seed RNG is created and the limitations or the alternative PRNG used.
In `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 420-421: Validate the warmupSteps parameter in the
TransformerArchitecture constructor (the overload accepting int warmupSteps,
int? randomSeed) by checking that warmupSteps > 0 at the constructor entry and
throw an ArgumentOutOfRangeException (including the parameter name
"warmupSteps") if the check fails; ensure this validation occurs before
assigning to any fields (e.g., where warmupSteps is currently stored around the
existing assignments at/after the constructor body) so invalid scheduler configs
are rejected immediately.
---
Duplicate comments:
In `@tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs`:
- Around line 222-255: AssertLengthPrefixedStringInBytes currently searches for
only the length-prefixed UTF‑8 bytes and can match unrelated protobuf fields;
update the search to require the ONNX dim_param field tag (0x12) immediately
before the length prefix. In the AssertLengthPrefixedStringInBytes helper,
change the sliding-window pattern to look for the three-part sequence [0x12,
lengthPrefix, nameBytes...] (adjust the window bounds to account for the extra
tag byte) and update the failure message to mention the dim_param field tag so
the test will only pass when a real dim_param wire encoding is present;
reference the AssertLengthPrefixedStringInBytes method and the 0x12 dim_param
field tag in your changes.
🪄 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: 7ebd4b58-12a4-4f55-9028-c78f26f7055d
📒 Files selected for processing (3)
src/Helpers/LayerHelper.cssrc/NeuralNetworks/TransformerArchitecture.cstests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/Helpers/LayerHelper.cs:1
- This determinism mechanism relies on
System.Randomboth to derive per-layer seeds and to drive initialization.System.Random’s algorithm differs across runtimes (notably .NET Framework vs modern .NET), sorandomSeed=42may yield different initial weights between net471 and net10, potentially reintroducing cross-TFM CI inconsistencies. If cross-runtime determinism is a requirement, consider deriving per-layer seeds via a runtime-stable function (e.g., a small custom PRNG/LCG or hashing(randomSeed, layerIndex)), and/or using a known-stable RNG implementation for sampling.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
addresses 3 unresolved review comments on pr #1269 + an underlying onnx exporter limitation surfaced by the test rewrite: transformerarchitecture.cs (vur5/vzgk): - validate warmupsteps > 0 in the ctor with argumentoutofrangeexception. previously a 0/negative value bubbled up later as an exception from the noamschedule ctor during transformer construction or training, obscuring the root cause. mirrors the equivalent fix on pr #1270's branch for the same parameter onnxsymbolicaxisruntimetests.cs (vzge): - assertlengthprefixedstringinbytes helper now also checks the protobuf field-tag byte (0x12 = field 2, wire type LEN) preceding the length+name. previously only matched <length><name>, so a stray length+name byte sequence elsewhere in the protobuf could falsely match. closes review-comment #1269.vzge onnxsymbolicaxisruntimetests.cs (vzgt): - tinyvisionnet stub now uses dense + activation (relu) instead of conv + maxpool. the onnxexporter only handles dense / linear / fullyconnected / activations / dropout / flatten — using conv/pool meant the exporter was silently skipping those layers, making the test brittle on (a) the silent-skip behaviour being load-bearing and (b) conv/pool's 4d shape contract not matching the rank-3 dynamic- spatial architecture the test sets up onnxexporter.cs (underlying): - exportdenselayer reflection now falls back from "weights" / "bias" property accessors to getweights() / getbiases() method accessors, the layerbase<t> standard surface. without this fallback the reflection probe couldn't reach denselayer's weights at all - converttofloatmatrix and converttofloatarray gain tensor<t> support via reflection (shape-walk for rank-2 weights, length+indexer for rank-1 biases). cross-tfm safe via tensor<t>.shape's int[] / TensorShape duck-typed extraction - both fallbacks unblock any test or production path that puts a layerbase<t>-derived dense layer through the exporter — what the reviewer's vzgt comment was indirectly flagging builds cleanly, both onnx tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
layerhelper.cs comment expanded to acknowledge the limit explicitly: same-runtime-same-seed determinism is what the per-layer rng plumbing provides, but cross-runtime determinism (net471 vs net10 producing identical sequences for the same seed) is NOT — system.random uses a runtime-specific algorithm (knuth subtractive on net471, xoshiro256** on .net core 6+). a stable-prng option (vendored xoshiro256** running identically on both targets) is tracked separately. closes vur1. pr description updated separately on github to acknowledge the adjacent transformer-side scope (noam step indexing, optimizer single-source, warmupsteps validation, embeddinglayer seed respect, onnxexporter tensor support) — closes vzfl. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
left a comment
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Onnx/OnnxExporter.cs (1)
401-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winActually fall back when the property value is unusable.
This only calls
GetWeights()/GetBiases()when the property is missing. If a layer exposesWeights/Biasbut returnsnullor a non-convertible wrapper there, the exporter never tries the method path and still fails, which keeps the reflection path brittle for hybrid layer implementations.Suggested fix
if (weightsProp is not null) { weights = ConvertToFloatMatrix(weightsProp.GetValue(layer), numOps); } -else +if (weights is null) { var getWeightsMethod = layerType.GetMethod("GetWeights", System.Type.EmptyTypes); if (getWeightsMethod is not null) { weights = ConvertToFloatMatrix(getWeightsMethod.Invoke(layer, null), numOps); } } if (biasProp is not null) { bias = ConvertToFloatArray(biasProp.GetValue(layer), numOps); } -else +if (bias is null) { var getBiasesMethod = layerType.GetMethod("GetBiases", System.Type.EmptyTypes); if (getBiasesMethod is not null) { bias = ConvertToFloatArray(getBiasesMethod.Invoke(layer, null), numOps); } }🤖 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 `@src/Onnx/OnnxExporter.cs` around lines 401 - 425, The current logic only calls GetWeights/GetBiases when the Weights/Bias properties are absent; change it so if weightsProp or biasProp exists but their value is null or conversion to float fails, you fall back to the method path. For both weights and bias: read the property value from weightsProp.GetValue(layer) / biasProp.GetValue(layer) into a temp, try converting via ConvertToFloatMatrix / ConvertToFloatArray inside a try/catch (or check for null/invalid return), and if conversion throws or returns null, obtain and invoke getWeightsMethod/getBiasesMethod on layerType to convert that result instead. Use the existing symbols weightsProp, biasProp, ConvertToFloatMatrix, ConvertToFloatArray, getWeightsMethod, getBiasesMethod, layerType and layer.
♻️ Duplicate comments (3)
src/NeuralNetworks/TransformerArchitecture.cs (1)
333-379:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign constructor XML
<param>tags with the actual signature.The docs still list
inputHeight,inputWidth,inputDepth, andrbmLayers, but those parameters are not in this constructor. This leaves IntelliSense misleading.Suggested doc-only fix
- /// <param name="inputHeight">The height of the input for 2D inputs like images. Defaults to 0.</param> - /// <param name="inputWidth">The width of the input for 2D inputs like images. Defaults to 0.</param> - /// <param name="inputDepth">The depth of the input for multi-channel inputs. Defaults to 1.</param> @@ - /// <param name="rbmLayers">Optional Restricted Boltzmann Machine layers for the network. Defaults to null.</param>🤖 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 `@src/NeuralNetworks/TransformerArchitecture.cs` around lines 333 - 379, The XML docs for the TransformerArchitecture constructor are out of sync: remove or update the <param> tags that don't exist in the actual constructor signature (specifically remove or correct inputHeight, inputWidth, inputDepth, and rbmLayers) in TransformerArchitecture's constructor XML comments so IntelliSense matches the method parameters; locate the ctor comment block near the TransformerArchitecture(...) constructor and either delete those obsolete <param> entries or replace them with the actual parameter names present in the signature (e.g., inputSize, inputType, taskType, numEncoderLayers, numDecoderLayers, etc.).tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs (2)
184-217:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftThe exporter-path test can pass with an unusable graph.
This warms
DenseLayer<float>with[1, 16]and then exports the same model as[1, 3, 32, 32]. The symbolic-axis strings can still be present even if the emittedMatMulcontract is shape-invalid, so this never proves thatOnnxExporter.ExportToBytes(...)produced a runnable model. Please either keep the warm-up/export shapes consistent or run the exported bytes through a singleInferenceSessionacross multiple shapes.As per coding guidelines, tests must be production-quality and should “Assert specific expected values, not just non-null.”
🤖 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/Onnx/OnnxSymbolicAxisRuntimeTests.cs` around lines 184 - 217, The test currently warms DenseLayer via MakeRamp(new[] { 1, 16 }) then exports with OnnxExporter.ExportToBytes(model, new[] { 1, 3, 32, 32 }) which can leave a non-runnable graph passing the symbol-name checks; fix by making the warm-up and export shapes consistent or by validating the exported bytes with an actual runtime: either change the warm-up call to MakeRamp(new[] { 1, 3, 32, 32 }) and run the same Forward calls on model.Layers[...] before exporting, or keep the existing warm-up but load the produced onnxBytes into an InferenceSession (or equivalent) and run a simple inference using an input tensor shaped [1,3,32,32], asserting the session returns a valid output; ensure you still call AssertLengthPrefixedStringInBytes(onnxBytes, ...) afterwards.
232-278:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftStop hand-scanning protobuf for
dim_param.This helper still proves only a byte pattern, not the actual
TensorShapeProto.Dimension.dim_paramfields, and it explicitly throws once the symbolic name needs a multi-byte varint length. That is still a brittle, partial test oracle—especially for one-character names like"H"and"W". Parse the ONNX model and assert the exactdim_paramvalues on the relevant input/output dims instead.As per coding guidelines, tests must be production-quality and should “Assert specific expected values, not just non-null.”
🤖 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/Onnx/OnnxSymbolicAxisRuntimeTests.cs` around lines 232 - 278, Replace the brittle byte-pattern scan in AssertLengthPrefixedStringInBytes with a real ONNX protobuf parse: parse the haystack bytes into an Onnx.ModelProto (using the protobuf parser for ModelProto), locate the appropriate GraphProto inputs/outputs and their TypeProto.tensor_type.shape.dim entries, and assert that at least one Dimension.dim_param equals the expected needle; ensure the helper handles multi-byte varint lengths implicitly by using the parsed structure (refer to AssertLengthPrefixedStringInBytes, TensorShapeProto.Dimension.dim_param, ModelProto/GraphProto/TypeProto APIs) and remove the manual tag/length/name byte comparisons and the NotSupportedException branch.
🤖 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.
Outside diff comments:
In `@src/Onnx/OnnxExporter.cs`:
- Around line 401-425: The current logic only calls GetWeights/GetBiases when
the Weights/Bias properties are absent; change it so if weightsProp or biasProp
exists but their value is null or conversion to float fails, you fall back to
the method path. For both weights and bias: read the property value from
weightsProp.GetValue(layer) / biasProp.GetValue(layer) into a temp, try
converting via ConvertToFloatMatrix / ConvertToFloatArray inside a try/catch (or
check for null/invalid return), and if conversion throws or returns null, obtain
and invoke getWeightsMethod/getBiasesMethod on layerType to convert that result
instead. Use the existing symbols weightsProp, biasProp, ConvertToFloatMatrix,
ConvertToFloatArray, getWeightsMethod, getBiasesMethod, layerType and layer.
---
Duplicate comments:
In `@src/NeuralNetworks/TransformerArchitecture.cs`:
- Around line 333-379: The XML docs for the TransformerArchitecture constructor
are out of sync: remove or update the <param> tags that don't exist in the
actual constructor signature (specifically remove or correct inputHeight,
inputWidth, inputDepth, and rbmLayers) in TransformerArchitecture's constructor
XML comments so IntelliSense matches the method parameters; locate the ctor
comment block near the TransformerArchitecture(...) constructor and either
delete those obsolete <param> entries or replace them with the actual parameter
names present in the signature (e.g., inputSize, inputType, taskType,
numEncoderLayers, numDecoderLayers, etc.).
In `@tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs`:
- Around line 184-217: The test currently warms DenseLayer via MakeRamp(new[] {
1, 16 }) then exports with OnnxExporter.ExportToBytes(model, new[] { 1, 3, 32,
32 }) which can leave a non-runnable graph passing the symbol-name checks; fix
by making the warm-up and export shapes consistent or by validating the exported
bytes with an actual runtime: either change the warm-up call to MakeRamp(new[] {
1, 3, 32, 32 }) and run the same Forward calls on model.Layers[...] before
exporting, or keep the existing warm-up but load the produced onnxBytes into an
InferenceSession (or equivalent) and run a simple inference using an input
tensor shaped [1,3,32,32], asserting the session returns a valid output; ensure
you still call AssertLengthPrefixedStringInBytes(onnxBytes, ...) afterwards.
- Around line 232-278: Replace the brittle byte-pattern scan in
AssertLengthPrefixedStringInBytes with a real ONNX protobuf parse: parse the
haystack bytes into an Onnx.ModelProto (using the protobuf parser for
ModelProto), locate the appropriate GraphProto inputs/outputs and their
TypeProto.tensor_type.shape.dim entries, and assert that at least one
Dimension.dim_param equals the expected needle; ensure the helper handles
multi-byte varint lengths implicitly by using the parsed structure (refer to
AssertLengthPrefixedStringInBytes, TensorShapeProto.Dimension.dim_param,
ModelProto/GraphProto/TypeProto APIs) and remove the manual tag/length/name byte
comparisons and the NotSupportedException branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18bc90a4-a8d4-466d-88bf-aec2f3462a2f
📒 Files selected for processing (4)
src/Helpers/LayerHelper.cssrc/NeuralNetworks/TransformerArchitecture.cssrc/Onnx/OnnxExporter.cstests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs
left a comment
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… override addresses 2 unresolved review comments on pr #1269: onnxsymbolicaxisruntimetests.cs (xzd-): - tinyvisionnet stub now uses two shape-preserving relu activation layers and warms up at the SAME [1, 3, 32, 32] shape that's passed to onnxexporter.exporttobytes. previous version warmed dense at [1, 16] but exported with [1, 3, 32, 32] — produced an onnx graph where matmul's input rank/inner-dim didn't align with the declared input shape, so the exported model wasn't runnable even though the symbolic-axis byte assertions passed. activation-only layers are rank-agnostic so the same 4d tensor flows from warmup through both layers and into export without any matmul/conv weight-shape concerns noamschedule.cs (xzeb): - override Reset() to restore the warmup-start lr (t=1) instead of the peak _baseLearningRate. _baseLearningRate is set to the peak lr via ComputePeakLr to satisfy the base ctor's positive-lr guard, so the default base.Reset() would set _currentLearningRate to the peak — skipping warmup and jumping straight to the peak lr on any subsequent training run. mirrors the equivalent fix on pr #1270's branch for the same scheduler builds cleanly net10.0, both onnx tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
left a comment
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
left a comment
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs (1)
232-278:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winParse the ONNX model instead of byte-scanning it.
This helper still only proves
0x12 <len> <bytes>exists somewhere in the protobuf. For short axis names like"H"/"W", that can still false-pass on unrelated field-2 strings, and it never verifies which tensor owns thedim_paramor whether both graph input and output were annotated. Please assert the parsedModelProtoinput/output shape dims directly.#!/bin/bash set -euo pipefail # Check whether structured ONNX protobuf types are already available in the repo. rg -n -C2 --glob 'Directory.Packages.props' --glob '*.csproj' 'Google\.Protobuf|Onnx' rg -n -C2 --type=cs '\b(ModelProto|TensorShapeProto)\b'As per coding guidelines, tests "MUST be production-quality" and should "Assert specific expected values, not just non-null."
🤖 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/Onnx/OnnxSymbolicAxisRuntimeTests.cs` around lines 232 - 278, Replace the brittle byte-scanning helper AssertLengthPrefixedStringInBytes with a proper protobuf parse: use the ONNX ModelProto types (ModelProto.Parser.ParseFrom(haystack) or ModelProto.MergeFrom) to deserialize the haystack, then locate the specific ValueInfoProto(s) or TensorProto(s) for the graph inputs/outputs you expect and assert their TensorShapeProto.Dimension.dim_param equals the needle (string) for the correct tensor(s); update the test in OnnxSymbolicAxisRuntimeTests to call this new check (reference the existing AssertLengthPrefixedStringInBytes symbol to find and remove/replace it) and add explicit assertions that both input and output shapes contain the expected dim_param rather than searching raw bytes.
🤖 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/LearningRateSchedulers/NoamSchedule.cs`:
- Around line 87-101: Update the XML doc comment on the Reset() override to
remove the PR-specific/internal reference "#1269.xZeb" and any other transient
review-tag; keep the explanatory text that explains why Reset() sets
_currentLearningRate = ComputeLearningRate(1) after calling base.Reset()
(referencing Reset(), _currentLearningRate and ComputeLearningRate for context)
so the comment remains accurate and self-contained without the review-specific
link.
In `@tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs`:
- Around line 173-218: The test
OnnxExporter_RealModel_EmitsSymbolicAxes_ViaReflection uses TinyVisionNet
(activation-only) so it never exercises the new weight/bias export path; add a
companion test that builds a small model with at least one weight-bearing layer
(e.g., a Conv2D or Linear layer) and run it through OnnxExporter.ExportToBytes
with the same dynamic-spatial input shape, then assert both that bytes are
emitted and that length-prefixed weight/bias names (or symbolic axes) appear;
ensure the new test forces the exporter fallback paths for
GetWeights()/GetBiases() and Tensor<T> shape handling so those code paths are
exercised (mirror the existing assertions like AssertLengthPrefixedStringInBytes
for "batch","H","W" and add checks for exported weight/bias identifiers).
---
Duplicate comments:
In `@tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs`:
- Around line 232-278: Replace the brittle byte-scanning helper
AssertLengthPrefixedStringInBytes with a proper protobuf parse: use the ONNX
ModelProto types (ModelProto.Parser.ParseFrom(haystack) or ModelProto.MergeFrom)
to deserialize the haystack, then locate the specific ValueInfoProto(s) or
TensorProto(s) for the graph inputs/outputs you expect and assert their
TensorShapeProto.Dimension.dim_param equals the needle (string) for the correct
tensor(s); update the test in OnnxSymbolicAxisRuntimeTests to call this new
check (reference the existing AssertLengthPrefixedStringInBytes symbol to find
and remove/replace it) and add explicit assertions that both input and output
shapes contain the expected dim_param rather than searching raw bytes.
🪄 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: dc0a92cf-cdae-49fb-b7c3-dc8cfb232b84
📒 Files selected for processing (2)
src/LearningRateSchedulers/NoamSchedule.cstests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs
…w random mirrors the same fix applied to pr #1270's branch — never use bare new Random(), always go through RandomHelper.CreateSeededRandom for the deterministic-but-cryptographically-secure path. layerhelper.cs two sites converted: architecture-seed-rng construction (line 1714) and per-layer seeded strategy (line 1727). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous commit (91be733) reduced LayerHelper.Wire to a no-op (`_ = seedRng; return layer;`) after stripping the EagerInitializationStrategy override that was silently changing every DenseLayer's init from activation-aware He / LeCun to Xavier-normal. Leaving in the no-op was unacceptable for production code — the architecture's RandomSeed wasn't actually reaching any layer's weight init, so reproducibility was broken. This commit lands the industry-standard layer-level seed pattern: - LayerBase<T> exposes `public int? RandomSeed { get; set; }`. Each layer's own InitializeParameters reads it and seeds its RNG via RandomHelper.CreateSeededRandom(seed.Value), so the layer's natural init algorithm runs unchanged but with reproducible RNG state. - LayerHelper.Wire derives a per-layer seed from the architecture's seed-RNG via seedRng.Next() and assigns it to LayerBase<T>.RandomSeed on each layer it constructs. Wire is called in layer-construction order so the per-layer seed sequence is deterministic for a given architecture seed. - EmbeddingLayer reads RandomSeed for both the embedding-tensor SimdRandom fill (closes #1270.vhmx) and the projection-weights Xavier fill (closes #1270.xElw). Drops the prior InitializationStrategy.RandomGenerator hack — the strategy-side accessor that was added is now reverted. - DenseLayer's activation-aware fallback (He / LeCun) reads RandomSeed and threads it into HeInitializationStrategy / LeCunInitializationStrategy via their new Random?-accepting ctor overloads. - HeInitializationStrategy and LeCunInitializationStrategy gain a `(Random? rng)` ctor that forwards to InitializationStrategyBase's existing seeded-RNG ctor; existing parameterless / bool-only ctors are preserved. - InitializationStrategyBase's parallel Xavier fill switches its per-thread `new Random(seed)` calls to RandomHelper.CreateSeededRandom, keeping the codebase-wide rule (never construct System.Random directly) intact even on the parallel-fill path. Closes review-comment #1270.yA1v (the prior Wire override was a behaviour change, not just a determinism fix).
left a comment
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- TransformerArchitecture: drop the stray <param name="rbmLayers"> XML doc entry (no such ctor parameter exists) so doc-gen + IntelliSense stop reporting CS1573. Closes #1269.yA64. - TransformerEndToEnd integration test: narrative comments now reference the actual LR values used in the test (0.01 aggressive / 1e-5 slow), not the stale 0.1 / 1e-3 from a prior revision. The 3x-loss-gap assertion is unchanged. Closes #1269.yA7b. - NoamSchedule.Reset XML doc: drop the PR-specific #1269.xZeb reference; the explanation of why warmup-start LR is restored remains intact. Closes #1269.yEOI. - OnnxSymbolicAxisRuntimeTests: add companion OnnxExporter_WeightBearingLayer_ExportsTensorWeights_ViaReflection test using a TinyDenseNet (single DenseLayer 4->2). Asserts the exporter produces non-empty bytes AND the graph contains the length-prefixed UTF-8 of "dense_0_weights" / "dense_0_bias" — only emitted when GetWeights()/GetBiases() reflection actually returns a Tensor<T> that the exporter walks. The pre-existing activation-only TinyVisionNet test only verifies dim_param emission; this companion locks in the new reflected weight/bias export path so it can't silently regress. Closes #1269.yEOQ.
ComputeLearningRate previously mapped step <= 0 to t=1, which collapsed both step=0 (ctor / Reset) AND step=1 (after the first OnBatchEnd) onto the paper's t=1 — so batches 0 and 1 used identical LRs and the entire warmup/decay curve was shifted by one step. Industry-standard fix matching tensor2tensor's reference implementation (Vaswani's own code uses `global_step + 1`) and the Hugging Face / PyTorch convention for warmup schedulers driven from a 0-indexed step counter: - ComputeLearningRate maps the 0-indexed framework step to the paper's 1-indexed t via t = step + 1. - Ctor and Reset() now call ComputeLearningRate(0) so the pre-Step() LR equals the paper's t=1 (batch 0's LR). Result: batch N runs with paper-t = N+1 LR, OnBatchEnd advances _currentStep to N+1 so batch N+1 picks up paper-t = N+2 LR. The warmup-from-tiny semantic is preserved and no two consecutive batches share the same LR. Closes #1269.yuXt.
Mirror the PR #1270 fix here so PR #1269 stays in lock-step: MultiHeadAttentionLayer.InitializeParameters now reads LayerBase<T>.RandomSeed and seeds its SimdRandom from it. Without this hook, TransformerArchitecture.RandomSeed flowed through LayerHelper.Wire to MHA's RandomSeed property but MHA's init was still pulling unseeded SimdRandom values, so attention weights stayed non-deterministic across runs even with a fixed architecture seed. Industry-standard layer-level seed pattern (matches PyTorch's nn.Module + torch.manual_seed cascade): each layer reads its own RandomSeed and constructs its own RNG, so concurrent lazy-init paths get independent RNG instances and never share mutable state.
left a comment
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- TransformerArchitecture.RandomSeed XML docs: rewritten to describe the layer-level RandomSeed pattern (LayerHelper assigns LayerBase<T>.RandomSeed; each layer reads it and seeds its own RNG via RandomHelper.CreateSeededRandom). Replaces the stale EagerInitializationStrategy / Xavier-init claim. Also adds the determinism-scope paragraph noting System.Random is runtime-specific. Closes #1269.zFtJ. - NeuralNetworkBase.TrainWithCustomLoss: stepped path now also calls OnBatchEnd on the optimizer, matching the TrainWithTape branch and the legacy fallback. Without this, per-batch schedulers (Noam, warmup, cosine) attached to an optimizer remained inert when a caller used TrainWithCustomLoss instead of Train(). Closes #1269.zFt3. - OnnxExporter Tensor<T> conversion: added fast path that pulls the underlying T[] via a SINGLE GetDataArray() reflection invoke, then copies+casts in a tight non-reflective loop. The previous per-element PropertyInfo.GetValue reflection inside nested loops was O(rows*cols) reflection dispatches — for a 4096x4096 attention weight that's 16M reflective calls and was unusably slow on realistic models. Same fast path applied to the 1D bias branch. Per-element indexer fallback retained for custom Tensor<T> implementations that don't expose GetDataArray(). Removed the redundant System.Convert.ToDouble wrapper around numOps.ToDouble (which already returns double). Closes #1269.zFuH.
This was referenced May 7, 2026
This was referenced May 15, 2026
3 of 4 tasks
This branch had an error being deployed
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
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.
Summary
Closes the remaining #1211 spec item: prove the symbolic-axis wire format actually binds dynamically at inference time. PR #1259 shipped the
OnnxAxisSpec/dim_paraminfrastructure; this PR adds the missing ONNX Runtime end-to-end coverage the issue asked for.This PR also expanded scope during review iteration to include adjacent Transformer-side correctness fixes (Noam scheduler step indexing, deterministic per-layer init RNG, optimizer single-source-of-truth between
_optimizerandBaseTrainOptimizer,OnBatchEndmirrored intoTrainWithCustomLoss,warmupSteps > 0validation inTransformerArchitecturector, EmbeddingLayer respecting architecture-supplied seed, OnnxExporter reflection probingGetWeights()/GetBiases()method accessors and Tensor shape extraction). The Transformer-side changes were originally on a sibling branch (PR #1270) but several reviewers re-flagged the same issues here, so the equivalent fixes landed on this branch too — the diffs are essentially duplicates of #1270's review-comment commits, applied to make this branch self-contained for merge. Closes review-comment #1269.vzFL (PR scope mismatch acknowledged).Closes #1211.
What's in
tests/AiDotNet.Tests/Onnx/OnnxSymbolicAxisRuntimeTests.cs(new)Two tests:
Graph_WithSymbolicAxes_RunsAtMultipleSpatialSizes_WithoutReExportbuilds a minimal ONNX graph (input → ReLU → output) with batch / H / W marked symbolic via long sentinel names (e.g.DYNAMIC_BATCH_AXIS_SENTINEL_TEST), exports it, asserts the protobuf TensorShapeProto.Dimensiondim_param(field tag 0x12) byte-pattern is present for each axis, then runs the graph throughMicrosoft.ML.OnnxRuntime'sInferenceSessionat three different shapes through the SAME session (1×3×224×224 / 1×3×320×320 / 2×3×256×192). Gated#if !NET471because ORT's native binding crashes on full framework in this test environment.OnnxExporter_RealModel_EmitsSymbolicAxes_ViaReflectionputs a realNeuralNetworkBasesubclass withHasDynamicSpatialDims = truethroughOnnxExporter.ExportToBytesand asserts the symbolic axis names appear in the produced bytes — proving the field-aware reflection probe (PR feat(#1209,#1214): lazy shape inference + architecture-optional NeuralNetworkBase #1259'sHasDynamicSpatialAxesreadsArchitectureas a public field, not a property) actually fires for a real NN model. Without this test the reflection fix would have been verified at the wire-format level only.Side-effects on
src/Onnx/OnnxExporter.cs: Tensor-aware weight extraction (handles bothWeightsproperty andGetWeights()method, and bothMatrix<T>andTensor<T>shape conventions) so the reflection-path test can drive a real LayerBase-derived model end-to-end.Test plan
Graph_WithSymbolicAxes_RunsAtMultipleSpatialSizes_WithoutReExportpasses on net10OnnxExporter_RealModel_EmitsSymbolicAxes_ViaReflectionpasses on net10 + net471🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests