Add LibriSpeechMix SOT benchmark#1412
Open
vmendelev wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds LibriSpeechMix SOT (speaker-of-the-talker), a multispeaker ASR benchmark with evaluation framework. Includes dataset preparation, cpWER metrics/utilities, evaluator implementation, inference wrappers for RTMT-ASR and Multitalker Parakeet models, unified server backends, and comprehensive tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
1 similar comment
Contributor
📝 WalkthroughWalkthroughAdds LibriSpeechMix SOT (speaker-of-the-talker), a multispeaker ASR benchmark with evaluation framework. Includes dataset preparation, cpWER metrics/utilities, evaluator implementation, inference wrappers for RTMT-ASR and Multitalker Parakeet models, unified server backends, and comprehensive tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
nemo_skills/evaluation/evaluator/__init__.py (1)
56-56: Consider adding this benchmark to Slurm CI coverage.Given the new evaluator + cpWER path, cluster test coverage would reduce regression risk (especially across all split/mix variants).
Based on learnings: "When enabling new modality or adding complicated evaluation/metrics logic in benchmarks, consider adding the dataset into slurm tests for comprehensive evaluation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemo_skills/evaluation/evaluator/__init__.py` at line 56, Add the new librispeechmix_sot benchmark to Slurm CI so the LibrispeechMixSOTEvaluator and its cpWER evaluation path are exercised: update the CI test matrix/job configuration to include the "librispeechmix_sot" benchmark identifier and ensure the job runs the same split/mix variants used by LibrispeechMixSOTEvaluator (and invokes the cpWER path), so cluster tests cover this evaluator and prevent regressions.tests/test_datasets.py (1)
61-61: ⚡ Quick winPlease add/update benchmark docs for the new dataset entry.
Now that
librispeechmix-sotis registered, add corresponding docs with run/eval example commands and expected model results to keep benchmark usage discoverable.Based on learnings: "When adding new benchmarks, add it to the corresponding place in the documentation with example commands for running evaluation and expected results for tested models".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_datasets.py` at line 61, Add documentation for the newly registered dataset "librispeechmix-sot": update the benchmarks/docs page that lists datasets and include a short section for librispeechmix-sot containing (1) example CLI commands to run training/evaluation (match the project's existing command style used for other datasets), (2) example eval flags and dataset splits (e.g., "test"), and (3) expected model results (report metrics and the tested model names used to produce them) so users can reproduce and compare; ensure the section follows the same format/structure as other dataset entries in the benchmarks docs.nemo_skills/dataset/librispeechmix-sot/__init__.py (1)
7-20: Consider adding Slurm coverage for the new benchmark matrix.This group now expands to 12 benchmark slices, so a representative Slurm smoke for at least one under-20s and one over-20s slice would help keep the new eval/metrics path exercised.
Based on learnings: this benchmark adds nontrivial evaluation/metrics logic, so adding it to Slurm smoke coverage is worth considering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemo_skills/dataset/librispeechmix-sot/__init__.py` around lines 7 - 20, The benchmark matrix defined by _DURATION_SPLITS, _LSM_SPLITS, _MIXES and expanded into BENCHMARKS now creates 12 slices and we should add Slurm smoke coverage for at least one under-20s and one over-20s slice; update your Slurm smoke config (or the smoke list used by CI) to include two representative keys such as "librispeechmix-sot.under20s-test-clean-1mix" and "librispeechmix-sot.over20s-test-clean-1mix" (or equivalent dev-clean variants) so the new evaluation/metrics path exercised by SCORE_MODULE ("nemo_skills.dataset.librispeechmix-sot.librispeechmix_sot_score") runs in CI.tests/test_librispeechmix_sot.py (1)
50-330: Consider covering one LibriSpeechMix SOT split in the slurm benchmark suite.The local coverage here is strong, but this benchmark also depends on cluster-only paths like dataset staging and backend/evaluator wiring. Adding one small LibriSpeechMix SOT slurm job would help keep the full benchmark path exercised.
Based on learnings, "When enabling new modality or adding complicated evaluation/metrics logic in benchmarks, consider adding the dataset into slurm tests for comprehensive evaluation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_librispeechmix_sot.py` around lines 50 - 330, Add a lightweight LibriSpeechMix SOT slurm test entry so the full benchmark path (staging, backend/evaluator wiring, metrics) is exercised: add "librispeechmix-sot" (or a specific split like "librispeechmix-sot.under20s-test-clean-1mix") to the slurm benchmark test matrix (the variable/list that defines slurm jobs, e.g., SLURM_BENCHMARKS or similar), configure it to use a tiny sample (max_samples=1 or a small manifest) and minimal resources, and enable any dataset staging/backends flags required for evaluation (so RTMTASR/MultitalkerParakeet backends and LibriSpeech staging are invoked). Ensure the new job references the same metric/evaluator pipeline exercised in tests (LibriSpeechMixSOTMetrics / LibriSpeechMixSOTEvaluator) so cluster-only code paths run in CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/evaluation/speech-audio.md`:
- Around line 411-458: The docs/evaluation/speech-audio.md section
"LibriSpeechMix SOT" is missing an expected-results snippet for the validated
baselines; add a short "Expected results" subsection under that section showing
corpus-level no-PnC cpWER numbers for the tested baseline(s) (e.g., results for
the recommended generation_module(s) such as
nemo_skills.inference.librispeechmix_rtmtasr and
nemo_skills.inference.librispeechmix_multitalker_parakeet), include the exact
eval command(s) users can run (matching the examples already present like the ns
eval command) and list the cpWER value(s) and any notes (e.g., whether MeetEval
was used or --normalizer chime8 applied) so readers can sanity-check fresh runs;
place the snippet near the prep/run examples in the LibriSpeechMix SOT section
and label it "Expected results" for clarity.
In `@nemo_skills/dataset/librispeechmix-sot/librispeechmix_sot_score.py`:
- Around line 18-27: The loop that aggregates counts (iterating over
benchmarks.values(), using eval_mode and metrics keys) must fail fast instead of
defaulting missing fields to zero: replace metrics =
benchmark_metrics.get(eval_mode) with direct indexing (metrics =
benchmark_metrics[eval_mode]) and replace all metrics.get("cpwer_*", 0) accesses
with direct indexing (e.g., metrics["cpwer_errors"],
metrics["cpwer_substitutions"], metrics["cpwer_insertions"],
metrics["cpwer_deletions"], metrics["cpwer_ref_words"], metrics["num_entries"])
so a KeyError surfaces on malformed input; if you want a clearer message wrap in
a try/except and re-raise a ValueError that includes the benchmark identifier
and eval_mode for debugging.
In `@nemo_skills/dataset/librispeechmix-sot/prepare.py`:
- Around line 123-125: The mixed_duration function (and the other places that
zip parallel row arrays) silently truncate malformed rows; validate that the
parallel lists have equal lengths before zipping (e.g., assert
len(row["delays"]) == len(row["durations"]) and raise a descriptive ValueError
including an identifier from row like row.get("id") or the row itself) or switch
to zip(..., strict=True) to force an error on mismatch; apply the same
length-check or strict zip change to the other zip usages referenced (the blocks
around lines 166-175 and 223-231) so malformed JSONL entries fail fast with a
clear message.
In `@nemo_skills/evaluation/evaluator/librispeechmix_sot.py`:
- Around line 34-40: In _get_hypothesis, don't silently return "" when
user-supplied prediction_keys are wrong: if self.config.prediction_keys is set,
iterate those keys and if none are found raise a ValueError (or custom
exception) identifying the missing keys and the data_point (e.g., include an id
or the keys tried); if self.config.prediction_keys is not set, keep the existing
fallback behavior using the default keys ["pred_text","generation"] and return
"" only as a last resort. Update the logic in method _get_hypothesis of
librispeechmix_sot.py to validate and fail fast when configured prediction_keys
are invalid.
In `@nemo_skills/evaluation/metrics/librispeechmix_sot_metrics.py`:
- Around line 31-44: The update() logic currently prefers "generation" over
"pred_text", which is inconsistent with
LibriSpeechMixSOTEvaluator._get_hypothesis(); change the predicted_answers list
comprehension in update() to prefer pred_text first (use p.get("pred_text") or
p.get("generation") or None) and also when accumulating cpwer counts for the
representative item, derive the chosen hypothesis consistently (e.g., chosen =
first.get("pred_text") or first.get("generation")) so the pass@k/no-answer
bookkeeping matches the hypothesis used for cpWER aggregation.
In `@nemo_skills/evaluation/metrics/librispeechmix_sot_utils.py`:
- Around line 108-140: The current exhaustive permutation search over
itertools.permutations(range(size)) can blow up for large size when hallucinated
hypothesis speakers appear; modify the logic around ref_items/hyp_items/size to
guard against this by (a) defining a MAX_SPEAKER_ASSIGNMENT constant (e.g., 6)
and if size > MAX_SPEAKER_ASSIGNMENT avoid full permutation, and (b) implement a
fallback assignment algorithm: build a cost matrix using _counts_for_text
(errors or substitutions) for each ref/hyp pair and run a linear assignment
(Hungarian) solver like scipy.optimize.linear_sum_assignment or a greedy
nearest-match heuristic to produce assignment, totals and cpwer; keep the same
returned structure
(best/candidate/assignment/reference_streams/hypothesis_streams) so callers of
the function are unchanged.
In `@nemo_skills/inference/librispeechmix_multitalker_parakeet.py`:
- Around line 143-145: The current loop silently collapses entries with missing
session identifiers into a synthetic "sample" bucket; change the logic in the
entries processing loop to require one of the keys "session_id", "recording_id",
or "audio_id" to be present on each entry, retrieving it via direct key access
(not .get) and raising a clear exception (e.g., ValueError or KeyError) when
none are found; set session_id = entry[found_key] and then append to
by_session[session_id] so missing/renamed session ids fail fast instead of
merging into a single "sample" bucket.
In `@nemo_skills/inference/librispeechmix_rtmtasr.py`:
- Around line 79-80: The code currently hardcodes langid="en" while accepting
source_lang/target_lang, so either derive langid from the incoming config (e.g.,
use source_lang or target_lang when present and map to the expected token like
"en"/"fr") or validate and raise an explicit error if a non-"en" language is
passed; update the RTMT-ASR invocation where "langid=en" and
"gt_lang_attr_name=target_lang" are composed to replace the hardcoded literal
with the resolved/validated langid and ensure callers cannot pass unused
source_lang/target_lang values (throw an exception or return a clear validation
error when unsupported languages are supplied).
- Around line 105-108: Currently the code silently normalizes missing prediction
fields into empty strings by using row.get(...) which masks schema problems;
change it to require a supported prediction field and fail loudly: check row for
the presence of "pred_text" or "generation", raise a ValueError (or KeyError)
with a clear message if neither exists, and then set row["pred_text"] =
row["pred_text"] if present else row["generation"] and ensure row["generation"]
is set from the present field — replace uses of
row.get("pred_text")/row.get("generation") with direct indexing and an explicit
presence check to surface incompatible/malformed rows.
In `@recipes/multimodal/server/backends/multitalker_parakeet_backend.py`:
- Around line 98-102: The current deserialization logic in the
MultitalkerParakeetBackend class (the method that uses cls.__dataclass_fields__
to build cls(...)) silently moves unknown keys into extra_config; instead,
validate the input dict by computing unknown_keys = set(d.keys()) - known -
{"extra_config"} and if unknown_keys is non-empty raise a TypeError (or
ValueError) listing the offending keys so typos are rejected, then construct the
dataclass using only known keys (and handle extra_config only if explicitly
provided). Update the method (the classmethod that returns cls(...)) to perform
this check before instantiating the class.
In `@recipes/multimodal/server/backends/salm_backend.py`:
- Around line 166-175: The current loop builds a single prompts list for all
valid requests and calls self._model.generate once, ignoring
self.salm_config.batch_size; change this to respect batch_size by chunking the
prompts/temp_paths/valid_indices into slices of size
self.salm_config.batch_size, for each chunk compute a chunk_max_new_tokens
(start from self.config.max_new_tokens and take the max with any
request.max_new_tokens in that slice), call
self._model.generate(prompts=chunk_prompts, max_new_tokens=chunk_max_new_tokens)
for each chunk, and concatenate the returned output_ids in order; ensure you
update references to prompts, temp_paths and valid_indices used in the original
loop (e.g., the loop that builds prompts and the single generate() call) so
oversized batches are processed in bounded-size model calls rather than one big
call.
- Around line 121-122: The warmup exception handling in load_model()/SALM
backend currently swallows all exceptions (except Exception) which can hide
fatal incompatibilities; change the except block in the warmup routine so that
only documented/benign exceptions are caught (e.g., the specific warmup-related
exception type your backend raises) and logged, and re-raise any other
exceptions so load_model() fails fast; alternatively, log the error and
immediately raise the caught exception after logging (preserving stack) instead
of silently returning success. Ensure you reference the warmup call inside
load_model()/the SALM backend’s warmup block and adjust the except Exception as
written to either a specific exception class or to re-raise after
logger.warning.
---
Nitpick comments:
In `@nemo_skills/dataset/librispeechmix-sot/__init__.py`:
- Around line 7-20: The benchmark matrix defined by _DURATION_SPLITS,
_LSM_SPLITS, _MIXES and expanded into BENCHMARKS now creates 12 slices and we
should add Slurm smoke coverage for at least one under-20s and one over-20s
slice; update your Slurm smoke config (or the smoke list used by CI) to include
two representative keys such as "librispeechmix-sot.under20s-test-clean-1mix"
and "librispeechmix-sot.over20s-test-clean-1mix" (or equivalent dev-clean
variants) so the new evaluation/metrics path exercised by SCORE_MODULE
("nemo_skills.dataset.librispeechmix-sot.librispeechmix_sot_score") runs in CI.
In `@nemo_skills/evaluation/evaluator/__init__.py`:
- Line 56: Add the new librispeechmix_sot benchmark to Slurm CI so the
LibrispeechMixSOTEvaluator and its cpWER evaluation path are exercised: update
the CI test matrix/job configuration to include the "librispeechmix_sot"
benchmark identifier and ensure the job runs the same split/mix variants used by
LibrispeechMixSOTEvaluator (and invokes the cpWER path), so cluster tests cover
this evaluator and prevent regressions.
In `@tests/test_datasets.py`:
- Line 61: Add documentation for the newly registered dataset
"librispeechmix-sot": update the benchmarks/docs page that lists datasets and
include a short section for librispeechmix-sot containing (1) example CLI
commands to run training/evaluation (match the project's existing command style
used for other datasets), (2) example eval flags and dataset splits (e.g.,
"test"), and (3) expected model results (report metrics and the tested model
names used to produce them) so users can reproduce and compare; ensure the
section follows the same format/structure as other dataset entries in the
benchmarks docs.
In `@tests/test_librispeechmix_sot.py`:
- Around line 50-330: Add a lightweight LibriSpeechMix SOT slurm test entry so
the full benchmark path (staging, backend/evaluator wiring, metrics) is
exercised: add "librispeechmix-sot" (or a specific split like
"librispeechmix-sot.under20s-test-clean-1mix") to the slurm benchmark test
matrix (the variable/list that defines slurm jobs, e.g., SLURM_BENCHMARKS or
similar), configure it to use a tiny sample (max_samples=1 or a small manifest)
and minimal resources, and enable any dataset staging/backends flags required
for evaluation (so RTMTASR/MultitalkerParakeet backends and LibriSpeech staging
are invoked). Ensure the new job references the same metric/evaluator pipeline
exercised in tests (LibriSpeechMixSOTMetrics / LibriSpeechMixSOTEvaluator) so
cluster-only code paths run in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4dab57b5-55a4-4f9b-aec1-98dc44c0922d
📒 Files selected for processing (28)
docs/evaluation/speech-audio.mdnemo_skills/dataset/librispeechmix-sot/__init__.pynemo_skills/dataset/librispeechmix-sot/librispeechmix_sot_score.pynemo_skills/dataset/librispeechmix-sot/over20s-dev-clean-1mix/__init__.pynemo_skills/dataset/librispeechmix-sot/over20s-dev-clean-2mix/__init__.pynemo_skills/dataset/librispeechmix-sot/over20s-dev-clean-3mix/__init__.pynemo_skills/dataset/librispeechmix-sot/over20s-test-clean-1mix/__init__.pynemo_skills/dataset/librispeechmix-sot/over20s-test-clean-2mix/__init__.pynemo_skills/dataset/librispeechmix-sot/over20s-test-clean-3mix/__init__.pynemo_skills/dataset/librispeechmix-sot/prepare.pynemo_skills/dataset/librispeechmix-sot/under20s-dev-clean-1mix/__init__.pynemo_skills/dataset/librispeechmix-sot/under20s-dev-clean-2mix/__init__.pynemo_skills/dataset/librispeechmix-sot/under20s-dev-clean-3mix/__init__.pynemo_skills/dataset/librispeechmix-sot/under20s-test-clean-1mix/__init__.pynemo_skills/dataset/librispeechmix-sot/under20s-test-clean-2mix/__init__.pynemo_skills/dataset/librispeechmix-sot/under20s-test-clean-3mix/__init__.pynemo_skills/evaluation/evaluator/__init__.pynemo_skills/evaluation/evaluator/librispeechmix_sot.pynemo_skills/evaluation/metrics/librispeechmix_sot_metrics.pynemo_skills/evaluation/metrics/librispeechmix_sot_utils.pynemo_skills/evaluation/metrics/map_metrics.pynemo_skills/inference/librispeechmix_multitalker_parakeet.pynemo_skills/inference/librispeechmix_rtmtasr.pyrecipes/multimodal/server/backends/__init__.pyrecipes/multimodal/server/backends/multitalker_parakeet_backend.pyrecipes/multimodal/server/backends/salm_backend.pytests/test_datasets.pytests/test_librispeechmix_sot.py
Comment on lines
+411
to
+458
| ## LibriSpeechMix SOT | ||
|
|
||
| LibriSpeechMix SOT evaluates multispeaker ASR with serialized speaker tags. It uses the official [LibriSpeechMix](https://github.com/NaoyukiKanda/LibriSpeechMix) JSONL lists and LibriSpeech/OpenSLR audio, then writes NeMo SOT manifests with `[s0]`, `[s1]`, etc. reference text, RTTM supervision, and reference SegLST files. | ||
|
|
||
| Supported benchmark variants: | ||
|
|
||
| - `librispeechmix-sot.under20s-test-clean-1mix` | ||
| - `librispeechmix-sot.under20s-test-clean-2mix` | ||
| - `librispeechmix-sot.under20s-test-clean-3mix` | ||
| - `librispeechmix-sot.over20s-test-clean-1mix` | ||
| - `librispeechmix-sot.over20s-test-clean-2mix` | ||
| - `librispeechmix-sot.over20s-test-clean-3mix` | ||
|
|
||
| Dev smoke variants are also available with `dev-clean` in place of `test-clean`. The duration split is explicit: `under20s` contains records with mixed duration `<= 20.0` seconds, and `over20s` contains records with duration `> 20.0` seconds. | ||
|
|
||
| Prepare the required test-clean variants: | ||
|
|
||
| ```bash | ||
| ns prepare_data librispeechmix-sot --cluster=local --data_dir=/path/to/ns-data --splits test-clean --mixes 1mix 2mix 3mix | ||
| ``` | ||
|
|
||
| For a tiny local smoke using a pre-cloned LibriSpeechMix checkout: | ||
|
|
||
| ```bash | ||
| python -m nemo_skills.dataset.prepare librispeechmix-sot \ | ||
| --data_dir=/path/to/lsm-sot-data \ | ||
| --librispeechmix-dir=/path/to/LibriSpeechMix \ | ||
| --splits test-clean \ | ||
| --mixes 1mix \ | ||
| --max-samples 2 \ | ||
| --no-audio | ||
| ``` | ||
|
|
||
| The primary metric is corpus-level no-PnC cpWER. The scorer reads reference SOT text from `text`, reads hypotheses from `pred_text` when available and otherwise `generation`, splits both by `[sN]` tags, removes punctuation/case, and applies minimum speaker permutation matching. It also writes `*_ref.seglst.json` and `*_hyp.seglst.json`; when `meeteval-wer` is installed it additionally runs MeetEval cpWER with `--normalizer chime8 --partial`. | ||
|
|
||
| True speaker-attributed baselines should use a SOT-capable generation module, for example: | ||
|
|
||
| ```bash | ||
| ns eval \ | ||
| --benchmarks=librispeechmix-sot.under20s-test-clean-1mix \ | ||
| --generation_module=nemo_skills.inference.librispeechmix_rtmtasr \ | ||
| --server_type=none \ | ||
| --server_gpus=0 \ | ||
| --extra_arguments="++model_path=/path/to/MSCanary-v2.nemo ++spk_supervision=rttm ++nemo_root=/path/to/NeMo" | ||
| ``` | ||
|
|
||
| `nemo_skills.inference.librispeechmix_multitalker_parakeet` wraps the streaming Multitalker Parakeet path. The generic SALM backend can run `nvidia/canary-qwen-2.5b`, but the public model card documents generic ASR output, not SOT speaker-tagged output, so treat it as a generic ASR baseline unless a run proves it emits `[sN]` speaker-attributed text. | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add an expected-results snippet for the tested baseline(s).
This section has good prep/run coverage, but it never shows the cpWER numbers users should expect from the benchmark runs that were already validated in this PR. That makes it hard to sanity-check a fresh setup.
As per coding guidelines: "When adding new benchmarks, add it to the corresponding place in the documentation with example commands for running evaluation and expected results for tested models"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/evaluation/speech-audio.md` around lines 411 - 458, The
docs/evaluation/speech-audio.md section "LibriSpeechMix SOT" is missing an
expected-results snippet for the validated baselines; add a short "Expected
results" subsection under that section showing corpus-level no-PnC cpWER numbers
for the tested baseline(s) (e.g., results for the recommended
generation_module(s) such as nemo_skills.inference.librispeechmix_rtmtasr and
nemo_skills.inference.librispeechmix_multitalker_parakeet), include the exact
eval command(s) users can run (matching the examples already present like the ns
eval command) and list the cpWER value(s) and any notes (e.g., whether MeetEval
was used or --normalizer chime8 applied) so readers can sanity-check fresh runs;
place the snippet near the prep/run examples in the LibriSpeechMix SOT section
and label it "Expected results" for clarity.
Comment on lines
+18
to
+27
| for benchmark_metrics in benchmarks.values(): | ||
| metrics = benchmark_metrics.get(eval_mode) | ||
| if not metrics: | ||
| continue | ||
| total_entries += metrics.get("num_entries", 0) | ||
| errors += metrics.get("cpwer_errors", 0) | ||
| substitutions += metrics.get("cpwer_substitutions", 0) | ||
| insertions += metrics.get("cpwer_insertions", 0) | ||
| deletions += metrics.get("cpwer_deletions", 0) | ||
| ref_words += metrics.get("cpwer_ref_words", 0) |
Contributor
There was a problem hiding this comment.
Fail fast on missing cpWER aggregate fields.
These raw counts are produced explicitly by LibriSpeechMixSOTMetrics.get_metrics(). Treating a missing eval_mode or count key as zero here will silently corrupt the rolled-up score instead of surfacing a bad input shape.
As per coding guidelines: "Don't use .get() for accessing dictionary keys if the code expects them to be present; use direct access data[key_name] to fail with a clear error instead of silently corrupting data"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemo_skills/dataset/librispeechmix-sot/librispeechmix_sot_score.py` around
lines 18 - 27, The loop that aggregates counts (iterating over
benchmarks.values(), using eval_mode and metrics keys) must fail fast instead of
defaulting missing fields to zero: replace metrics =
benchmark_metrics.get(eval_mode) with direct indexing (metrics =
benchmark_metrics[eval_mode]) and replace all metrics.get("cpwer_*", 0) accesses
with direct indexing (e.g., metrics["cpwer_errors"],
metrics["cpwer_substitutions"], metrics["cpwer_insertions"],
metrics["cpwer_deletions"], metrics["cpwer_ref_words"], metrics["num_entries"])
so a KeyError surfaces on malformed input; if you want a clearer message wrap in
a try/except and re-raise a ValueError that includes the benchmark identifier
and eval_mode for debugging.
Comment on lines
+123
to
+125
| def mixed_duration(row: dict) -> float: | ||
| """Return mixed utterance duration from source delays and durations.""" | ||
| return float(max(delay + duration for delay, duration in zip(row["delays"], row["durations"]))) |
Contributor
There was a problem hiding this comment.
Validate row field lengths before zipping speaker-aligned lists.
zip() will silently truncate malformed LibriSpeechMix rows, so a bad JSONL entry can corrupt duration bucketing, RTTM/SegLST sidecars, and the mixed WAV without any error. Please validate the parallel arrays once or use strict=True on each zip(...).
Also applies to: 166-175, 223-231
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 125-125: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemo_skills/dataset/librispeechmix-sot/prepare.py` around lines 123 - 125,
The mixed_duration function (and the other places that zip parallel row arrays)
silently truncate malformed rows; validate that the parallel lists have equal
lengths before zipping (e.g., assert len(row["delays"]) == len(row["durations"])
and raise a descriptive ValueError including an identifier from row like
row.get("id") or the row itself) or switch to zip(..., strict=True) to force an
error on mismatch; apply the same length-check or strict zip change to the other
zip usages referenced (the blocks around lines 166-175 and 223-231) so malformed
JSONL entries fail fast with a clear message.
Comment on lines
+34
to
+40
| def _get_hypothesis(self, data_point: dict[str, Any]) -> str: | ||
| keys = self.config.prediction_keys or ["pred_text", "generation"] | ||
| for key in keys: | ||
| value = data_point.get(key) | ||
| if value is not None: | ||
| return str(value) | ||
| return "" |
Contributor
There was a problem hiding this comment.
Invalid prediction_keys should fail, not score as empty text.
With the current fallback, a typo in prediction_keys quietly returns "", and cpwer() turns every sample into a full-deletion error. Raise once none of the configured keys are present in the record.
As per coding guidelines: "Avoid cases where user-passed parameters are unused; code should fail if user specifies an unsupported argument or if a required argument is missing. Use dataclass or **kwargs syntax to handle this automatically"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemo_skills/evaluation/evaluator/librispeechmix_sot.py` around lines 34 - 40,
In _get_hypothesis, don't silently return "" when user-supplied prediction_keys
are wrong: if self.config.prediction_keys is set, iterate those keys and if none
are found raise a ValueError (or custom exception) identifying the missing keys
and the data_point (e.g., include an id or the keys tried); if
self.config.prediction_keys is not set, keep the existing fallback behavior
using the default keys ["pred_text","generation"] and return "" only as a last
resort. Update the logic in method _get_hypothesis of librispeechmix_sot.py to
validate and fail fast when configured prediction_keys are invalid.
Comment on lines
+31
to
+44
| def update(self, predictions: list[dict]) -> None: | ||
| super().update(predictions) | ||
| predicted_answers = [p.get("generation") or p.get("pred_text") or None for p in predictions] | ||
| self._compute_pass_at_k(predictions=predictions, predicted_answers=predicted_answers) | ||
| if len(predictions) > 1: | ||
| self._compute_majority_at_k(predictions=predictions, predicted_answers=predicted_answers) | ||
|
|
||
| first = predictions[0] | ||
| for mode in ["pass@1", "pass@1[avg-of-1]"]: | ||
| self.eval_dict[mode]["cpwer_errors"] += first.get("cpwer_errors", 0) | ||
| self.eval_dict[mode]["cpwer_substitutions"] += first.get("cpwer_substitutions", 0) | ||
| self.eval_dict[mode]["cpwer_insertions"] += first.get("cpwer_insertions", 0) | ||
| self.eval_dict[mode]["cpwer_deletions"] += first.get("cpwer_deletions", 0) | ||
| self.eval_dict[mode]["cpwer_ref_words"] += first.get("cpwer_ref_words", 0) |
Contributor
There was a problem hiding this comment.
Prefer pred_text over generation here too.
LibriSpeechMixSOTEvaluator._get_hypothesis() picks pred_text first, but update() reverses that order. If both fields exist and differ, pass@k/no-answer bookkeeping can drift from the cpWER counts accumulated from the evaluated hypothesis.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemo_skills/evaluation/metrics/librispeechmix_sot_metrics.py` around lines 31
- 44, The update() logic currently prefers "generation" over "pred_text", which
is inconsistent with LibriSpeechMixSOTEvaluator._get_hypothesis(); change the
predicted_answers list comprehension in update() to prefer pred_text first (use
p.get("pred_text") or p.get("generation") or None) and also when accumulating
cpwer counts for the representative item, derive the chosen hypothesis
consistently (e.g., chosen = first.get("pred_text") or first.get("generation"))
so the pass@k/no-answer bookkeeping matches the hypothesis used for cpWER
aggregation.
Comment on lines
+79
to
+80
| "langid=en", | ||
| "gt_lang_attr_name=target_lang", |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Validate or honor non-English language settings.
langid is hardcoded to en, so callers can pass source_lang / target_lang values that the actual RTMT-ASR invocation ignores. Either derive langid from the config or reject anything but English here so unsupported settings fail loudly.
As per coding guidelines, "Avoid cases where user-passed parameters are unused; code should fail if user specifies an unsupported argument or if a required argument is missing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemo_skills/inference/librispeechmix_rtmtasr.py` around lines 79 - 80, The
code currently hardcodes langid="en" while accepting source_lang/target_lang, so
either derive langid from the incoming config (e.g., use source_lang or
target_lang when present and map to the expected token like "en"/"fr") or
validate and raise an explicit error if a non-"en" language is passed; update
the RTMT-ASR invocation where "langid=en" and "gt_lang_attr_name=target_lang"
are composed to replace the hardcoded literal with the resolved/validated langid
and ensure callers cannot pass unused source_lang/target_lang values (throw an
exception or return a clear validation error when unsupported languages are
supplied).
Comment on lines
+105
to
+108
| row = json.loads(line) | ||
| pred_text = row.get("pred_text") or row.get("generation") or "" | ||
| row["pred_text"] = pred_text | ||
| row["generation"] = row.get("generation") or pred_text |
Contributor
There was a problem hiding this comment.
Don't normalize missing prediction fields into empty hypotheses.
If the RTMT-ASR script changes schema or emits a bad row, this rewrites it as an empty hypothesis and turns the failure into deletion-heavy cpWER instead of surfacing the incompatibility. Require a supported prediction field here and raise if it is absent.
Suggested fix
- pred_text = row.get("pred_text") or row.get("generation") or ""
+ pred_text = row.get("pred_text") or row.get("generation")
+ if not pred_text:
+ raise ValueError(f"Missing prediction text in RTMT-ASR output row: {row}")
row["pred_text"] = pred_text
row["generation"] = row.get("generation") or pred_textAs per coding guidelines, "Don't use .get() for accessing dictionary keys if the code expects them to be present; use direct access data[key_name] to fail with a clear error instead of silently corrupting data".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemo_skills/inference/librispeechmix_rtmtasr.py` around lines 105 - 108,
Currently the code silently normalizes missing prediction fields into empty
strings by using row.get(...) which masks schema problems; change it to require
a supported prediction field and fail loudly: check row for the presence of
"pred_text" or "generation", raise a ValueError (or KeyError) with a clear
message if neither exists, and then set row["pred_text"] = row["pred_text"] if
present else row["generation"] and ensure row["generation"] is set from the
present field — replace uses of row.get("pred_text")/row.get("generation") with
direct indexing and an explicit presence check to surface incompatible/malformed
rows.
Comment on lines
+98
to
+102
| known = {field.name for field in cls.__dataclass_fields__.values()} | ||
| return cls( | ||
| **{k: v for k, v in d.items() if k in known and k != "extra_config"}, | ||
| extra_config={k: v for k, v in d.items() if k not in known}, | ||
| ) |
Contributor
There was a problem hiding this comment.
Reject unknown backend config keys instead of ignoring them.
Anything outside known is moved into extra_config and then never applied by this backend. A misspelled option will look accepted but won't affect inference.
As per coding guidelines: "Avoid cases where user-passed parameters are unused; code should fail if user specifies an unsupported argument or if a required argument is missing. Use dataclass or **kwargs syntax to handle this automatically"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@recipes/multimodal/server/backends/multitalker_parakeet_backend.py` around
lines 98 - 102, The current deserialization logic in the
MultitalkerParakeetBackend class (the method that uses cls.__dataclass_fields__
to build cls(...)) silently moves unknown keys into extra_config; instead,
validate the input dict by computing unknown_keys = set(d.keys()) - known -
{"extra_config"} and if unknown_keys is non-empty raise a TypeError (or
ValueError) listing the offending keys so typos are rejected, then construct the
dataclass using only known keys (and handle extra_config only if explicitly
provided). Update the method (the classmethod that returns cls(...)) to perform
this check before instantiating the class.
Comment on lines
+121
to
+122
| except Exception as exc: | ||
| logger.warning("SALM warmup skipped due to error: %s", exc) |
Contributor
There was a problem hiding this comment.
Don't hide warmup failures behind a warning.
Warmup is the only place that verifies this backend can actually build prompts and call generate(). Swallowing every exception here means an incompatible model can report as loaded and only fail on the first real request. Let unexpected failures abort load_model(), or narrow the catch to a documented benign case.
As per coding guidelines, "Don't catch exceptions when they are not expected to be normally raised; let the code fail when something unexpected happens so users notice it instead of silently misbehaving".
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 121-121: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@recipes/multimodal/server/backends/salm_backend.py` around lines 121 - 122,
The warmup exception handling in load_model()/SALM backend currently swallows
all exceptions (except Exception) which can hide fatal incompatibilities; change
the except block in the warmup routine so that only documented/benign exceptions
are caught (e.g., the specific warmup-related exception type your backend
raises) and logged, and re-raise any other exceptions so load_model() fails
fast; alternatively, log the error and immediately raise the caught exception
after logging (preserving stack) instead of silently returning success. Ensure
you reference the warmup call inside load_model()/the SALM backend’s warmup
block and adjust the except Exception as written to either a specific exception
class or to re-raise after logger.warning.
Comment on lines
+166
to
+175
| max_new_tokens = self.config.max_new_tokens | ||
| audio_tag = self._model.audio_locator_tag | ||
| for out_idx, path in enumerate(temp_paths): | ||
| request = requests[valid_indices[out_idx]] | ||
| user_prompt = request.user_prompt or request.extra_params.get("user_prompt", self.salm_config.user_prompt) | ||
| prompts.append([{"role": "user", "content": f"{user_prompt} {audio_tag}", "audio": [path]}]) | ||
| if request.max_new_tokens: | ||
| max_new_tokens = max(max_new_tokens, request.max_new_tokens) | ||
|
|
||
| output_ids = self._model.generate(prompts=prompts, max_new_tokens=max_new_tokens) |
Contributor
There was a problem hiding this comment.
Honor batch_size instead of sending the whole request list at once.
This backend exposes batch_size, but generate() still submits every valid request in one model call. Large request bursts can still OOM or spike latency even when the backend is configured smaller. Chunk prompts by self.salm_config.batch_size, or reject oversized batches explicitly.
As per coding guidelines, "Avoid cases where user-passed parameters are unused; code should fail if user specifies an unsupported argument or if a required argument is missing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@recipes/multimodal/server/backends/salm_backend.py` around lines 166 - 175,
The current loop builds a single prompts list for all valid requests and calls
self._model.generate once, ignoring self.salm_config.batch_size; change this to
respect batch_size by chunking the prompts/temp_paths/valid_indices into slices
of size self.salm_config.batch_size, for each chunk compute a
chunk_max_new_tokens (start from self.config.max_new_tokens and take the max
with any request.max_new_tokens in that slice), call
self._model.generate(prompts=chunk_prompts, max_new_tokens=chunk_max_new_tokens)
for each chunk, and concatenate the returned output_ids in order; ensure you
update references to prompts, temp_paths and valid_indices used in the original
loop (e.g., the loop that builds prompts and the single generate() call) so
oversized batches are processed in bounded-size model calls rather than one big
call.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a LibriSpeechMix SOT benchmark group for evaluating multispeaker ASR models with serialized speaker tags:
librispeechmix-sot.{under20s,over20s}-{dev-clean,test-clean}-{1mix,2mix,3mix}under20s <= 20.0,over20s > 20.0)[s0],[s1], etc., RTTM supervision, and SegLST sidecarsmeeteval-weris installedserve_unifiedmultitalker_parakeetunified backend that loads Multitalker Parakeet ASR and Sortformer diarization onceThe Multitalker baseline defaults to:
nvidia/multitalker-parakeet-streaming-0.6b-v1nvidia/diar_streaming_sortformer_4spk-v2.1Testing
Rebased on current
github/main(7dcb0a67).Local focused tests:
Result:
37 passed in 18.54s.Compile checks:
Result: passed.
Also ran:
git diff --check: passed[s0]SOT output anddebug_info.persistent_inprocess: trueCluster Results
Full under-20s LibriSpeechMix test-clean runs completed on DRACO OCI via Symphony job:
issue81-full-multitalker-20260501-0005580.symphony/artifacts/issue81-final-report.md.symphony/artifacts/jobs/issue81-full-multitalker-20260501-000558/job.logEach run used
--num-chunks 16,--server-batch-size 2,--mt-batch-size 2, and--max-concurrent-requests 2.Remote output roots:
issue81_librispeechmix_sot/baselines/multitalker-parakeet/under20s-test-clean-1mix-full,under20s-test-clean-2mix-full,under20s-test-clean-3mix-fullLocal copied metrics:
.symphony/artifacts/issue81-baselines/multitalker-parakeet/under20s-test-clean-1mix-full/metrics.json.symphony/artifacts/issue81-baselines/multitalker-parakeet/under20s-test-clean-2mix-full/metrics.json.symphony/artifacts/issue81-baselines/multitalker-parakeet/under20s-test-clean-3mix-full/metrics.jsonMultitalker Parakeet cpWER:
Notes
meeteval-werwas not installed in the local scoring venv, so optional MeetEval average/per-record JSON was skipped. The scorer still wrote SegLST sidecars and NeMo cpWER metrics.