Skip to content

Add NVIDIA NeMo Sortformer speaker-diarization model - #410

Merged
justinchuby merged 14 commits into
mainfrom
themason2011/sortformer-diarization
Aug 4, 2026
Merged

Add NVIDIA NeMo Sortformer speaker-diarization model#410
justinchuby merged 14 commits into
mainfrom
themason2011/sortformer-diarization

Conversation

@themason2011

Copy link
Copy Markdown
Contributor

Summary

Adds support for the NVIDIA NeMo Sortformer streaming speaker-diarization model (nvidia/diar_streaming_sortformer_4spk-v2.1) to mobius.

This is not a HuggingFace transformers architecture — it ships as a NeMo .nemo checkpoint with no config.json. The architecture was reconstructed from the checkpoint and NeMo source. The exported ONNX graph implements the offline forward path (frontend_encoder + forward_infer): mel-spectrogram features in, per-frame speaker-activity probabilities out.

Architecture

  • FastConformer encoder (NeMo ConformerEncoder): rel_pos Transformer-XL attention, 8x dw_striding conv subsampling, Macaron feed-forward blocks, batch-norm convolution module, xscaling.
  • Encoder projection (512 -> 192) + post-LayerNorm Transformer encoder (18 layers).
  • Speaker sigmoid head (relu -> Linear -> relu -> Linear -> sigmoid).

Module attribute names mirror the NeMo weight prefixes, so preprocess_weights only drops unused keys (mel preprocessor, num_batches_tracked, the streaming FIFO head). The mel preprocessor (STFT) is left out of the graph, matching the mobius audio convention.

Changes

  • models/sortformer.pySortformerConfig (+ from_nemo_yaml), SortformerDiarizationModel, and a build_sortformer(.nemo) loader that extracts the archive, builds the graph, and applies checkpoint weights.
  • tasks/_diarization.pyDiarizationTask (input_features -> speaker_probs), registered as the diarization task.
  • Exports wired into models/__init__.py and the task registry.

Testing

  • Unit tests (tests/build_graph_test.py::TestBuildGraphSortformer): tiny-config graph build, I/O contract, initializers, task lookup, and an ORT forward run. All pass.
  • Integration parity (tests/sortformer_integration_test.py, -m integration): compares the ONNX output against the real NeMo PyTorch reference. Offline output matches to 1.2e-7 (float32).
  • ruff check + format clean.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Comment thread tests/sortformer_integration_test.py Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new speaker-diarization export path to mobius by implementing NVIDIA NeMo’s Sortformer streaming diarization model as an ONNX graph (offline forward path), along with a new diarization task and tests to validate graph construction and NeMo parity.

Changes:

  • Added SortformerConfig, SortformerDiarizationModel, and a .nemo loader (build_sortformer) to reconstruct the model and apply NeMo weights.
  • Introduced DiarizationTask (input_features → speaker_probs) and registered it under the diarization task name.
  • Added unit graph-build tests and an integration parity test against NeMo.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
tests/sortformer_integration_test.py Integration test comparing ONNX output to NeMo PyTorch offline inference.
tests/build_graph_test.py New L1-style graph build + ORT execution smoke tests for Sortformer + DiarizationTask.
src/mobius/tasks/_diarization.py New encoder-only diarization task wiring input_features to speaker_probs.
src/mobius/tasks/init.py Exports/registers DiarizationTask and the diarization task mapping.
src/mobius/models/sortformer.py New Sortformer model implementation + .nemo extraction/loading helper.
src/mobius/models/init.py Exports Sortformer* symbols and build_sortformer.

Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/tasks/_diarization.py
Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/models/__init__.py Outdated
Comment thread src/mobius/models/sortformer.py Outdated
Comment thread src/mobius/models/sortformer.py
Comment thread src/mobius/models/sortformer.py
Comment thread src/mobius/models/sortformer.py Outdated
themason2011 added a commit that referenced this pull request Jul 17, 2026
Applies @justinchuby's PR #410 review feedback:

- Use the opset-24 `op.Swish` op directly for Conformer feed-forward and
  convolution activations instead of the manual `x * sigmoid(x)` helper
  (removes the `_swish` function).
- Replace the manual scaled-dot-product attention in `_TransformerAttention`
  with the opset-24 `op.Attention` op (bidirectional, unmasked).
- Move the `.nemo` archive loader `build_sortformer` out of the modeling code
  into a dedicated NeMo integration package
  (`mobius.integrations.nemo`), mirroring the GGUF integration layout, and
  drop it from the public `mobius.models` namespace.

Verified: sortformer graph unit tests pass and offline output stays bit-close
to the NeMo reference (max abs diff 1.2e-7 vs golden).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Mason Corey <masoncorey@microsoft.com>
Comment thread src/mobius/integrations/nemo/_sortformer.py Outdated
Wire the NeMo Sortformer speaker-diarization model into mobius's central
build_from_nemo loader instead of a bespoke build_sortformer entry point.

- Register SortformerDiarizationModel under model_type 'sortformer' with the
  'diarization' task in _registry.py.
- Export SortformerConfig/SortformerDiarizationModel from models/__init__.py
  and DiarizationTask from tasks/__init__.py (TASK_REGISTRY 'diarization').
- Map the NeMo SortformerEncLabelModel target to model_type 'sortformer' and
  dispatch nemo_to_config to SortformerConfig.from_nemo_yaml before the
  FastConformer-RNNT encoder validation.
- Add model_type field to SortformerConfig so the generic pipeline can resolve
  the model class and task from config.model_type.
- Re-add graph-build unit tests and rewrite the integration test to use
  build_from_nemo.

Verified: build_from_nemo output matches the NeMo golden reference
(max abs diff 1.15e-7); 5 graph tests pass; ruff clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Mason Corey <masoncorey@microsoft.com>
@themason2011
themason2011 force-pushed the themason2011/sortformer-diarization branch from c235538 to ac3670a Compare July 20, 2026 21:30
@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

Performance Comparison

Comparing 3dc671c3d13b9f

Model Metric Baseline Current Delta
bert (feature-extraction) model_size_bytes 359 KB 359 KB +0.0%
bert (feature-extraction) num_nodes 60 60 +0.0%
falcon model_size_bytes 364 KB 364 KB +0.0%
falcon num_nodes 68 68 +0.0%
gemma2 model_size_bytes 428 KB 428 KB +0.0%
gemma2 num_nodes 107 107 +0.0%
gpt2 model_size_bytes 388 KB 388 KB +0.0%
gpt2 num_nodes 54 54 +0.0%
llama model_size_bytes 425 KB 425 KB +0.0%
llama num_nodes 62 62 +0.0%
llama (static-cache) model_size_bytes 425 KB 425 KB +0.0%
llama (static-cache) num_nodes 58 58 +0.0%
mamba (ssm-text-generation) model_size_bytes 296 KB 296 KB +0.0%
mamba (ssm-text-generation) num_nodes 98 98 +0.0%
phi3 model_size_bytes 421 KB 421 KB +0.0%
phi3 num_nodes 60 60 +0.0%
phi3 (static-cache) model_size_bytes 421 KB 421 KB +0.0%
phi3 (static-cache) num_nodes 56 56 +0.0%
qwen2 model_size_bytes 425 KB 425 KB +0.0%
qwen2 num_nodes 62 62 +0.0%
qwen2 (static-cache) model_size_bytes 425 KB 425 KB +0.0%
qwen2 (static-cache) num_nodes 58 58 +0.0%
qwen3_5_moe (hybrid-text-generation) model_size_bytes 506 KB 506 KB +0.0%
qwen3_5_moe (hybrid-text-generation) num_nodes 275 275 +0.0%
qwen3_5_text (hybrid-text-generation) model_size_bytes 458 KB 458 KB +0.0%
qwen3_5_text (hybrid-text-generation) num_nodes 129 129 +0.0%
qwen3_5_vl (hybrid-qwen-vl) model_size_bytes 977 KB 977 KB +0.0%
qwen3_5_vl (hybrid-qwen-vl) num_nodes 413 413 +0.0%
t5 (seq2seq) model_size_bytes 836 KB 836 KB +0.0%
t5 (seq2seq) num_nodes 166 166 +0.0%
whisper (speech-to-text) model_size_bytes 1008 KB 1008 KB +0.0%
whisper (speech-to-text) num_nodes 128 128 +0.0%

No performance regressions.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

🏗️ Architecture Diff

Comparing 3dc671c3d13b9f

Model Sub-model Changes Status
bert (feature-extraction) model 0
falcon model 0
gemma2 model 0
gemma4 (gemma4) decoder 0
gemma4 (gemma4) embedding 0
gemma4 (gemma4) vision_encoder 0
gemma4_text model 0
gpt2 model 0
llama model 0
llama (static-cache) model 0
mamba (ssm-text-generation) model 0
phi3 model 0
phi3 (static-cache) model 0
qwen model 0
qwen (static-cache) model 0
qwen2 model 0
qwen2 (static-cache) model 0
qwen2_moe model 0
qwen2_moe (static-cache) model 0
qwen3 model 0
qwen3 (static-cache) model 0
qwen3_5_moe (hybrid-text-generation) model 0
qwen3_5_text (hybrid-text-generation) model 0
qwen3_5_vl (hybrid-qwen-vl) decoder 0
qwen3_5_vl (hybrid-qwen-vl) embedding 0
qwen3_5_vl (hybrid-qwen-vl) vision_encoder 0
qwen3_moe model 0
qwen3_moe (static-cache) model 0
qwen3_next (hybrid-text-generation) model 0
t5 (seq2seq) decoder 0
t5 (seq2seq) encoder 0
whisper (speech-to-text) decoder 0
whisper (speech-to-text) encoder 0

No architecture changes detected.


Legend: ⚪ No change · 🔵 Minor (attrs/inits) · 🟡 Moderate (nodes added/removed) · 🔴 Major (interface changed)

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

The author of this PR, themason2011, is not an activated member of this organization on Codecov.
Please activate this user on Codecov to display this PR comment.
Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.
Please don't hesitate to email us at support@codecov.io with any questions.

@justinchuby justinchuby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you address the copilot comments and ensure the unit test CI + lint CI are green?

@justinchuby

Copy link
Copy Markdown
Member

If possible, could you also add the L4/L5 tests?

Copilot AI review requested due to automatic review settings July 30, 2026 20:55
@CLAassistant

CLAassistant commented Jul 30, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ themason2011
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (9)

src/mobius/models/sortformer.py:403

  • scale is a float32 Constant, but matrix_ac/matrix_bd follow the model compute dtype (can be f16/bf16 when build_from_nemo(..., dtype=...) is used). ONNX Mul requires matching dtypes, so this can produce an invalid graph for non-float32 builds. Cast the scalar to the scores dtype (the codebase typically uses op.CastLike).
        scale = op.Constant(value=ir.tensor(np.array(self._head_dim**-0.5, dtype=np.float32)))
        scores = op.Mul(op.Add(matrix_ac, matrix_bd), scale)

src/mobius/models/sortformer.py:486

  • _build_pos_emb() constructs positional embeddings in float32 (Cast(..., FLOAT) + float32 div_term), but those embeddings are fed into linear_pos which will use the model dtype (potentially f16/bf16 via build_from_nemo(dtype=...)). This dtype mismatch will break MatMul inside Linear. Cast the final pos_emb to the compute dtype before returning (keeping Sin/Cos in f32 if desired).
        t_scalar = op.Squeeze(op.Shape(x, start=1, end=2))  # scalar T
        one = op.Constant(value_int=1)
        start = op.Sub(t_scalar, one)  # T - 1
        limit = op.Neg(t_scalar)  # -T (exclusive) -> last value -(T-1)
        positions = op.Range(start, limit, op.Constant(value_int=-1))  # [2T-1]
        pos_f = op.Unsqueeze(op.Cast(positions, to=ir.DataType.FLOAT), [-1])  # [2T-1,1]
        div_term = op.Constant(value=ir.tensor(self._div_term))  # [d/2]
        angles = op.Mul(pos_f, div_term)  # [2T-1, d/2]
        sin = op.Unsqueeze(op.Sin(angles), [-1])
        cos = op.Unsqueeze(op.Cos(angles), [-1])
        interleaved = op.Concat(sin, cos, axis=-1)  # [2T-1, d/2, 2]
        pe = op.Reshape(interleaved, [0, -1])  # [2T-1, d]
        return op.Unsqueeze(pe, [0])  # [1, 2T-1, d]

src/mobius/tasks/_diarization.py:42

  • feat_in if feat_in else "feat" treats feat_in=0 as missing and falls back to a symbolic dimension. If feat_in is present, the check should be is not None to avoid surprising behavior and keep the shape contract consistent.
        feat_in = getattr(config, "feat_in", None)
        input_features = builder.input(
            "input_features",
            dtype=config.dtype,
            shape=["batch", feat_in if feat_in else "feat", "time"],
        )

src/mobius/models/sortformer.py:494

  • The xscaling multiplier is emitted as a float32 constant, but x is in the model compute dtype (potentially f16/bf16). This can make the graph invalid for non-float32 builds. Use CastLike on the scalar constant (same pattern used in other models).
        # Scale inputs to the attention layers by sqrt(d_model) (xscaling).
        x = op.Mul(x, op.Constant(value=ir.tensor(np.array(self._xscale, np.float32))))
        pos_emb = self._build_pos_emb(op, x)

src/mobius/integrations/nemo/_config_mapping.py:23

  • The module-level docstring still claims this mapping only targets ArchitectureConfig / FastConformer-RNNT, but the module now also maps Sortformer diarization to SortformerConfig. Updating the top docstring prevents misleading documentation for NeMo support.
from mobius._configs import ArchitectureConfig
from mobius._configs._base import BaseModelConfig

src/mobius/models/sortformer.py:16

  • The module docstring claims the subsampled time length is T = T_mel / 8, but the dw_striding Conv2d stack uses stride=2 with padding=1/kernel=3, which yields ceil(T_mel / 8) for odd lengths. This doc mismatch can confuse downstream users when shapes don’t divide evenly.
    input_features [B, feat, T_mel]
      -> transpose                       [B, T_mel, feat]
      -> FastConformer encoder           [B, T, fc_d_model]   (T = T_mel / 8)
      -> encoder_proj (Linear)           [B, T, tf_d_model]
      -> Transformer encoder (post-LN)   [B, T, tf_d_model]
      -> speaker sigmoid head            [B, T, num_spks]

src/mobius/tasks/_diarization.py:25

  • The output-frame formula in the docstring is inaccurate for the Sortformer subsampling stem (stride-2 convs with padding). The time dimension becomes ceil(time / subsampling_factor) rather than a simple division when time isn’t an exact multiple.
    Input:  ``input_features`` — ``[batch, feat, time]`` mel spectrogram.
    Output: ``speaker_probs`` — ``[batch, frames, num_spks]`` sigmoid
    probabilities (``frames = time / subsampling_factor``).
    """

src/mobius/integrations/nemo/_config_mapping.py:23

  • This file imports BaseModelConfig from the private module mobius._configs._base. Elsewhere (e.g. src/mobius/tasks/_diarization.py) the convention is to import config types from the public mobius._configs package. Using the public import keeps internal module boundaries consistent.

This issue also appears on line 22 of the same file.

from mobius._configs import ArchitectureConfig
from mobius._configs._base import BaseModelConfig

src/mobius/models/sortformer.py:87

  • The PR description mentions a build_sortformer(.nemo) loader implemented in models/sortformer.py, but there is no such function in this file (and build_sortformer is not found in the repo). Either add the helper (if intended API surface) or update the PR description to match the current build_from_nemo entry point.
# ---------------------------------------------------------------------------
# Configuration
# ---------------------------------------------------------------------------


@dataclasses.dataclass
class SortformerConfig(BaseModelConfig):
    """Configuration for the Sortformer diarization model.

    Fields mirror the NeMo ``.nemo`` ``model_config.yaml`` (``encoder``,
    ``transformer_encoder`` and ``sortformer_modules`` sections).
    """

    # Mel feature dimension (preprocessor ``features``).
    feat_in: int = 128
    # FastConformer encoder.
    fc_d_model: int = 512
    fc_num_layers: int = 17
    fc_num_heads: int = 8
    fc_ff_expansion: int = 4
    fc_conv_kernel: int = 9
    fc_subsampling_conv_channels: int = 256
    fc_subsampling_factor: int = 8
    # Transformer encoder (operates on projected embeddings).
    tf_d_model: int = 192
    tf_num_layers: int = 18
    tf_num_heads: int = 8
    tf_inner_size: int = 768
    tf_hidden_act: str = "relu"
    # Diarization head.
    num_spks: int = 4

    # HuggingFace/registry model_type — consumed by the generic
    # ``build_from_nemo`` pipeline to resolve the model class and task.
    model_type: str | None = "sortformer"

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Mason Corey <36940706+themason2011@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 30, 2026 21:17
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Mason Corey <36940706+themason2011@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/mobius/models/sortformer.py:489

  • _build_pos_emb() returns pe as FLOAT (Sin/Cos are computed in float32), but the attention path later feeds this into Linear/MatMul tensors that follow config.dtype (f16/bf16 possible via build_from_nemo(..., dtype=...)). Without casting pe to the compute dtype, ONNX will reject the graph due to mismatched element types. nemo_rnnt.py handles this by casting the positional encoding to the model dtype before projection (see src/mobius/models/nemo_rnnt.py:595-599).
        interleaved = op.Concat(sin, cos, axis=-1)  # [2T-1, d/2, 2]
        pe = op.Reshape(interleaved, [0, -1])  # [2T-1, d]
        return op.Unsqueeze(pe, [0])  # [1, 2T-1, d]

src/mobius/models/sortformer.py:434

  • op.Constant(value_float=0.5) creates a float32 scalar. If the model is built with config.dtype = f16/bf16, this will cause a dtype mismatch in Mul/Add (ONNX requires matching element types). The repo already uses a cast-to-ref pattern for scalars (e.g. _scalar_like() in src/mobius/models/nemo_rnnt.py:55-63). Consider casting the scalar once and reusing it for both Macaron half-step residuals.
        # Macaron feed-forward (half-step residual)
        ff = self.feed_forward1(op, self.norm_feed_forward1(op, x))
        x = op.Add(x, op.Mul(ff, op.Constant(value_float=0.5)))

src/mobius/models/sortformer.py:497

  • The xscaling constant is emitted as a float32 tensor (np.float32) and multiplied into x. If the graph is built with config.dtype = f16/bf16, this introduces a float32×float16 Mul, which ONNX will reject. Please cast the scalar to x's dtype (same pattern as _scalar_like() in src/mobius/models/nemo_rnnt.py:55-63).
        # Scale inputs to the attention layers by sqrt(d_model) (xscaling).
        x = op.Mul(x, op.Constant(value=ir.tensor(np.array(self._xscale, np.float32))))
        pos_emb = self._build_pos_emb(op, x)

src/mobius/integrations/nemo/_config_mapping.py:23

  • BaseModelConfig is part of the public mobius._configs API (re-exported from src/mobius/_configs/__init__.py:24-29), but this file imports it from the private module mobius._configs._base. Since ArchitectureConfig is already imported from mobius._configs, it would be more consistent to import BaseModelConfig from the same public module to avoid leaking internal paths into other packages.
from mobius._configs import ArchitectureConfig
from mobius._configs._base import BaseModelConfig

Comment thread scripts/generate_sortformer_golden.py Fixed
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Mason Corey <36940706+themason2011@users.noreply.github.com>
@themason2011

Copy link
Copy Markdown
Contributor Author

If possible, could you also add the L4/L5 tests?

Added L4/L5 tests

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Signed-off-by: Mason Corey <36940706+themason2011@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/mobius/models/sortformer.py:486

  • _build_pos_emb() currently references sin and cos without defining them, so the Sortformer graph cannot be built (NameError during graph construction). Implement the standard Transformer-XL sinusoidal relative positional embedding (arange(T-1, -T, -1) × div_term, then sin/cos interleave) before concatenation/reshape.
        t_scalar = op.Squeeze(op.Shape(x, start=1, end=2))  # scalar T
        one = op.Constant(value_int=1)
        start = op.Sub(t_scalar, one)  # T - 1
        limit = op.Neg(t_scalar)  # -T (exclusive) -> last value -(T-1)

        # Positions: [T-1, T-2, ..., -(T-1)] -> shape [2T-1]
        positions = op.Range(start, limit, op.Constant(value_int=-1))

src/mobius/integrations/nemo/_config_mapping.py:23

  • BaseModelConfig is imported from the private module mobius._configs._base, but it is already re-exported from mobius._configs (and that’s how other tasks import it). Prefer the public import to avoid coupling to private module structure.
from mobius._configs import ArchitectureConfig
from mobius._configs._base import BaseModelConfig

src/mobius/tasks/_diarization.py:34

  • DiarizationTask.build() leaves the module parameter untyped. This repo uses MyPy strict mode and other tasks type this as onnxscript.nn.Module; leaving it untyped is likely to fail type-checking and is inconsistent with nearby task implementations.
    def build(
        self,
        module,
        config: BaseModelConfig,
    ) -> ModelPackage:

Copilot AI review requested due to automatic review settings August 3, 2026 21:42
Co-authored-by: themason2011 <36940706+themason2011@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/mobius/models/sortformer.py:92

  • fc_subsampling_factor is parsed from NeMo YAML but the model implementation hardcodes 8x subsampling (3 stride-2 conv stages) and never uses this field. If a config ever has a different subsampling factor, the graph output length will be wrong while the config suggests otherwise. Add validation (or plumb the factor into _ConvSubsampling) so mismatches fail fast.
    def validate(self) -> None:
        if self.fc_d_model % self.fc_num_heads != 0:
            raise ValueError("fc_d_model must be divisible by fc_num_heads")
        if self.tf_d_model % self.tf_num_heads != 0:
            raise ValueError("tf_d_model must be divisible by tf_num_heads")

src/mobius/tasks/_diarization.py:25

  • The docstring claims frames = time / subsampling_factor, but the Sortformer export uses strided conv subsampling with padding, which yields ceil(time / subsampling_factor) in general (only exact division when time is a multiple of the factor). Update the docstring to avoid documenting an incorrect shape relationship.
    Input:  ``input_features`` — ``[batch, feat, time]`` mel spectrogram.
    Output: ``speaker_probs`` — ``[batch, frames, num_spks]`` sigmoid
    probabilities (``frames = time / subsampling_factor``).

tests/sortformer_integration_test.py:90

  • Parsing the NPZ meta field via json.loads(str(golden["meta"])) is brittle because str(...) depends on NumPy's formatting. Use .item() to extract the scalar string reliably before JSON parsing.
    meta = json.loads(str(golden["meta"]))

Copilot AI review requested due to automatic review settings August 3, 2026 21:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/mobius/integrations/nemo/_config_mapping.py:23

  • BaseModelConfig is imported from the private module path mobius._configs._base, while the rest of the codebase imports config types from the public mobius._configs package. Keeping the public import avoids leaking internal module structure and prevents accidental circular-import/regression if _base layout changes.
from mobius._configs import ArchitectureConfig
from mobius._configs._base import BaseModelConfig

src/mobius/integrations/nemo/_config_mapping.py:104

  • The new model_type == "sortformer" branch in nemo_to_config() is currently untested. Existing _config_mapping_test.py only exercises the RNNT path, so regressions in Sortformer field extraction (e.g., feat_in, layer counts) won’t be caught by unit tests.
def nemo_to_config(nemo_config: dict[str, Any]) -> BaseModelConfig:
    """Build a mobius config from a NeMo ``model_config.yaml`` dict.

    Dispatches on the NeMo ``target`` class path: FastConformer-RNNT models
    produce an :class:`ArchitectureConfig`; Sortformer diarization models
    produce a :class:`SortformerConfig`.
    """
    target = str(nemo_config.get("target", ""))
    model_type = nemo_model_type(target)

    if model_type == "sortformer":
        # Imported lazily to avoid a models→integrations import cycle.
        from mobius.models.sortformer import SortformerConfig

        return SortformerConfig.from_nemo_yaml(nemo_config)

src/mobius/tasks/_diarization.py:25

  • The task docstring says frames = time / subsampling_factor, but the Sortformer subsampling stem uses stride-2 convs with padding, which yields frames = ceil(time / subsampling_factor) in general. This is a user-facing contract comment, so it should match the actual shape behavior.
    Input:  ``input_features`` — ``[batch, feat, time]`` mel spectrogram.
    Output: ``speaker_probs`` — ``[batch, frames, num_spks]`` sigmoid
    probabilities (``frames = time / subsampling_factor``).

src/mobius/models/sortformer.py:17

  • Top-level model docstring says T = T_mel / 8, but the conv subsampling stem actually produces T = ceil(T_mel / 8) due to stride-2 + padding (and _ConvSubsampling already documents the ceil). Aligning this avoids confusion for callers that feed lengths not divisible by 8.
    input_features [B, feat, T_mel]
      -> transpose                       [B, T_mel, feat]
      -> FastConformer encoder           [B, T, fc_d_model]   (T = T_mel / 8)
      -> encoder_proj (Linear)           [B, T, tf_d_model]

Copilot AI review requested due to automatic review settings August 3, 2026 21:58
@themason2011 themason2011 reopened this Aug 3, 2026
@themason2011

Copy link
Copy Markdown
Contributor Author

Ran lintrunner with copilot

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/mobius/tasks/_diarization.py:34

  • DiarizationTask.build() leaves module untyped. Most tasks annotate this as module: nn.Module (e.g. src/mobius/tasks/_feature_extraction.py:32-36, src/mobius/tasks/_image_classification.py:30-35), which helps MyPy strict mode and keeps task signatures consistent.
    def build(
        self,
        module,
        config: BaseModelConfig,
    ) -> ModelPackage:

src/mobius/integrations/nemo/_config_mapping.py:104

  • The new model_type == "sortformer" branch in nemo_to_config() isn't covered by a unit test (current src/mobius/integrations/nemo/_config_mapping_test.py only exercises RNNT). Adding a minimal Sortformer config fixture + assertions would prevent silent mapping regressions (e.g., wrong key names / defaults).
    if model_type == "sortformer":
        # Imported lazily to avoid a models→integrations import cycle.
        from mobius.models.sortformer import SortformerConfig

        return SortformerConfig.from_nemo_yaml(nemo_config)

src/mobius/models/sortformer.py:679

  • SortformerDiarizationModel.preprocess_weights() is new, but Sortformer isn't currently included in the tests/weight_alignment_test.py parametrization (it only uses ALL_CAUSAL_LM_CONFIGS, ENCODER_CONFIGS, VISION_CONFIGS, etc.; no speech/diarization configs and no sortformer entry in tests/_test_configs.py). This means the standard Tier-1 weight-alignment coverage won't catch accidental key drops/renames here.
    def preprocess_weights(
        self, state_dict: dict[str, torch.Tensor]
    ) -> dict[str, torch.Tensor]:
        """Map NeMo checkpoint names onto module parameter names.

        NeMo prefixes align 1:1 with this module tree (``encoder.*``,
        ``transformer_encoder.*``, ``sortformer_modules.*``).  Only the mel
        preprocessor, batch-norm ``num_batches_tracked`` counters and the
        unused streaming ``hidden_to_spks`` head are dropped.
        """
        renamed: dict[str, torch.Tensor] = {}
        for key, value in state_dict.items():
            if key.startswith("preprocessor."):
                continue
            if key.endswith("num_batches_tracked"):
                continue
            # Unused in the offline forward path (streaming FIFO head).
            if key.startswith("sortformer_modules.hidden_to_spks."):
                continue
            renamed[key] = value

The model_coverage_test enforces that every registered model_type has either a
shared test config (tests/_test_configs.py + _registry.py test_model_id) or an
entry in _COVERAGE_SKIP. Sortformer is a NeMo .nemo diarization model with a
custom SortformerConfig and no HuggingFace config.json, so it uses the same
mechanism as the sibling fastconformer_rnnt model: skip the generic coverage
and rely on the dedicated tests/sortformer_integration_test.py (L4/L5 golden)
plus TestBuildGraphSortformer (L1).

Fixes 4 CI failures in tests/model_coverage_test.py for model_type 'sortformer'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Mason Corey <masoncorey@microsoft.com>
Copilot AI review requested due to automatic review settings August 3, 2026 22:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mobius/integrations/nemo/_config_mapping.py:23

  • Importing BaseModelConfig from the private module mobius._configs._base is inconsistent with the rest of the codebase (tasks and other call sites import it from the public mobius._configs). Using the public import avoids coupling to internal module layout.
from mobius._configs import ArchitectureConfig
from mobius._configs._base import BaseModelConfig

src/mobius/tasks/_diarization.py:25

  • The docstring states frames = time / subsampling_factor, but Sortformer’s dw_striding subsampling uses stride-2, pad-1 convs, which yield ceil(time / subsampling_factor) for odd-length inputs. This is important for downstream consumers that rely on the exact frame count behavior.
    Input:  ``input_features`` — ``[batch, feat, time]`` mel spectrogram.
    Output: ``speaker_probs`` — ``[batch, frames, num_spks]`` sigmoid
    probabilities (``frames = time / subsampling_factor``).

@justinchuby
justinchuby merged commit be3de0b into main Aug 4, 2026
23 of 24 checks passed
@justinchuby
justinchuby deleted the themason2011/sortformer-diarization branch August 4, 2026 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants