Skip to content

Add AudioDataFilterStage composite pipeline for end-to-end audio curation - #1640

Merged
sarahyurick merged 52 commits into
NVIDIA-NeMo:mainfrom
shubhamNvidia:pr/advance-pipeline
Apr 6, 2026
Merged

Add AudioDataFilterStage composite pipeline for end-to-end audio curation#1640
sarahyurick merged 52 commits into
NVIDIA-NeMo:mainfrom
shubhamNvidia:pr/advance-pipeline

Conversation

@shubhamNvidia

Copy link
Copy Markdown
Contributor

Summary

  • Add AudioDataFilterStage, a CompositeStage that decomposes into a configurable sequence of independent audio processing stages for extracting clean single-speaker segments from raw audio files.
  • Add AudioDataFilterConfig dataclass with unified configuration for all pipeline stages (VAD, band filter, UTMOS, SIGMOS, speaker separation, timestamp mapping).
  • The composite stage leverages the executor's cross-file parallelism -- each sub-stage has its own resource allocation (CPU for band/concat, GPU for VAD/UTMOS/SIGMOS/speaker-sep).

Pipeline stages (when all enabled)

  1. MonoConversion (CPU, 1:1) -- normalize to mono at target sample rate
  2. VAD (GPU, batch mode) -- segment audio into speech chunks
  3. BandFilter (CPU, 1:1) -- filter by bandwidth (full_band/narrow_band)
  4. UTMOS (GPU, 1:1) -- filter by MOS quality score
  5. SIGMOS (GPU, 1:1) -- filter by multi-dimensional quality metrics
  6. SegmentConcatenation (CPU, 1:1) -- merge filtered segments with silence gaps
  7. SpeakerSeparation (GPU, 1:N fan-out) -- diarize and split by speaker
    8-11. Per-speaker filters -- VAD + Band + UTMOS + SIGMOS on each speaker's audio
  8. TimestampMapper (CPU, 1:1) -- resolve final positions back to original file

Configuration

All stages are individually toggleable via AudioDataFilterConfig:

  • enable_vad, enable_band_filter, enable_utmos, enable_sigmos, enable_speaker_separation
  • Quality thresholds: utmos_mos_threshold, sigmos_noise_threshold, sigmos_ovrl_threshold, etc.
  • General: sample_rate, band_value, vad_min_duration_sec, vad_max_duration_sec
  • Supports to_dict() / from_dict() for serialization

Usage

from nemo_curator.stages.audio.advance_pipelines import AudioDataFilterStage, AudioDataFilterConfig

config = AudioDataFilterConfig(
enable_utmos=True,
enable_sigmos=True,
enable_speaker_separation=True,
utmos_mos_threshold=3.5,
)
pipeline.add_stage(AudioDataFilterStage(config=config))

@copy-pr-bot

copy-pr-bot Bot commented Mar 22, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@greptile-apps

greptile-apps Bot commented Mar 22, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR completes a large refactoring of AudioDataFilterStage, moving from the earlier @dataclass-based AudioDataFilterConfig approach to a clean YAML-config + explicit __init__ design. The revised code addresses the majority of concerns raised in previous reviews (renamed to advanced_pipelines/audio_data_filter, added _validate(), fixed SegmentConcatenation guard, correct TimestampMapper condition).

The one new finding is a minor mismatch in get_enabled_stages(): it reports \"concatenation\" as an enabled stage whenever speaker separation is on, even when VAD is off — while decompose() correctly skips SegmentConcatenationStage in that case — causing the informational log message to be inaccurate.

Confidence Score: 4/5

Safe to merge with minor attention needed; two previously-flagged P1 concerns (validation bypass for dict overrides, SIGMOS partial-file corruption) remain open from prior review threads.

Score is 4 rather than 5 because prior review threads identified two genuine P1-level issues (no _validate call after dict-override _deep_merge in audio_data_filter.py:83-84, and the SIGMOS partial-file guard in sigmos.py:175-187) that have not been addressed in this revision. The new finding is P2. If those two prior P1 items are resolved, this is a 5.

nemo_curator/stages/audio/advanced_pipelines/audio_data_filter/audio_data_filter.py (validation bypass after dict merge); nemo_curator/stages/audio/filtering/sigmos.py (partial model file detection)

Important Files Changed

Filename Overview
nemo_curator/stages/audio/advanced_pipelines/audio_data_filter/audio_data_filter.py Core CompositeStage; refactored to explicit init with config_path/config/name params; sub-stage names still hardcoded and no _validate after dict-override merge
nemo_curator/stages/audio/advanced_pipelines/audio_data_filter/config.py YAML config loader with _validate() and _deep_merge(); get_enabled_stages() incorrectly includes 'concatenation' when speaker-sep enabled but VAD disabled
nemo_curator/stages/audio/filtering/sigmos.py SIGMOS ONNX filter; partial-write guard only checks zero-size files, leaving non-empty corrupt partial downloads undetected
nemo_curator/stages/audio/advanced_pipelines/audio_data_filter/default_config.yaml Clean defaults for all pipeline stages with sensible quality thresholds and cpu/gpu allocations
tests/stages/audio/advanced_pipelines/test_audio_data_filter.py Good coverage: config loading, _validate, deep merge, decompose counts, speaker-without-VAD edge case, and pickling
nemo_curator/stages/audio/segmentation/vad_segmentation.py Silero VAD with nested/fan-out modes, resampling fallback, and clean device handling
nemo_curator/stages/audio/segmentation/speaker_separation.py SpeakerSeparationStage with setup/teardown, OOM handling, and correct _metadata propagation
nemo_curator/stages/audio/postprocessing/timestamp_mapper.py Correctly handles both segment-mapping (from SegmentConcatenation) and no-mapping paths; strips waveform from output
nemo_curator/stages/audio/preprocessing/concatenation.py Concatenates nested VAD segments into single waveform, writes SegmentMapping metadata to task._metadata
nemo_curator/stages/audio/init.py Package init updated to re-export AudioDataFilterStage alongside all existing stage classes

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[MonoConversionStage\nCPU 1:1] --> B{enable_vad?}
    B -- yes --> C[VADSegmentationStage\nGPU nested=True]
    B -- no --> D{enable_band?}
    C --> D
    D -- yes --> E[BandFilterStage\nCPU 1:1]
    D -- no --> F{enable_utmos?}
    E --> F
    F -- yes --> G[UTMOSFilterStage\nGPU 1:1]
    F -- no --> H{enable_sigmos?}
    G --> H
    H -- yes --> I[SIGMOSFilterStage\nGPU 1:1]
    H -- no --> J{enable_speaker?}
    I --> J
    J -- no --> K{enable_vad?}
    J -- yes --> L{enable_vad?}
    L -- yes --> M[SegmentConcatenationStage\nCPU M:1]
    L -- no --> N[SpeakerSeparationStage\nGPU 1:N]
    M --> N
    N --> O[VAD_Speaker\nnested=False]
    O --> P[BandFilter_Speaker]
    P --> Q[UTMOS_Speaker]
    Q --> R[SIGMOS_Speaker]
    R --> S[TimestampMapperStage\nCPU 1:1]
    K -- yes --> S
    K -- no --> T[End - no TimestampMapper]
Loading

Reviews (28): Last reviewed commit: "Merge upstream/main into pr/advance-pipe..." | Re-trigger Greptile

Comment thread nemo_curator/stages/audio/advance_pipelines/Audio_data_filter/config.py Outdated
Comment on lines +22 to +25
)

DEFAULT_OUTPUT_FORMAT: str = "wav"

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.

P2 SUPPORTED_AUDIO_FORMATS and DEFAULT_OUTPUT_FORMAT are unused

Both constants are defined at module level but are never referenced anywhere in this PR (nor exported from __all__). If they are intended as public API constants for callers to validate user input, they should be used in validation logic (e.g. in AudioDataFilterConfig.__post_init__) or at minimum exported via __all__. Otherwise, remove them to reduce dead code.

Comment thread nemo_curator/stages/audio/advance_pipelines/Audio_data_filter/__init__.py Outdated
Comment thread nemo_curator/stages/audio/advanced_pipelines/__init__.py
…ng, speaker separation, and timestamp mapping
Comment on lines +86 to +88
gpu_res = self.gpu_resources
cpu_res = Resources(cpus=1.0)
band_res = Resources(cpus=4.0)

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.

P2 cpu_res variable is dead code

cpu_res = Resources(cpus=1.0) is assigned on line 87 but is never passed to any stage. MonoConversionStage, SegmentConcatenationStage, and TimestampMapperStage are all appended without calling .with_(resources=cpu_res). Either pass it to those stages so they get proper CPU resource accounting, or remove the variable to avoid confusion.

Suggested change
gpu_res = self.gpu_resources
cpu_res = Resources(cpus=1.0)
band_res = Resources(cpus=4.0)
gpu_res = self.gpu_resources
band_res = Resources(cpus=4.0)

"""

from dataclasses import dataclass, field
from typing import Any, List

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.

P2 Unused Any import

Any is imported from typing but never referenced anywhere in this file. Only List is used.

Suggested change
from typing import Any, List
from typing import List

Comment on lines +83 to +91
def get_enabled_filters(self) -> List[str]:
filters = []
if self.enable_band_filter:
filters.append("band")
if self.enable_utmos:
filters.append("utmos")
if self.enable_sigmos:
filters.append("sigmos")
return filters

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.

P2 get_enabled_filters() silently omits VAD

The method is used directly in the log message in decompose() to summarise the active filters, but enable_vad is never included. When VAD is enabled (the default), the log will report something like filters: ['band', 'utmos', 'sigmos'] with no mention of VAD, making diagnostics misleading. Consider adding vad to the list:

Suggested change
def get_enabled_filters(self) -> List[str]:
filters = []
if self.enable_band_filter:
filters.append("band")
if self.enable_utmos:
filters.append("utmos")
if self.enable_sigmos:
filters.append("sigmos")
return filters
def get_enabled_filters(self) -> List[str]:
filters = []
if self.enable_vad:
filters.append("vad")
if self.enable_band_filter:
filters.append("band")
if self.enable_utmos:
filters.append("utmos")
if self.enable_sigmos:
filters.append("sigmos")
return filters

Comment on lines +76 to +81
def to_dict(self) -> Dict[str, Any]:
return asdict(self)

@classmethod
def from_dict(cls, config_dict: Dict[str, Any]) -> "AudioDataFilterConfig":
return cls(**{k: v for k, v in config_dict.items() if k in cls.__dataclass_fields__})

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.

P2 from_dict silently drops unknown keys with no warning

The from_dict implementation silently discards any key not present in __dataclass_fields__:

return cls(**{k: v for k, v in config_dict.items() if k in cls.__dataclass_fields__})

If a caller round-trips a config saved with a newer version of AudioDataFilterConfig (which may have renamed or removed fields), they'll get no error and instead silently receive default values for the missing fields. Consider logging a warning or raising a ValueError for unrecognised keys to make serialisation mismatches visible:

@classmethod
def from_dict(cls, config_dict: Dict[str, Any]) -> "AudioDataFilterConfig":
    known = cls.__dataclass_fields__
    unknown = set(config_dict) - set(known)
    if unknown:
        logger.warning(f"AudioDataFilterConfig.from_dict: ignoring unknown keys: {unknown}")
    return cls(**{k: v for k, v in config_dict.items() if k in known})

stage = AudioDataFilterStage(config=config)

# Process audio
results = stage.process(audio_batch)

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.

P1 stage.process() always raises RuntimeError on CompositeStage

The docstring example calls stage.process(audio_batch), but AudioDataFilterStage is a CompositeStage and the base class implementation explicitly raises a RuntimeError for direct calls:

def process(self, task: X) -> Y | list[Y]:
    """Composite stages should never be executed directly."""
    raise RuntimeError(
        f"Composite stage '{self.name}' should not be executed directly. "
        "It should be decomposed into execution stages during planning."
    )

Any user who copies this example verbatim will get a RuntimeError at runtime. The example should be corrected to show the intended usage via pipeline.add_stage(stage) (mirroring the correct pattern shown in the Audio_data_filter/__init__.py docstring).

Suggested change
results = stage.process(audio_batch)
pipeline.add_stage(stage)

@@ -0,0 +1,56 @@
# Copyright (c) 2026, NVIDIA CORPORATION. All rights reserved.

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.

P2 Module name advance_pipelines appears to be a typo

The directory (and therefore the public import path) is advance_pipelines, but the conventional English adjective form would be advanced_pipelines. Since this becomes a public API path (nemo_curator.stages.audio.advance_pipelines), fixing the typo now (before this is shipped) would avoid a breaking rename later. All four new files and any future additions to this package would need to move to nemo_curator/stages/audio/advanced_pipelines/.


@classmethod
def from_dict(cls, config_dict: Dict[str, Any]) -> "AudioDataFilterConfig":
return cls(**{k: v for k, v in config_dict.items() if k in cls.__dataclass_fields__})

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.

P2 cls.__dataclass_fields__ uses a semi-private attribute

__dataclass_fields__ is a dataclass implementation detail (prefixed with double underscores). The public API for introspecting dataclass fields is dataclasses.fields(). Prefer the public form to avoid relying on internal CPython behaviour:

Suggested change
return cls(**{k: v for k, v in config_dict.items() if k in cls.__dataclass_fields__})
known_keys = {f.name for f in dataclasses.fields(cls)}
return cls(**{k: v for k, v in config_dict.items() if k in known_keys})

This also requires adding import dataclasses (or from dataclasses import fields) at the top of the file alongside the existing from dataclasses import dataclass, asdict.

pipeline.add_stage(AudioDataFilterStage(config=config))
"""

from dataclasses import dataclass, field

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.

P2 dataclass imported but used only as decorator on the class itself

from dataclasses import dataclass, fieldfield is used for config on line 75, which is correct. However, using @dataclass on a CompositeStage subclass means the generated __init__ accepts config and name as constructor parameters. This makes AudioDataFilterStage constructible as AudioDataFilterStage(name="custom"), which overrides the stage name at instantiation time. This is valid Python, but since other ProcessingStage subclasses in the project (see base.py) typically use plain class attributes for name rather than dataclass fields, this pattern may be inconsistent. It also means __eq__ and __hash__ = None are auto-generated, making stage instances unhashable (e.g., cannot be stored in a set or used as a dict key). If no other composite stages in the project use @dataclass, consider whether this is intentional or if a plain class with explicit __init__ would be more idiomatic.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +79 to +80
def __post_init__(self):
super().__init__()

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.

P2 super().__init__() called without stage-init arguments

CompositeStage.__init__ (in base.py) only initialises self._with_operations = [] and takes no arguments, so this call is correct. However, since ProcessingStage defines name = "ProcessingStage" as a class-level attribute (not in __init__), and @dataclass generates instance-level self.name via the generated __init__, there is a subtle layering: the instance attribute set by the dataclass __init__ shadows the class attribute from ProcessingStage. This works correctly in CPython today, but is fragile: if ProcessingStage or CompositeStage ever gains an __init__ that assigns self.name, it would silently overwrite the dataclass-set value after __post_init__ calls super().__init__(). A comment here explaining why super().__init__() is called with no args (just to initialise _with_operations) would make this intent explicit and safe for future maintainers.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

vad_max_duration_sec: float = 60.0

# Concatenation
silence_duration_ms: int = 500

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.

P2 silence_duration_ms typed as int but consumed as seconds

The field is declared silence_duration_ms: int = 500 (implying integer milliseconds), but is used on line 121 as cfg.silence_duration_ms / 1000.0 to produce a float seconds value. While the conversion is arithmetically correct, there is a usability trap: a caller who mistakenly passes silence_duration_ms=0.5 (thinking the unit is seconds) gets 0.0005 s of silence — effectively none — with no warning. Since Python does not enforce type hints at runtime, this failure is silent. Consider adding a docstring clarification or a __post_init__ guard:

if self.silence_duration_ms < 0:
    raise ValueError(f"silence_duration_ms must be non-negative, got {self.silence_duration_ms}")

Or rename to silence_duration_sec: float = 0.5 to match the parameter name of SegmentConcatenationStage directly, eliminating the unit conversion entirely.

@sarahyurick

Copy link
Copy Markdown
Contributor

/ok to test 0022d28

@shubhamNvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 86c2569

@copy-pr-bot

copy-pr-bot Bot commented Apr 2, 2026

Copy link
Copy Markdown

/ok to test 86c2569

@shubhamNvidia, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@sarahyurick sarahyurick 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.

Are there any tests we can add for this PR?

self.name = name
self._cfg = load_config(config_path)
if config:
from .config import _deep_merge

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.

This can be a top-level import.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Currently, there is no test for this PR. Will add tests once the dependent PRs (filtering, segmentation, preprocessing) are merged; currently can't test due to cross-PR import dependencies.

- Merge upstream/main, resolve __init__.py conflicts keeping SIGMOS + UTMOS + SpeakerSep
- Add MIT (Microsoft) + NVIDIA license headers to third-party sigmos files
- Switch third-party sigmos.py from logging to loguru for consistency
- Remove sys.path hack in sigmos_pipeline.py, use direct relative import
- Refactor SIGMOSFilterStage: load model once in setup(), call self._model.run() directly
- Update tests to mock _initialize_model and use _make_mock_model helper
- Fix Ruff lint (__all__ sorting)

Signed-off-by: shbhawsar <shbhawsar@nvidia.com>
- Remove ONNX model files from repo (third_party/sigmos/ and model/)
- Add auto-download from Microsoft's SIG-Challenge GitHub repository
- Model cached at ~/.cache/nemo_curator/sigmos_model/ by default
- Users can override with model_path= or model_dir=
- Add setup_on_node() for multi-node pre-download
- Add file validation (missing/empty check with clear error message)
- Uses requests.get() matching NSFW filter pattern

Signed-off-by: shbhawsar <shbhawsar@nvidia.com>
stages,
vad,
band,
utmos,

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.

When speaker separation is enabled (the default), the full filter chain (VAD → BandFilter → UTMOS → SIGMOS) runs TWICE:

  1. First pass: filters on the raw audio segments
  2. After SpeakerSeparation: same filters again on per-speaker segments

UTMOS and SIGMOS are GPU-inference stages. Running them twice doubles the GPU compute cost. This is especially wasteful because audio quality metrics (MOS scores, noise, bandwidth) don't fundamentally change between pre-speaker and post-speaker segments — the waveform content is the same, just attributed to different speakers.

With defaults, AudioDataFilterStage() decomposes into 12 stages including redundant GPU inference.

Consider making the second filter pass optional via a config flag (post_speaker_refilter: true/false), or document why double filtering is architecturally necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The double filter pass is intentional — after speaker separation, the per-speaker waveforms are different from the original mixed audio. VAD on isolated single-speaker audio produces different segment boundaries, and quality scores (UTMOS, SIGMOS) on separated speech are more representative of actual per-speaker signal quality. This was validated through extensive analysis on our test sets.

enable: true
mos_threshold: 3.5

sigmos:

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.

Every stage has enable: true by default, including speaker separation, SIGMOS, and UTMOS. A user writing AudioDataFilterStage() with no args gets the full 12-stage pipeline including GPU inference stages that require:

  • Silero VAD model (torch.hub download)
  • SIGMOS ONNX model + onnxruntime + librosa + scipy
  • UTMOS model
  • SpeakerSeparation NeMo model

First-time usage will likely crash if users don't have all these dependencies and models available.

Consider either:
(a) Enabling only basic stages by default (mono conversion, VAD) and requiring explicit opt-in for GPU-heavy stages, or
(b) Documenting the full dependency set prominently in the class docstring.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The default config with all stages enabled represents our best-validated pipeline derived from extensive analysis, optimized for extracting high-quality audio data. Users who want our recommended accuracy and results can use it as-is. Any stage can be individually disabled via enable: false in the YAML config to match specific requirements or reduce dependency overhead. Will add prominent documentation listing the full dependency set so users know what's needed upfront.


mc = cfg.get("mono_conversion", {})
stages.append(
MonoConversionStage(

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.

MonoConversionStage is always added unconditionally — there's no enable flag for it, unlike every other stage (VAD, Band, UTMOS, SIGMOS, SpeakerSep all have enable toggles in the YAML config).

If audio is already mono at the correct sample rate, this is wasted processing. For consistency, add enable: true to the mono_conversion section in default_config.yaml and guard the append in decompose().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

MonoConversion is intentionally unconditional because it also serves as the audio loader — it reads the file, populates waveform, sample_rate, duration, and num_samples into the task data. Disabling it would force each downstream stage to independently load from audio_filepath, causing redundant file I/O and risking failures on multi-channel files. The strict_sample_rate check acts as a guard against corrupted or mismatched files in large-scale datasets that would otherwise produce silent errors downstream. The operation is lightweight (CPU-only, no GPU), so keeping it always-on prevents failures at negligible overhead.

return merged


def _validate(cfg: dict[str, Any]) -> None:

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.

_validate() only checks two constraints:

  1. vad.min_duration_sec < vad.max_duration_sec
  2. concatenation.silence_duration_sec >= 0

Missing validations for threshold parameters:

  • UTMOS mos_threshold should be in [1, 5] (MOS scale)
  • SIGMOS thresholds (noise_threshold, ovrl_threshold, etc.) should be in [1, 5]
  • VAD threshold should be in [0, 1]
  • speaker_separation.min_duration should be positive
  • mono_conversion.output_sample_rate should be a positive integer

A user passing utmos: { mos_threshold: 50 } gets no validation error but filters out 100% of audio silently.

Comment thread nemo_curator/stages/audio/__init__.py Outdated
and advanced audio processing pipelines.
"""

from nemo_curator.stages.audio.advance_pipelines import AudioDataFilterStage

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.

Adding AudioDataFilterStage to the top-level audio/__init__.py means import nemo_curator.stages.audio triggers the entire import chain:

  • AudioDataFilterStageSIGMOSFilterStage → third-party sigmos.pyimport librosa, import scipy, import onnxruntime
  • AudioDataFilterStageVADSegmentationStageimport torch, import torchaudio, from silero_vad import ...
  • AudioDataFilterStageUTMOSFilterStageimport torch

Users who only need basic audio stages (e.g., GetAudioDurationStage, PreserveByValueStage) pay the cost of loading all these heavy ML libraries.

Fix with lazy-importing AudioDataFilterStage.

@mohammadaaftabv

mohammadaaftabv commented Apr 5, 2026

Copy link
Copy Markdown
Contributor

Adding comments for adv specific changes already on main below:

1.SpeakerSeparationStage imports pydub at module level in both speaker_separation.py and speaker_sep.py, but pydub is not declared in the audio_common, audio_cpu, or audio_cuda12 extras in pyproject.toml.

Users installing with pip install nemo_curator[audio_cpu] will get error
when importing SpeakerSeparationStage.

Fix: add "pydub>=0.25.1" to the audio_common extra (same category as the missing librosa/scipy for SIGMOS).

  1. Use ffspec instead of os everywhere. Multiple locations use os.path.exists() or os.path.isfile() to check audio file paths:
  • common.pyresolve_waveform_from_item() (line 102, 115)
  • mono_conversion.pyprocess() (line 731)
  • utmos.py_load_waveform_tensor() uses os.path.isfile(path)

These silently return False for cloud paths (s3://, gs://, etc.), causing tasks to be dropped without error. The codebase already has fsspec available. Consider:

from fsspec.core import url_to_fs
fs, path = url_to_fs(audio_filepath)
if fs.exists(path):
  1. main — nemo_curator/stages/audio/segmentation/speaker_separation_module/speaker_sep.py lines 332–342 (diarize_audio) and lines 478–487 (get_speaker_audio_data)
    When called with a waveform tensor (not a file path), speaker_sep.py creates temp files twice for the same input:

  2. diarize_audio() writes waveform → temp .wav, passes to SortformerEncLabelModel.diarize(audio=temp_path)

  3. get_speaker_audio_data() writes the same waveform → another temp .wav, reads it as AudioSegment.from_file(temp_path)

The second write could be avoided by constructing AudioSegment directly from the numpy array:

from pydub import AudioSegment
import numpy as np

samples = (waveform.squeeze().cpu().numpy() * 32767).astype(np.int16)
original_audio = AudioSegment(
    data=samples.tobytes(),
    sample_width=2,
    frame_rate=sample_rate,
    channels=1,
)
  1. In speaker_sep.py get_speaker_audio_data():
silent_audio = AudioSegment.silent(duration=duration_ms)
for start_time, end_time in segments:
    segment_audio = original_audio[start_ms:end_ms]
    silent_audio = silent_audio.overlay(segment_audio, position=start_ms)
    

For a 1-hour audio file, this creates a full-hour silent AudioSegment per speaker, then overlays small segments onto it. Memory usage scales with (num_speakers × file_duration) even if each speaker only has a few seconds of speech.

A more efficient approach would be to concatenate only the speaker's segments directly (like SegmentConcatenationStage)

  1. utmos.py defines a standalone _load_waveform_tensor() function that duplicates most of the logic in resolve_waveform_from_item() from common.py. Both:
  • Check for waveform + sample_rate in the item dict
  • Fall back to loading from audio_filepath
  • Handle missing sample rate
  • Convert to correct tensor format

UTMOSFilterStage should use the shared utility from common.py instead. BandFilterStage (#1576) already uses resolve_waveform_from_item correctly.

  1. band_filter_module/predict.py predict_audio() returns error strings on failure:
return f"Error: Could not load model: {e}"
return f"Error during prediction: {e}"

The caller in band.py checks with isinstance(pred, str) and not pred.startswith("Error"), which is fragile — if the error message format changes, the check silently breaks.

Other stages (UTMOS, SIGMOS) use the standard pattern: raise an exception or return None. predict_audio() should do the same for consistency.

  1. torchcodec is listed only under audio_cuda12 but it has CPU wheels available on PyPI. If any stage uses torchcodec for audio decoding on CPU, it won't be available.

If torchcodec is intentionally CUDA-only, that should be documented. Otherwise, consider adding it to audio_cpu as well, or at least to audio_common.

  1. MonoConversionStage with strict_sample_rate=True (default) silently drops any audio not at output_sample_rate (default 48kHz). With strict_sample_rate=False, it accepts as-is but doesn't resample.

There's no option to actually resample to the target rate, which limits the stage's usefulness for heterogeneous datasets. Consider adding a resample=True option using torchaudio.transforms.Resample, similar to what UTMOSFilterStage does internally with its _resamplers cache.

  1. SegmentConcatenationStage._concatenate() logs a warning when segments have different sample rates but concatenates them anyway:
if parts and sr != sample_rate:
    logger.warning(f"Sample rate mismatch: expected {sample_rate}Hz, got {sr}Hz. Output audio may be corrupted.")

Concatenating waveforms with different sample rates produces corrupted audio (pitch-shifted, time-stretched). This should either:

Resample the segment to match the first segment's rate, or Skip the mismatched segment with a warning or explain why this pipeline allows for corrupted audio output in the first place.

  1. speaker_sep.py _get_param() supports 4 different config access patterns: direct attribute, dict access, nested dict ["speaker_separation"][param], and .get() method. But SpeakerSeparationStage always passes a plain dict.

This adds ~30 lines of complexity for unused flexibility. Consider simplifying to plain dict access:

def _get_param(self, param_name: str, default_value):
    return self.config.get(param_name, default_value)
  1. speaker_sep.py suppresses: C901 (complexity), PLR0912 (too many branches), PLR0913 (too many arguments), BLE001 (broad except), S110 (pass in except).

The timeline-based overlap processing in clean_cut_overlapping_segments() and exclude_overlapping_segments() could be extracted into a separate TimelineProcessor class with smaller, focused methods, reducing complexity and eliminating the need for noqa suppressions.

  1. Tests only cover the stage wrapper (process()) with mocked separators. There are no unit tests for the core timeline algorithms in SpeakerSeparator:
  • clean_cut_overlapping_segments()
  • exclude_overlapping_segments()
  • merge_adjacent_segments()
  • filter_short_segments()

These are complex timeline algorithms with edge cases (adjacent segments, fully-overlapping segments, buffer time exceeding segment duration). PR #1575 (preprocessing) demonstrates excellent test coverage — the same standard should apply here.

shubhamNvidia and others added 5 commits April 5, 2026 14:05
- Guard torch.cuda.empty_cache() with torch.cuda.is_available() in
  SIGMOSFilterStage.teardown() to prevent crash on CPU-only machines
- Add librosa and scipy to audio_common extras in pyproject.toml
  (required by third-party sigmos.py module-level imports)
- Remove unused sigmos_pipeline.py (dead code, never imported by stage)
- Regenerate uv.lock
Signed-off-by: Sarah Yurick <53962159+sarahyurick@users.noreply.github.com>
…tests

- Rename advance_pipelines -> advanced_pipelines, Audio_data_filter ->
  audio_data_filter (PEP 8 naming)
- Add config validations for MOS [0,5], VAD threshold [0,1], sample rate,
  min_duration, silence_duration
- Add per-stage cpus/gpus resource config in YAML and decompose()
- Guard torch.cuda.empty_cache() in teardown() for utmos, speaker_sep, vad
- Remove redundant _initialize_predictor() call in BandFilterStage
- Remove dead code: SUPPORTED_AUDIO_FORMATS, DEFAULT_OUTPUT_FORMAT,
  vad.mode, output.format
- Fix division-by-zero in mono_conversion and common.py
- Fix sample rate mismatch in concatenation (skip instead of corrupt)
- Fix predict.py returning zero features on failure (return None)
- Fix UTMOS setup() to fail loud if model doesn't load
- Add try/except guard for SortformerEncLabelModel import in speaker_sep.py
- Standardize SIGMOS file loading to use soundfile (not librosa)
- Add pydub to audio_common deps in pyproject.toml
- Add warning logs in speaker_separation and timestamp_mapper
- Add nested=(suffix=="") comment, get_enabled_stages completeness
- Add 30 unit tests for AudioDataFilterStage config and decompose
- Add 21 unit tests for SpeakerSeparator timeline algorithms
- Regenerate uv.lock
@shubhamNvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 47372e3

Comment on lines +191 to +215
f"and place it at {weights_path}"
)
raise RuntimeError(msg)

return weights_path

def setup_on_node(
self, _node_info: NodeInfo | None = None, _worker_metadata: WorkerMetadata | None = None
) -> None:
try:
if self.model_path is None:
self._download_model(self.model_dir)
logger.info("SIGMOS model pre-downloaded on node")
except Exception: # noqa: BLE001
logger.warning("SIGMOS model pre-download in setup_on_node failed; will retry in setup().")

def setup(self, _: WorkerMetadata | None = None) -> None:
from nemo_curator.utils.gpu_utils import ensure_cudnn_loaded

ensure_cudnn_loaded()
self._initialize_model()

def teardown(self) -> None:
self._model = None
if torch.cuda.is_available():

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.

P1 Partial ONNX file silently used after failed write

The download guard checks os.path.exists(weights_path). If a prior run fetched the model successfully (requests.get() returned 200) but failed partway through f.write(response.content) (e.g. disk-full), a non-empty but corrupt partial file is left on disk. On the next call os.path.exists is True, so the download is skipped entirely. The only subsequent safety net (os.path.getsize == 0) only catches an empty file; a non-empty partial file passes and is handed to ONNX Runtime, which may raise a cryptic InvalidProtobuf or OrtException.

Write to a temp path and do an atomic rename on success:

import tempfile

tmp_path = weights_path + ".tmp"
try:
    logger.info(f"Downloading SIGMOS model from {_SIGMOS_MODEL_URL}")
    with requests.get(_SIGMOS_MODEL_URL, timeout=120, stream=True) as response:
        response.raise_for_status()
        with open(tmp_path, "wb") as f:
            for chunk in response.iter_content(chunk_size=65536):
                f.write(chunk)
    os.replace(tmp_path, weights_path)
    logger.info(f"SIGMOS model saved to {weights_path}")
except Exception:
    if os.path.exists(tmp_path):
        os.remove(tmp_path)
    raise

@shubhamNvidia

Copy link
Copy Markdown
Contributor Author

/ok to test abeca1e

@sarahyurick sarahyurick 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.

Mostly LGTM, adding some comments about tests.



class TestValidate:
def test_validate_valid_defaults(self) -> None:

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.

I don't think this test is needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

assert vad_stages[0].nested is True
assert vad_stages[1].nested is False

def test_decompose_stage_names_have_suffix(self) -> None:

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.

I don't think we need this one either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done


class TestPickling:
def test_audio_data_filter_stage_pickling(self) -> None:
import pickle

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.

This can be a top-level import.

for stage_name in stages_with_gpus:
assert "gpus" in cfg[stage_name], f"{stage_name} missing 'gpus' in config"

def test_default_yaml_loads_without_error(self) -> None:

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.

Same here, I think this can be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@shubhamNvidia

Copy link
Copy Markdown
Contributor Author

/ok to test f1696f2

@shubhamNvidia

Copy link
Copy Markdown
Contributor Author

/ok to test 1ea1455

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants