Skip to content

34 models build their options object twice, giving base and derived different instances #2228

Description

@ooples

The bug

34 files under src/ construct their options object twice:

public DeepARModel(DeepAROptions<T>? options = null)
    : base(options ?? new DeepAROptions<T>())   // line 93 — instance A
{
    _options = options ?? new DeepAROptions<T>();  // line 95 — instance B

When the caller passes null, the ?? evaluates on both lines, so the base class and the derived class end up holding two different options objects. Whatever the base reads off its copy is not what _options holds, and a mutation through one is invisible to the other.

Why this is not theoretical

MatryoshkaEmbedding had the same shape and it was load-bearing. TransformerEmbeddingNetwork's constructor sets _embeddingDimension = _options.EmbeddingDimension, so with no options supplied the base built a TransformerEmbeddingOptions (768) while the derived class built a MatryoshkaEmbeddingOptions (1536). The base's copy is the one that sizes every layer and bounds EmbedResized, so a model documented and tested as 1536 wide was built 768 wide, and three MatryoshkaEmbeddingTests were failing on exactly that. Fixed in e578b030b9 on feature/options-surface-phase-7-audio; all 32 of those tests now pass.

Severity across the other 33 depends on whether the base reads anything off its copy, so many are currently latent — but they are one base-class change away from behaving like Matryoshka did, and none of them is intentional.

The symptom only appears on the options-null path. Any test that passes an explicit options object sees nothing wrong, which is why this survived.

The fix

Materialize once and hand the same instance to the base:

    : base(options ??= new DeepAROptions<T>())
{
    _options = options;

How to find them

for f in $(git grep -l -E ': base\(options \?\? new' -- 'src/**/*.cs'); do
  grep -qE '_options = options \?\? new' "$f" && echo "$f"
done

The 34

src/Clustering/ dominates the list — AutoK/{GMeans,XMeans}, Density/{DBSCAN,HDBSCAN,MeanShift,OPTICS}, Hierarchical/{AgglomerativeClustering,BIRCH,BisectingKMeans}, Neural/SelfOrganizingMap, Partitioning/{AffinityPropagation,FuzzyCMeans,KMeans,KMedoids,MiniBatchKMeans}, Probabilistic/GaussianMixtureModel, SemiSupervised/{COPKMeans,SeededKMeans}, Spectral/SpectralClustering, Streaming/MiniBatchKMeans — plus src/TimeSeries/DeepARModel.cs and others. Run the command above for the current full list.

Found while adversarially reviewing #2130; all 34 are pre-existing on master and none was introduced by that PR.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions