Conversation
Six family base classes, each declaring a model family's shared knobs once: - ModelHyperparameterOptions (new, Models/Options) — MaxGradNorm plus the Require() guards every family shares. - SequenceModelOptions, VisionLanguageModelOptions, EmbeddingModelOptions, GanOptions (new, NeuralNetworks/Options). - DocumentNeuralNetworkOptions — EXTENDED rather than replaced. All 29 Document options classes already derive from it, so adding the shared knobs there reaches the whole area without touching a leaf. - VideoHyperparameterOptions (new, Video/Options). Video has no base in use: 96 of its 108 options classes derive straight from NeuralNetworkOptions. Properties are non-nullable with no base default. A shared default would be wrong for nearly every model that inherits it — NumLayers = 12 is right for BERT and wrong for Mamba-130M. Per-model values go in each leaf's constructor in phases 2-7. Until a leaf is wired, Require() throws naming the property rather than letting a zero-width model through silently. The ratchet counts tunable defaulted constructor parameters that have no correspondingly-named property on their model's options type, resolving models by transitive reflection over NeuralNetworkBase<T> — BGE derives from TransformerEmbeddingNetwork, TrOCR from DocumentNeuralNetworkBase, and only 3 files under src/NeuralNetworks name the base directly, so no naming or path heuristic finds them. BASELINE IS 1067, not the 806 the spec estimated. The file-based estimate only looked at the three areas the spec named. Reflection also finds Tacotron2Model (20 params), TtsModel (17) and VITSModel (16) configured against a generic OnnxModelOptions, and SpeechEmotionRecognizer (11) with no options parameter at all. Those areas scored clean on "do their Options classes declare properties", which turns out to be a different question from "do the models read them". The behavioural assertion — that setting an options property actually changes the model — is written and skipped until phase 2, when the first family reads its options and there is something for it to assert against. Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Phase 1's ratchet measurement pulled TextToSpeech, SpeechRecognition and Audio into scope: 22 models carrying 192 tunable constructor parameters between them, including the three largest single offenders in the library (Tacotron2Model at 20, TtsModel at 17, VITSModel at 16, all configured against a generic OnnxModelOptions). The knobs are dominated by signal parameters rather than network shape — sampleRate appears in 21 of the 22 models — so they get their own base rather than being folded into an existing one. It spans three source areas, so it lives in Models/Options beside DocumentNeuralNetworkOptions. Canonicalises two pairs of synonyms the constructors currently use interchangeably: hopLength/hopSize and fftSize/frameSize. Build green, ratchet unchanged at 1067 — nothing inherits from this yet, which is correct until the wiring phases. Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…onfig Establishes the per-model pattern for the sequence family: - MambaOptions derives from SequenceModelOptions and sets the model's shipped defaults in its parameterless constructor. - The constructor drops its six scalar parameters, taking (architecture, options, lossFunction) and reading every value off _options. - The hand-written positivity guards are replaced by MambaOptions.Validate(), which covers the same four values plus MaxSequenceLength and ExpandFactor. Values are carried over UNCHANGED. Mamba-130M is 768x24; this ships 256x4. Correcting that is phase 9, kept apart so behaviour changes are reviewed separately from the mechanical move. Build green, no call site in src passed the removed parameters. Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ven config Moves each model's tunable ctor params onto its Options class, which now derives from SequenceModelOptions. Values are carried over UNCHANGED, read straight out of each ctor signature so nothing can drift in the move; verifying them against the papers is a later phase. Validation now throws ArgumentException naming 'options', preserving the public contract these constructors already had rather than changing it to InvalidOperationException as an incidental consequence of the move. RWKV7's model-dimension/head divisibility check is moved below the options assignment — repointing it at _options had put it above, where _options was still null. RWKV4's four hand-written positivity guards are removed as unreachable behind Validate(). Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The script that swapped <exception cref> tags filtered on 'has a Validate() method', which is far too loose: six files outside this work have their own Validate() that genuinely throws InvalidOperationException, and their docs were mislabelled as ArgumentException. Only the #2090 family bases keep the ArgumentException contract, which is the one their Require() actually throws.
Migrating 11 sequence models to options-driven configuration is a breaking change to every call site that passed the removed scalar parameters. The compiler enumerated them; there were 106 across 6 files plus the generator. - TestScaffoldGenerator: 8 construction snippets. Worth noting WHY they exist — the generator builds these models at reduced "scaffold scale" precisely because the production defaults are too large for CI (a 50,277-way LM head made one synthetic target 6.4M values). That is a legitimate use of the configuration surface, and it now goes through Options like everything else. - MambaLanguageModelTests, RWKV7LanguageModelTests: 40 call sites. - WeightImporterTests, RealModelLocalInferenceTests, LoRAFineTunerTests: 6. All 16 existing ArgumentException assertions still pass unchanged, which is the evidence that keeping ArgumentException over InvalidOperationException was the right call — the public contract of these constructors is unaltered. Ratchet 1067 -> 1004, exactly the 63 parameters moved. 88 model tests pass. Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-> 977 Finishes the six models the earlier migration guard refused because their Options classes already declared a constructor: Finch, GLA, GatedDeltaNet, Griffin, Hawk, RecurrentGemma. These six also declare COPY constructors, and GLAOptions carries an explicit warning about them: a property missing from the copy constructor does not merely vanish from the clone — the clone silently reverts to the default while the original keeps its configured value, and nothing reports the divergence. CreateNewInstance calls it. Every property added here is therefore added to the copy constructor too (28 params, 27 new base properties, all copied). Call sites needed three shapes none of the earlier scripts matched: - `: base(...)` in test subclasses (4 sites) — hand-edited. - Generator blocks covering several models at once, where the options type differs per model within one block: Griffin/Hawk and Hawk/GLA/GatedDeltaNet now derive it from model.ClassName. - A third generator shape, `scaleArgs` appended to the architecture expression. Two escaping traps, both caught by the build: a replacement string containing "$1" must not be escaped, or the capture reference is emitted literally and the declaration is deleted; and "}}" collapses to one brace only inside an INTERPOLATED string, so the same text in a plain segment emits two. Build green. 93 tests pass, ratchet 977. Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eleven multimodal models (CLIP, BLIP, BLIP-2, Flamingo, LLaVA, ImageBind, GPT-4 Vision, VideoCLIP, UnifiedMultimodal, and the two AudioVisual networks) now take their configuration through VisionLanguageModelOptions. 102 parameters moved, carried over unchanged. Three defects in my own tooling, each caught by the build rather than by care: 1. The migration handled only ONE constructor per model. Eight of these eleven declare two, and both contain `_options = options ?? new XOptions()`, so the script took the first and left the second's parameters in place — 44 params moved instead of 102. It now migrates every constructor that takes an options parameter, and the options class carries the union of their defaults. 2. Repointing a guard at _options could place it ABOVE the assignment, a null dereference. This happened in three different guard shapes (RWKV7 in phase 2, Blip2 and Flamingo here), so rather than recognising each shape the options assignment is now hoisted to the top of the constructor body. 3. Call sites received the options object as the last POSITIONAL argument, but the new signature puts `options` before the optional collaborators. It is now emitted as a named argument. Also handled: defaults that are `private const` on the model are resolved to their literal value with the constant's name kept as a comment; a default naming an open generic is reported rather than emitted into a non-generic options class. Known gap: enum- and string-typed parameters are not yet moved (VideoCLIP's TemporalAggregation, LLaVA's LanguageModelBackbone). The ratchet counts them, so they are covered in a follow-up rather than left implicit. Build green, ratchet 875. Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ratchet counts enum- and string-typed constructor parameters as
configuration, so the count could never reach its floor while the migration
moved only numeric scalars. Detection is syntactic, since the script does not
resolve types: a bare identifier type whose default reads `Type.Member` is an
enum, and `string`/`string?` with a literal default is a string. An interface
defaulting to null matches neither.
Also picks up VisionMambaModel (9 params), which sat in the sequence group but
was never run, and UnifiedMultimodalNetwork, which was silently NOT migrated in
phase 3 — see below.
A third defect in the migration script, and the reason it mattered:
the constructor-signature pattern captured its parameter list with `[\s\S]*?`.
When a model declares a parameterless constructor that chains with `: this(...)`
BEFORE the real one, the lazy match ran past it hunting for `: base`, swallowing
a whole constructor body. The replacement was then not found inside the narrower
region the script edits, so the migration did nothing while still reporting
success. A parameter list never contains braces, so the capture is now `[^{}]*?`.
That failure mode is worth noting against the ratchet itself: it counts a
parameter as covered when the Options class has a matching NAME, so
UnifiedMultimodalNetwork scored as migrated while its constructor still took all
three parameters. The behavioural assertion is what closes that hole.
The migration is also idempotent now — a default the options constructor already
assigns is not appended a second time — so it can be re-run to pick up parameter
kinds an earlier pass did not recognise.
Build green, ratchet 861.
Refs #2090
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…> 782 Eleven models (BGE, ColBERT, SGPT, SPLADE, SimCSE, Instructor, Matryoshka, FastText, GloVe, Word2Vec, TransformerEmbeddingNetwork) now configure through EmbeddingModelOptions. 77 parameters moved, carried over unchanged. PoolingStrategy is promoted out of TransformerEmbeddingNetwork<T> into AiDotNet.Enums.EmbeddingPoolingStrategy. This is the collision phase 1 deliberately deferred: a type nested in a generic class is a DISTINCT type per type argument, so TransformerEmbeddingNetwork<float>.PoolingStrategy and the <double> one were unrelated, and neither could be named from a non-generic options class. It was never usable as configuration, which is what it describes. Renamed rather than moved as-is, because src/VisionLanguage/Encoders has its own unrelated PoolingStrategy with different members. Options inheritance now mirrors model inheritance. Seven of these models derive from TransformerEmbeddingNetwork<T> and forward through `: base(...)`, so their options classes had to derive from TransformerEmbeddingOptions rather than straight from EmbeddingModelOptions — otherwise a derived model cannot pass its own options to base at all. Their duplicated pooling property and Validate() are removed; the base's are shared. Two more migration-script fixes: - A delegating `: this(...)` initializer was being repointed at _options, which is not in scope before the body and produced an argument name that was an expression. Initializers are now pruned (the target no longer takes those parameters) and only the body is repointed. - `options` IS a parameter, so an initializer needing a moved value reads `options?.MaxGradNorm ?? 1.0` — used by FastText, GloVe, Word2Vec and TransformerEmbeddingNetwork. GANs are deliberately NOT in this commit. DCGAN, BigGAN, SAGAN and ProgressiveGAN use their moved values inside `: base(CreateGeneratorArchitecture(latentSize, ...))` to build architectures before the object exists. That is per-model judgement rather than a mechanical move, so it gets its own pass instead of being forced through the script. Build green. 37 tests pass, ratchet 782. Refs #2090 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten GANs (DCGAN, WGAN, WGAN-GP, BigGAN, SAGAN, StyleGAN, ProgressiveGAN, InfoGAN,
Pix2Pix, CycleGAN) now configure through GanOptions. 41 parameters moved.
These broke three more assumptions the migration had been carrying:
1. NOT EVERY MODEL HAS AN ARCHITECTURE PARAMETER. The GAN primary constructors
take explicit image dimensions and build their generator and discriminator
architectures from them. The architecture parameter was only ever used to
place a parameter first; when absent, the required parameters lead.
2. A CONSTRUCTOR MAY DELEGATE WITH `: this(...)`, not just `: base(...)`, and
such a constructor is public and carries movable parameters of its own.
3. A MOVED VALUE MAY BE USED INSIDE THE INITIALIZER:
: base(CreateDCGANGeneratorArchitecture(latentSize, ..., generatorFeatureMaps), ...)
Dropping the identifier is wrong — the expression needs the value. `options`
IS in scope in an initializer, so it becomes `(options?.GeneratorChannels ?? 64)`,
carrying the same default. And a name that is defaulted in one constructor but
REQUIRED in another (DCGAN's latentSize) must be supplied there, not dropped.
Every argument that is a bare parameter name in an initializer is now emitted
named, because moving `options` ahead of the optional collaborators changes what
a positional argument binds to.
GanOptions.ValidateCore no longer requires LatentSize or InitialLearningRate.
This was caught by 30 failing tests, not by the build: LatentSize stays a
required CONSTRUCTOR argument on the four models that build architectures from
it, so it is legitimately unset on those options objects, and most GANs never had
an InitialLearningRate parameter at all. It now carries the DCGAN paper's 0.0002
as a default and only ImageChannels is required.
Build green. All 57 GAN tests pass, ratchet 741.
Refs #2090
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
Important Review skippedToo many files! This PR contains 134 files, which is 34 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (134)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
Closing as superseded, with the evidence rather than an assumption.
Two things worth noting about why this looked worse than it was:
The branch is not deleted. If anything here turns out not to have made it into #2158 after all, it can be recovered from |
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.
Phase 4 of #2090. Follows #2130 (phase 3). Spec in #2122.
Ratchet: 875 → 741. Three groups land here: enum/string parameters that earlier phases skipped, the 11 embedding and retrieval models, and the 10 GANs.
VisionMambaModelPoolingStrategyis promoted out of a generic typeThis is the collision phase 1 deliberately deferred.
PoolingStrategywas nested insideTransformerEmbeddingNetwork<T>, and a type nested in a generic class is a distinct type per type argument —TransformerEmbeddingNetwork<float>.PoolingStrategyand the<double>one were unrelated types, and neither could be named from a non-generic options class. It was never usable as configuration, which is exactly what it describes.It becomes
AiDotNet.Enums.EmbeddingPoolingStrategy. Renamed rather than moved as-is, becausesrc/VisionLanguage/Encodershas its own unrelatedPoolingStrategywith different members.Options inheritance now mirrors model inheritance
Seven embedding models derive from
TransformerEmbeddingNetwork<T>and forward through: base(...). Their options classes had to derive fromTransformerEmbeddingOptionsrather than straight fromEmbeddingModelOptions— otherwise a derived model cannot pass its own options to base at all.Three assumptions the GANs broke
: this(...), not only: base(...), and such a constructor is public and carries movable parameters of its own.: base(CreateDCGANGeneratorArchitecture(latentSize, ..., generatorFeatureMaps), ...). Dropping the identifier is wrong — the expression needs the value.optionsis in scope in an initializer, so it becomes(options?.GeneratorChannels ?? 64), carrying the same default. And a name defaulted in one constructor but required in another (DCGAN'slatentSize) must be supplied there, not dropped.What the tests caught that the build did not
GanOptions.ValidateCorerequiredLatentSizeandInitialLearningRate. 30 GAN tests failed.LatentSizestays a required constructor argument on the four models that build architectures from it, so it is legitimately unset on those options objects; and most GANs never had anInitialLearningRateparameter at all. It now carries the DCGAN paper's 0.0002 as a default, and onlyImageChannelsis required.Worth noting against the ratchet itself: it counts a parameter as covered when the options class has a matching name, so a model whose constructor still takes the parameter can score as migrated.
UnifiedMultimodalNetworkdid exactly that in phase 3. The skipped behavioural assertion is what closes that hole.Verification
dotnet build src/AiDotNet.csproj -f net8.0— 0 errorsdotnet build tests/AiDotNet.Tests -f net8.0— 0 errorsRefs #2090
🤖 Generated with Claude Code