feat(speech): add AppTek Call-Center Dialogues ASR benchmark#1443
Closed
pzelasko wants to merge 1 commit into
Closed
feat(speech): add AppTek Call-Center Dialogues ASR benchmark#1443pzelasko wants to merge 1 commit into
pzelasko wants to merge 1 commit into
Conversation
pzelasko
marked this pull request as ready for review
May 29, 2026 00:29
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the AppTek Call-Center Dialogues ASR benchmark: docs, HF dataset preparation with metadata-only/audio-prefix options, AppTek text normalization and large mapping tables, evaluator integration, a Parakeet v3 reproduction script, and tests/CI adjustments. ChangesAppTek Call-Center Dialogues Dataset
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Manifest as test.jsonl
participant VAD as SileroVAD
participant ASR as NeMoASRModel
participant Scorer as AppTekNormalizer+jiwer
participant Output as metrics.json
CLI->>Manifest: load manifest
alt silero mode
CLI->>VAD: generate segments (segments.jsonl + WAVs)
VAD->>CLI: segment list
CLI->>ASR: transcribe segments
ASR->>CLI: segment predictions
CLI->>CLI: aggregate segments -> file-level predictions
else direct mode
CLI->>ASR: transcribe full audio files
ASR->>CLI: file-level predictions
end
CLI->>Scorer: normalize refs/hyps and compute WER per-accent
Scorer->>Output: write metrics.json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 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 unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
nemo_skills/pipeline/judges/nvembed_judge.py (1)
102-124:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake CLI arg assembly shell-safe for path values.
Building args as
"--flag value"strings and then joining can break when values contain spaces. Build tokenized args and quote at render time.💡 Proposed fix
+import shlex @@ - script_args = [f"--output-dir {output_dir_path}"] + script_args = ["--output-dir", str(output_dir_path)] @@ - script_args.append(f"--input-dir {input_dir}") - script_args.append(f"--num-seeds {num_seeds}") + script_args.extend(["--input-dir", str(input_dir)]) + script_args.extend(["--num-seeds", str(num_seeds)]) else: - script_args.append(f"--input-file {input_file}") + script_args.extend(["--input-file", str(input_file)]) @@ - run_cmd = ( + run_cmd = ( "NVEMBED_DEPS_DIR=/tmp/nvembed_deps_${SLURM_JOB_ID:-$$} && " @@ - f"python3 /nemo_run/code/nemo_skills/evaluation/evaluator/nvembed_judge.py {' '.join(script_args)}" + f"python3 /nemo_run/code/nemo_skills/evaluation/evaluator/nvembed_judge.py " + f"{' '.join(shlex.quote(arg) for arg in script_args)}" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemo_skills/pipeline/judges/nvembed_judge.py` around lines 102 - 124, The current assembly of CLI args in script_args uses plain f-strings and a join which is unsafe for values with spaces; change script_args to be a list of tokens (e.g., append "--input-dir", input_dir instead of "--input-dir {input_dir}") and for flags without values keep single tokens (e.g., "--skip-existing"), then when building run_cmd render those tokens safely by importing shlex and using ' '.join(shlex.quote(t) for t in script_args) inside the f-string that constructs run_cmd; update references to script_args, run_cmd, input_file, output_dir_path, num_seeds and rerun_done accordingly.nemo_skills/dataset/audiobench/prepare.py (2)
607-621:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow unexpected dataset-level exceptions.
Catching
Exceptionhere hides root causes and allows partial outputs to look successful.💡 Proposed fix
- try: - num_samples, manifest_entries = process_dataset( - dataset_name=dataset_name, - output_dir=output_dir, - save_audio=args.save_audio, - split=args.split, - max_samples=args.max_samples, - ) - total_samples += num_samples - total_datasets += 1 - combined_entries[category].extend(manifest_entries) - print(f"✓ Completed {dataset_name}: {num_samples} samples") - except Exception as e: - print(f"✗ Failed {dataset_name}: {e}") - continue + num_samples, manifest_entries = process_dataset( + dataset_name=dataset_name, + output_dir=output_dir, + save_audio=args.save_audio, + split=args.split, + max_samples=args.max_samples, + ) + total_samples += num_samples + total_datasets += 1 + combined_entries[category].extend(manifest_entries) + print(f"✓ Completed {dataset_name}: {num_samples} samples")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".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemo_skills/dataset/audiobench/prepare.py` around lines 607 - 621, The current broad except Exception in the per-dataset loop around process_dataset swallows unexpected errors; update the handler in the block that calls process_dataset (and which updates combined_entries and total_samples/total_datasets) to only catch expected, documented exceptions (e.g., a custom DatasetNotFoundError or ValidationError if those exist) and handle those by logging the failure for that dataset, but re-raise or let other exceptions propagate so the process fails visibly; alternatively remove the broad except entirely and only log successes, or if you must log failures, log and then re-raise the exception after printing to avoid hiding root causes.
605-617:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on unsupported dataset names instead of defaulting to nonjudge.
Line 605 maps any non-judge dataset to
"nonjudge", which can misclassify unknown/custom datasets and pollute the combined nonjudge manifest.💡 Proposed fix
- # Determine category for logging - category = "judge" if dataset_name in JUDGE_DATASETS else "nonjudge" + # Determine category explicitly; fail fast on unsupported names + if dataset_name in JUDGE_DATASETS: + category = "judge" + elif dataset_name in NONJUDGE_DATASETS: + category = "nonjudge" + else: + raise ValueError(f"Unsupported dataset name: {dataset_name}")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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemo_skills/dataset/audiobench/prepare.py` around lines 605 - 617, The code currently assigns category = "judge" if dataset_name in JUDGE_DATASETS else "nonjudge", which silently classifies unknown names as nonjudge; change this to validate dataset_name explicitly against known sets and fail fast: check whether dataset_name is in JUDGE_DATASETS or in a known NONJUDGE_DATASETS (or an ALL_DATASETS union) before calling process_dataset, raise a ValueError (or custom exception) if it's not found, and then set category to "judge" or "nonjudge" accordingly so combined_entries[category].extend(manifest_entries) only runs for supported datasets; use the symbols dataset_name, JUDGE_DATASETS, (or NONJUDGE_DATASETS/ALL_DATASETS), process_dataset and combined_entries to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/evaluation/speech-audio.md`:
- Around line 315-320: The fenced code block showing the output tree (the triple
backticks surrounding "<output_dir>/ ... metrics.json") lacks a language tag
which triggers MD040; update that fenced block to include a language identifier
(for example "text") immediately after the opening backticks so the block
becomes "```text" and the markdown linter stops flagging it.
In `@nemo_skills/evaluation/evaluator/nvembed_judge.py`:
- Line 45: The subprocess invocation in install_packages() in nvembed_judge.py
uses a bare "pip" which can target the wrong Python environment; change it to
invoke the current interpreter by importing sys and running subprocess.run with
[sys.executable, "-m", "pip", "install", ...] (preserve flags like "-q" and
package list) for all pip install calls so packages are installed into the
active Python environment.
In `@nemo_skills/inference/generate.py`:
- Around line 920-925: The loop over generations extracts async_pos via
self.cfg.async_position_key and currently assigns ordered_generations[async_pos]
= gen_dict without checking for duplicates; modify the block in generate.py (the
loop handling generations) to first check if async_pos is already present in
ordered_generations and raise a ValueError (or appropriate exception) if so
(e.g., "Duplicate async output position {async_pos}"), keep the existing
negative-position check, and only then assign ordered_generations[async_pos] =
gen_dict to prevent silent overwrites.
In `@nemo_skills/inference/model/vllm_multimodal.py`:
- Around line 324-331: The dict-branch that handles content objects currently
preserves {"type":"text","text":None} as a text item; change that so when
content.get("type") == "text" you normalize text==None to an empty content list
(same behavior used in the other branch), i.e. when building result["content"]
from a dict ensure you convert None text to [] instead of
[{"type":"text","text":None}]; update the block handling isinstance(content,
dict) (and apply the same fix to the analogous block around lines 444-451) so
both paths consistently produce [] for None text values.
In `@nemo_skills/pipeline/judges/nvembed_judge.py`:
- Around line 114-115: CI is failing due to formatting changes around the line
that appends the flag (script_args.append("--skip-install")) in
nvembed_judge.py; run the project's formatter (ruff or ruff-format) against that
file (or run the repo's precommit formatting) and commit the formatted
nvembed_judge.py so the ruff-format differences are resolved.
In `@nemo_skills/utils.py`:
- Around line 621-637: The loop in get_server_wait_cmd uses "curl -fsS" so any
reachable endpoint that returns non-2xx (401/403/404) makes curl exit non‑zero
and the wait loop never finishes; update the returned command so both curl calls
remove the "-f" flag (use "-sS" or otherwise omit --fail) so a reachable server
response (even non-2xx) breaks the loop; modify the string construction that
references models_url and health_url in get_server_wait_cmd to use "curl -sS
{models_url}" and "curl -sS {health_url}" instead of "curl -fsS ..." so the
probe stops waiting when the server is reachable.
In `@recipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.py`:
- Around line 256-257: Replace the non-strict zip calls that can silently drop
items by adding strict=True so mismatched lengths raise immediately: change the
loop "for source_row, text in zip(batch, texts)" to "for source_row, text in
zip(batch, texts, strict=True)" and likewise update the other loop that zips
batch with segments/hypotheses (the second "for source_row, ... in zip(batch,
...)" in reproduce_parakeet_v3.py) to use strict=True to preserve row↔prediction
alignment.
---
Outside diff comments:
In `@nemo_skills/dataset/audiobench/prepare.py`:
- Around line 607-621: The current broad except Exception in the per-dataset
loop around process_dataset swallows unexpected errors; update the handler in
the block that calls process_dataset (and which updates combined_entries and
total_samples/total_datasets) to only catch expected, documented exceptions
(e.g., a custom DatasetNotFoundError or ValidationError if those exist) and
handle those by logging the failure for that dataset, but re-raise or let other
exceptions propagate so the process fails visibly; alternatively remove the
broad except entirely and only log successes, or if you must log failures, log
and then re-raise the exception after printing to avoid hiding root causes.
- Around line 605-617: The code currently assigns category = "judge" if
dataset_name in JUDGE_DATASETS else "nonjudge", which silently classifies
unknown names as nonjudge; change this to validate dataset_name explicitly
against known sets and fail fast: check whether dataset_name is in
JUDGE_DATASETS or in a known NONJUDGE_DATASETS (or an ALL_DATASETS union) before
calling process_dataset, raise a ValueError (or custom exception) if it's not
found, and then set category to "judge" or "nonjudge" accordingly so
combined_entries[category].extend(manifest_entries) only runs for supported
datasets; use the symbols dataset_name, JUDGE_DATASETS, (or
NONJUDGE_DATASETS/ALL_DATASETS), process_dataset and combined_entries to locate
and update the logic.
In `@nemo_skills/pipeline/judges/nvembed_judge.py`:
- Around line 102-124: The current assembly of CLI args in script_args uses
plain f-strings and a join which is unsafe for values with spaces; change
script_args to be a list of tokens (e.g., append "--input-dir", input_dir
instead of "--input-dir {input_dir}") and for flags without values keep single
tokens (e.g., "--skip-existing"), then when building run_cmd render those tokens
safely by importing shlex and using ' '.join(shlex.quote(t) for t in
script_args) inside the f-string that constructs run_cmd; update references to
script_args, run_cmd, input_file, output_dir_path, num_seeds and rerun_done
accordingly.
🪄 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: 6ad68b24-7575-49f2-9147-83a17e2ad8cf
📒 Files selected for processing (27)
docs/evaluation/speech-audio.mdnemo_skills/dataset/apptek-callcenter-dialogues/__init__.pynemo_skills/dataset/apptek-callcenter-dialogues/prepare.pynemo_skills/dataset/audiobench/nonjudge/__init__.pynemo_skills/dataset/audiobench/prepare.pynemo_skills/dataset/mmau-pro/closed_form/__init__.pynemo_skills/dataset/mmau-pro/prepare.pynemo_skills/evaluation/evaluator/apptek_callcenter.pynemo_skills/evaluation/evaluator/apptek_callcenter_word_mappings.pynemo_skills/evaluation/evaluator/audio.pynemo_skills/evaluation/evaluator/mmau_pro.pynemo_skills/evaluation/evaluator/nvembed_judge.pynemo_skills/inference/generate.pynemo_skills/inference/model/vllm_multimodal.pynemo_skills/pipeline/eval.pynemo_skills/pipeline/judges/nvembed_judge.pynemo_skills/pipeline/utils/cluster.pynemo_skills/pipeline/utils/exp.pynemo_skills/pipeline/utils/scripts/eval.pynemo_skills/pipeline/utils/scripts/server.pynemo_skills/pipeline/utils/server.pynemo_skills/utils.pyrecipes/apptek-callcenter-dialogues/README.mdrecipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.pytests/gpu-tests/test_eval.pytests/test_audio_evaluator.pytests/test_datasets.py
| LOG.info("Installing required packages...") | ||
| subprocess.run( | ||
| ["pip", "install", "-q", "datasets", "einops", "transformers==4.42.4", "tqdm"], | ||
| ["pip", "install", "-q", "numpy<2", "datasets", "einops", "transformers==4.42.4", "tqdm"], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all pip subprocess invocations and whether they are interpreter-bound.
rg -n -C2 'subprocess\.run\(\s*\[.*pip' --type=py
rg -n -C2 'sys\.executable.*-m",\s*"pip"' --type=pyRepository: NVIDIA-NeMo/Skills
Length of output: 2715
🏁 Script executed:
#!/bin/bash
# 1) Inspect the exact pip invocation in the targeted file
rg -n --hidden --no-ignore-vcs 'nvembed_judge\.py|subprocess\.(run|check_call|check_output)\(' .
rg -n 'pip(\.exe)?\b|pip\s+install|numpy<2|transformers==4\.42\.4|datasets|einops|tqdm' nemo_skills/evaluation/evaluator/nvembed_judge.py -S || true
# 2) Find any other bare "pip" subprocess calls that match the same anti-pattern
rg -n --hidden --no-ignore-vcs 'subprocess\.(run|check_call|check_output)\([^)]*\["pip"|subprocess\.run\(\s*["'\'']pip|subprocess\.run\(\s*\["pip"' --type=py .
# 3) Look for pip usage even when not passed as an argv list (e.g., shell=True)
rg -n --hidden --no-ignore-vcs 'pip\s+install' --type=py .Repository: NVIDIA-NeMo/Skills
Length of output: 16273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact code around the reported line in nvembed_judge.py
sed -n '1,120p' nemo_skills/evaluation/evaluator/nvembed_judge.py | nl -ba | sed -n '30,90p'
# Search within this file for any interpreter-bound pip usage
rg -n 'sys\.executable.*-m\s*pip|pip(\.exe)?\s*,|-m\s*pip' nemo_skills/evaluation/evaluator/nvembed_judge.py
# Also show any other subprocess.run(...) that includes "pip" in this file
rg -n 'subprocess\.(run|check_call|check_output)\([^)]*pip' nemo_skills/evaluation/evaluator/nvembed_judge.pyRepository: NVIDIA-NeMo/Skills
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print nvembed_judge.py with line numbers (without nl)
awk '{printf "%5d\t%s\n", NR, $0}' nemo_skills/evaluation/evaluator/nvembed_judge.py | sed -n '30,80p'
# Search within this file for any interpreter-bound pip usage
rg -n 'sys\.executable.*-m\s*pip|python3?\s*-m\s*pip|pip(\.exe)?\s*,|-m\s*pip' nemo_skills/evaluation/evaluator/nvembed_judge.py || true
# Show any subprocess calls in this file that include "pip"
rg -n 'subprocess\.(run|check_call|check_output)\([^)]*pip' nemo_skills/evaluation/evaluator/nvembed_judge.py || trueRepository: NVIDIA-NeMo/Skills
Length of output: 1966
Use sys.executable -m pip instead of bare pip in NVEmbed evaluator.
install_packages() in nemo_skills/evaluation/evaluator/nvembed_judge.py runs subprocess.run(["pip", "install", ...]), which can install into a different environment than the current Python interpreter.
Proposed fix
- subprocess.run(
- ["pip", "install", "-q", "numpy<2", "datasets", "einops", "transformers==4.42.4", "tqdm"],
+ subprocess.run(
+ [sys.executable, "-m", "pip", "install", "-q", "numpy<2", "datasets", "einops", "transformers==4.42.4", "tqdm"],
check=True,
capture_output=True,
text=True,
)🧰 Tools
🪛 Ruff (0.15.14)
[error] 45-45: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemo_skills/evaluation/evaluator/nvembed_judge.py` at line 45, The subprocess
invocation in install_packages() in nvembed_judge.py uses a bare "pip" which can
target the wrong Python environment; change it to invoke the current interpreter
by importing sys and running subprocess.run with [sys.executable, "-m", "pip",
"install", ...] (preserve flags like "-q" and package list) for all pip install
calls so packages are installed into the active Python environment.
Comment on lines
920
to
925
| for gen_dict in generations: | ||
| async_pos = gen_dict.pop(self.cfg.async_position_key) | ||
| if async_pos < 0: | ||
| raise ValueError(f"Invalid async output position {async_pos}") | ||
| ordered_generations[async_pos] = gen_dict | ||
|
|
Contributor
There was a problem hiding this comment.
Detect duplicate async positions before assignment.
If _async_position appears twice, Line 924 silently overwrites the earlier row, losing data without error.
💡 Proposed fix
for gen_dict in generations:
async_pos = gen_dict.pop(self.cfg.async_position_key)
if async_pos < 0:
raise ValueError(f"Invalid async output position {async_pos}")
+ if ordered_generations[async_pos] is not None:
+ raise RuntimeError(f"Duplicate async output position {async_pos}")
ordered_generations[async_pos] = gen_dict🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemo_skills/inference/generate.py` around lines 920 - 925, The loop over
generations extracts async_pos via self.cfg.async_position_key and currently
assigns ordered_generations[async_pos] = gen_dict without checking for
duplicates; modify the block in generate.py (the loop handling generations) to
first check if async_pos is already present in ordered_generations and raise a
ValueError (or appropriate exception) if so (e.g., "Duplicate async output
position {async_pos}"), keep the existing negative-position check, and only then
assign ordered_generations[async_pos] = gen_dict to prevent silent overwrites.
Comment on lines
+324
to
+331
| elif isinstance(content, dict): | ||
| if content.get("type") == "text" and "text" in content: | ||
| result["content"] = [content] | ||
| elif "text" in content: | ||
| text = content["text"] | ||
| result["content"] = [] if text is None else [{"type": "text", "text": str(text)}] | ||
| else: | ||
| raise TypeError(f"Unexpected content dict keys: {sorted(content)}") |
Contributor
There was a problem hiding this comment.
Normalize {"type":"text","text":None} as empty content consistently.
Both updated branches currently keep text=None when type=="text", but the sibling path treats None as empty. This inconsistency can produce invalid text items in request payloads.
💡 Proposed fix
- elif isinstance(content, dict):
- if content.get("type") == "text" and "text" in content:
- result["content"] = [content]
+ elif isinstance(content, dict):
+ if content.get("type") == "text" and "text" in content:
+ text = content["text"]
+ result["content"] = [] if text is None else [{"type": "text", "text": str(text)}]
elif "text" in content:
text = content["text"]
result["content"] = [] if text is None else [{"type": "text", "text": str(text)}]
else:
raise TypeError(f"Unexpected content dict keys: {sorted(content)}")- elif isinstance(content, dict):
- if content.get("type") == "text" and "text" in content:
- text_content = [content]
+ elif isinstance(content, dict):
+ if content.get("type") == "text" and "text" in content:
+ text = content["text"]
+ text_content = [] if text is None else [{"type": "text", "text": str(text)}]
elif "text" in content:
text = content["text"]
text_content = [] if text is None else [{"type": "text", "text": str(text)}]
else:
raise TypeError(f"Unexpected content dict keys: {sorted(content)}")Also applies to: 444-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemo_skills/inference/model/vllm_multimodal.py` around lines 324 - 331, The
dict-branch that handles content objects currently preserves
{"type":"text","text":None} as a text item; change that so when
content.get("type") == "text" you normalize text==None to an empty content list
(same behavior used in the other branch), i.e. when building result["content"]
from a dict ensure you convert None text to [] instead of
[{"type":"text","text":None}]; update the block handling isinstance(content,
dict) (and apply the same fix to the analogous block around lines 444-451) so
both paths consistently produce [] for None text values.
Comment on lines
+114
to
+115
| script_args.append("--skip-install") | ||
|
|
Contributor
There was a problem hiding this comment.
Fix the formatting failure before merge.
CI is currently failing due to ruff-format changes in this file (around Line 115). Please run formatting and commit the result.
🧰 Tools
🪛 GitHub Actions: Lint and Format / 0_pre-commit.txt
[error] 115-115: pre-commit hook ruff-format failed: files were modified by this hook (1 file reformatted). Re-run and commit the formatting changes.
🪛 GitHub Actions: Lint and Format / pre-commit
[error] 115-115: Pre-commit ruff-format failed: files were modified by hook. 1 file reformatted (hook id: ruff-format). Please commit the formatting changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemo_skills/pipeline/judges/nvembed_judge.py` around lines 114 - 115, CI is
failing due to formatting changes around the line that appends the flag
(script_args.append("--skip-install")) in nvembed_judge.py; run the project's
formatter (ruff or ruff-format) against that file (or run the repo's precommit
formatting) and commit the formatted nvembed_judge.py so the ruff-format
differences are resolved.
Comment on lines
621
to
637
| def get_server_wait_cmd(server_address): | ||
| server_url = server_address.rstrip("/") | ||
| if not server_url.startswith(("http://", "https://")): | ||
| server_url = f"http://{server_url}" | ||
| if server_url.endswith("/v1"): | ||
| models_url = f"{server_url}/models" | ||
| health_url = f"{server_url[:-3]}/health" | ||
| else: | ||
| models_url = f"{server_url}/v1/models" | ||
| health_url = f"{server_url}/health" | ||
| # might be required if we are not hosting server ourselves | ||
| # this will try to handshake in a loop and unblock when the server responds | ||
| # this will try to handshake in a loop and unblock when the server is ready | ||
| return ( | ||
| f"echo 'Waiting for the server to start at {server_address}' && " | ||
| f"while [ $(curl -X PUT {server_address} >/dev/null 2>&1; echo $?) -ne 0 ]; do sleep 3; done " | ||
| f"while ! (curl -fsS {models_url} >/dev/null 2>&1 || " | ||
| f"curl -fsS {health_url} >/dev/null 2>&1); do sleep 3; done " | ||
| ) |
Contributor
There was a problem hiding this comment.
Avoid infinite wait when probe endpoints return non-2xx.
Using curl -f means reachable servers that return 401/403/404 never satisfy the loop, so startup can hang forever.
💡 Proposed fix
def get_server_wait_cmd(server_address):
@@
- return (
- f"echo 'Waiting for the server to start at {server_address}' && "
- f"while ! (curl -fsS {models_url} >/dev/null 2>&1 || "
- f"curl -fsS {health_url} >/dev/null 2>&1); do sleep 3; done "
- )
+ return (
+ f"echo 'Waiting for the server to start at {server_address}' && "
+ f"while true; do "
+ f"models_code=$(curl -s -o /dev/null -w '%{{http_code}}' --connect-timeout 2 --max-time 5 {models_url} || echo 000); "
+ f"health_code=$(curl -s -o /dev/null -w '%{{http_code}}' --connect-timeout 2 --max-time 5 {health_url} || echo 000); "
+ f"if [ \"$models_code\" != '000' ] || [ \"$health_code\" != '000' ]; then break; fi; "
+ f"sleep 3; "
+ f"done"
+ )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemo_skills/utils.py` around lines 621 - 637, The loop in get_server_wait_cmd
uses "curl -fsS" so any reachable endpoint that returns non-2xx (401/403/404)
makes curl exit non‑zero and the wait loop never finishes; update the returned
command so both curl calls remove the "-f" flag (use "-sS" or otherwise omit
--fail) so a reachable server response (even non-2xx) breaks the loop; modify
the string construction that references models_url and health_url in
get_server_wait_cmd to use "curl -sS {models_url}" and "curl -sS {health_url}"
instead of "curl -fsS ..." so the probe stops waiting when the server is
reachable.
This was referenced Jun 4, 2026
pzelasko
force-pushed
the
speech/apptek-callcenter-dialogues
branch
from
June 4, 2026 17:17
e376488 to
6973da2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemo_skills/dataset/apptek-callcenter-dialogues/prepare.py`:
- Around line 287-297: The current loop opens output_file in write mode
immediately which truncates test.jsonl before processing completes; instead,
collect all valid entries first (use _iter_metadata_rows and _format_entry to
build a list while counting skipped), only open output_file for writing after
processing succeeds (or write to a temp file and atomically replace the target),
and then write the json.dumps lines and update count/skipped and the
BENCHMARK_NAME progress exactly as before to avoid partial/truncated manifests
on failure.
In `@nemo_skills/evaluation/evaluator/audio.py`:
- Around line 452-456: The AppTek branch hard-codes is_prediction=False when
calling normalize_apptek_callcenter_text, causing hypotheses to be normalized as
references; update the relevant caller (preprocess_asr_text or the function that
dispatches on mode in nemo_skills.evaluation.evaluator.audio) to accept and
forward an is_prediction boolean and call normalize_apptek_callcenter_text(text,
is_prediction=is_prediction) instead of hard-coding False; if the public API
already accepts kwargs, forward **kwargs or add is_prediction to the function
signature and raise a clear error if callers pass an unsupported argument so the
normalization behavior is explicit.
🪄 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: 46428096-96ec-4f08-ab7e-226a5fe2941c
📒 Files selected for processing (11)
docs/evaluation/speech-audio.mdnemo_skills/dataset/apptek-callcenter-dialogues/__init__.pynemo_skills/dataset/apptek-callcenter-dialogues/prepare.pynemo_skills/evaluation/evaluator/apptek_callcenter.pynemo_skills/evaluation/evaluator/apptek_callcenter_word_mappings.pynemo_skills/evaluation/evaluator/audio.pyrecipes/apptek-callcenter-dialogues/README.mdrecipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.pytests/gpu-tests/test_eval.pytests/test_audio_evaluator.pytests/test_datasets.py
✅ Files skipped from review due to trivial changes (2)
- recipes/apptek-callcenter-dialogues/README.md
- tests/gpu-tests/test_eval.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_audio_evaluator.py
- nemo_skills/dataset/apptek-callcenter-dialogues/init.py
- nemo_skills/evaluation/evaluator/apptek_callcenter.py
- docs/evaluation/speech-audio.md
- tests/test_datasets.py
- recipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.py
pzelasko
force-pushed
the
speech/apptek-callcenter-dialogues
branch
2 times, most recently
from
June 4, 2026 17:30
62e5c67 to
bcc5e0f
Compare
Collaborator
Author
|
@coderabbitai review\n\nPushed fixes for the latest AppTek review comments: manifest writing now happens after row formatting succeeds, and AppTek preprocessing forwards is_prediction. Please re-review the current head. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nemo_skills/dataset/apptek-callcenter-dialogues/prepare.py (1)
287-297:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent manifest truncation by writing atomically after successful processing.
Line 290 opens
test.jsonlin write mode before row processing finishes. If any row fails mid-loop, the existing manifest is already truncated and can be left partial. Build all entries first, then write to a temp file and replace atomically.Proposed fix
- count = 0 - skipped = 0 - print(f"Writing {output_file}") - with output_file.open("w", encoding="utf-8") as fout: - for accent_code, row in tqdm(list(_iter_metadata_rows(data_dir, args.accent_codes)), desc=BENCHMARK_NAME): - entry = _format_entry(row, accent_code, data_dir, audio_prefix) - if entry is None: - skipped += 1 - continue - fout.write(json.dumps(entry, ensure_ascii=False) + "\n") - count += 1 + rows_to_write: list[dict] = [] + skipped = 0 + for accent_code, row in tqdm(_iter_metadata_rows(data_dir, args.accent_codes), desc=BENCHMARK_NAME): + entry = _format_entry(row, accent_code, data_dir, audio_prefix) + if entry is None: + skipped += 1 + continue + rows_to_write.append(entry) + + print(f"Writing {output_file}") + tmp_output_file = output_file.with_suffix(".jsonl.tmp") + with tmp_output_file.open("w", encoding="utf-8") as fout: + for entry in rows_to_write: + fout.write(json.dumps(entry, ensure_ascii=False) + "\n") + tmp_output_file.replace(output_file) + count = len(rows_to_write)As per coding guidelines, "When adding new benchmarks, avoid data loss by doing all computation before re-opening files for writing; ensure computation completes before file writes to prevent accidental data loss if code fails".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemo_skills/dataset/apptek-callcenter-dialogues/prepare.py` around lines 287 - 297, The current loop opens output_file for writing up front which can truncate the manifest if processing fails; instead, iterate through _iter_metadata_rows and collect entries using _format_entry (track count/skipped), then after successful processing write the collected JSON lines to a temporary file and atomically replace output_file (e.g., write to output_file.with_suffix(".tmp") and move/rename) so the manifest is only overwritten on successful completion; keep BENCHMARK_NAME progress reporting during collection and ensure you still update count and skipped before the atomic replace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@nemo_skills/dataset/apptek-callcenter-dialogues/prepare.py`:
- Around line 287-297: The current loop opens output_file for writing up front
which can truncate the manifest if processing fails; instead, iterate through
_iter_metadata_rows and collect entries using _format_entry (track
count/skipped), then after successful processing write the collected JSON lines
to a temporary file and atomically replace output_file (e.g., write to
output_file.with_suffix(".tmp") and move/rename) so the manifest is only
overwritten on successful completion; keep BENCHMARK_NAME progress reporting
during collection and ensure you still update count and skipped before the
atomic replace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5809084-e929-4fb7-8b93-a87ef87c5b15
📒 Files selected for processing (11)
docs/evaluation/speech-audio.mdnemo_skills/dataset/apptek-callcenter-dialogues/__init__.pynemo_skills/dataset/apptek-callcenter-dialogues/prepare.pynemo_skills/evaluation/evaluator/apptek_callcenter.pynemo_skills/evaluation/evaluator/apptek_callcenter_word_mappings.pynemo_skills/evaluation/evaluator/audio.pyrecipes/apptek-callcenter-dialogues/README.mdrecipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.pytests/gpu-tests/test_eval.pytests/test_audio_evaluator.pytests/test_datasets.py
✅ Files skipped from review due to trivial changes (3)
- recipes/apptek-callcenter-dialogues/README.md
- tests/gpu-tests/test_eval.py
- docs/evaluation/speech-audio.md
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_datasets.py
- nemo_skills/dataset/apptek-callcenter-dialogues/init.py
- nemo_skills/evaluation/evaluator/audio.py
- tests/test_audio_evaluator.py
- recipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.py
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the latest PR diff
gh pr view 1443 --repo NVIDIA-NeMo/Skills --json files,headRefName,baseRefName
# Get the diff of key changed files
gh pr diff 1443 --repo NVIDIA-NeMo/SkillsLength of output: 65186
Fix 1 — Atomic manifest write ( Fix 2 — Minor nits found in this pass:
Neither of these blocks merging. The overall implementation looks solid — normalization pipeline, empty-reference guard, accent-level metrics, and the standalone Parakeet recipe all hold up under review. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemo_skills/evaluation/evaluator/apptek_callcenter_word_mappings.py`:
- Line 37: The hesitation removal defined in hesitations (the dict containing
"ahh") is being silently overridden by the "ahh" -> "ah" entry in
word_normalisations; either remove the conflicting "ahh" -> "ah" mapping from
word_normalisations or change the merge/lookup order so hesitations take
precedence (apply hesitations after merging, or ensure checks use hesitations
first). Locate the hesitations dict and the word_normalisations mapping and
either delete the "ahh" mapping there or modify the merge logic to give
hesitations precedence so "ahh" is removed as intended.
🪄 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: a544b063-85ab-4b2c-ad7c-c230e1d37bfd
📒 Files selected for processing (11)
docs/evaluation/speech-audio.mdnemo_skills/dataset/apptek-callcenter-dialogues/__init__.pynemo_skills/dataset/apptek-callcenter-dialogues/prepare.pynemo_skills/evaluation/evaluator/apptek_callcenter.pynemo_skills/evaluation/evaluator/apptek_callcenter_word_mappings.pynemo_skills/evaluation/evaluator/audio.pyrecipes/apptek-callcenter-dialogues/README.mdrecipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.pytests/gpu-tests/test_eval.pytests/test_audio_evaluator.pytests/test_datasets.py
✅ Files skipped from review due to trivial changes (3)
- recipes/apptek-callcenter-dialogues/README.md
- tests/test_datasets.py
- docs/evaluation/speech-audio.md
🚧 Files skipped from review as they are similar to previous changes (6)
- nemo_skills/dataset/apptek-callcenter-dialogues/init.py
- tests/gpu-tests/test_eval.py
- nemo_skills/evaluation/evaluator/apptek_callcenter.py
- nemo_skills/evaluation/evaluator/audio.py
- nemo_skills/dataset/apptek-callcenter-dialogues/prepare.py
- recipes/apptek-callcenter-dialogues/reproduce_parakeet_v3.py
Signed-off-by: Piotr Żelasko <pzelasko@nvidia.com>
pzelasko
force-pushed
the
speech/apptek-callcenter-dialogues
branch
from
June 4, 2026 17:41
bcc5e0f to
37186e5
Compare
Collaborator
Author
|
@coderabbitai review Pushed fixes for the latest AppTek review: hesitation removals now take precedence over broader word mappings, whitespace is normalized after substitutions, and a regression test covers the ahh case. Please re-review the current head. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr diff 1443 --repo NVIDIA-NeMo/SkillsLength of output: 64120
Fix 1 — Hesitation priority ( Fix 2 — Post-substitution whitespace ( Fix 3 — Regression test ( As a bonus, both nits from the previous pass were also cleaned up: the dead The implementation is complete and correct — happy to approve. [approve] |
Collaborator
|
@pzelasko could you please recreate from a branch, so that tests can run? Sent you an invite |
This was referenced Jun 15, 2026
Collaborator
Author
|
Recreated as #1486 from the same-repo branch |
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
2 participants
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 the AppTek Call-Center Dialogues ASR benchmark, sourced from
apptek-com/apptek_callcenter_dialogueson HuggingFace.The benchmark covers long-form English call-center audio: 1,746 split-channel WAVs, roughly 129 hours, across 14 accents and 16 domains.
What Changed
apptek-callcenter-dialoguesdataset package and preparation CLI.--audio-prefixsupport.apptek_callcenterASR normalization that follows the official scoring protocol:recipes/apptek-callcenter-dialogues/.PR Stack
This PR now contains only the AppTek benchmark work.
Validation
python -m py_compileon changed Python files.Follow-Up