[TTS] Update multi-language eval report format#15915
Conversation
Signed-off-by: Ryan <rlangman@nvidia.com>
| 'mscenespeech': 'zh', | ||
| } | ||
|
|
||
| SUPPORTED_BENCHMARK_NAMES = BENCHMARK_META.keys() |
There was a problem hiding this comment.
The type was list[str], but it became KeysView; it should be converted to a list
| @@ -52,4 +67,4 @@ | |||
| DUMMY_TASK_ID: str = "NEMOTTS-0000" | |||
There was a problem hiding this comment.
Since we decided to move from Jira to POR, DUMMY_TASK_ID is no longer valid. Linking a report to Jira used to be optional, so the task ID was optional as well. However, all development work must now be linked to a POR ticket, which means every comparison report should also be linked to a POR ticket. This suggests that we no longer need DUMMY_TASK_ID, and that --task_id should become a required argument in the report-generation CLI. This will also require updates to dependent functionality, such as the TaskInfo structure.
| ) | ||
|
|
||
|
|
||
| def make_task_info(task_id: str) -> TaskInfo: |
There was a problem hiding this comment.
Since we decided to move from Jira to POR, TaskInfo should be updated accordingly. All Jira-specific references should be removed, including variable names and docstrings. It would make sense to keep only the task_id and task_url attributes.
| <ul> | ||
| <li class="link-comment"> | ||
| <a href="{{ jira_url }}" target="_blank" rel="noopener noreferrer">Jira {{ jira_id }}</a> | ||
| <a href="{{ jira_url }}" target="_blank" rel="noopener noreferrer">POR {{ jira_id }}</a> |
There was a problem hiding this comment.
Please rename jira_url to task_url and jira_id to task_id for this template to match the updated terminology
| <ul> | ||
| <li class="link-comment"> | ||
| <a href="{{ jira_url }}" target="_blank" rel="noopener noreferrer">Jira {{ jira_id }}</a> | ||
| <a href="{{ jira_url }}" target="_blank" rel="noopener noreferrer">POR {{ jira_id }}</a> |
There was a problem hiding this comment.
Same: please rename jira_url to task_url and jira_id to task_id for this template to match the updated terminology
| - Both generated reports include a clickable Jira link derived from `--task_id`. | ||
| If no task ID is specified, the link points to the Jira project page. | ||
| - Both generated reports include a clickable POR link derived from `--task_id`. | ||
| If no task ID is specified, the link points to the POR project page. |
There was a problem hiding this comment.
Suggestion: remove this sentence, since every comparison report should now be linked to a POR ticket, so a missing task ID is no longer a valid case
|
Since |
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughTTS inference outputs now organize paths by language and preserve Unicode metrics. Comparison reports add benchmark language labels and replace Jira ticket links and labels with POR links. ChangesTTS inference output organization
TTS comparison report updates
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@examples/tts/magpietts_inference.py`:
- Around line 219-221: Update the checkpoint identity flow around
full_checkpoint_name and append_metrics_to_csv so aggregate CSV rows preserve
dataset language. Either add a language column to the aggregate CSV schema and
populate it at both writer call sites, or include the language in the checkpoint
identifier passed to the writer; ensure multilingual runs remain distinguishable
for the same checkpoint and dataset.
- Line 346: Update the JSON output file opening in the code surrounding
json.dump to explicitly use UTF-8 encoding, while preserving ensure_ascii=False
and the existing serialization behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8ac73b20-ae4a-4cc4-b90f-643f91629d82
📒 Files selected for processing (7)
examples/tts/magpietts_inference.pyscripts/tts_comparison_report/README.mdscripts/tts_comparison_report/reporting/constants.pyscripts/tts_comparison_report/reporting/helpers.pyscripts/tts_comparison_report/reporting/orchestrator.pyscripts/tts_comparison_report/templates/audio_report.jinjascripts/tts_comparison_report/templates/eval_report.jinja
| full_checkpoint_name = ( | ||
| f"{checkpoint_name}_{moe_info}{inference_config.build_identifier()}_SV_{eval_config.sv_model}" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve language in aggregate CSV identity.
full_checkpoint_name no longer contains the dataset language, but append_metrics_to_csv still writes it at Lines 349 and 376 while the CSV schema has no language column. Multilingual runs can therefore produce indistinguishable aggregate rows for the same checkpoint and dataset. Add a language column or pass a language-qualified checkpoint identifier to the aggregate CSV writer.
🤖 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 `@examples/tts/magpietts_inference.py` around lines 219 - 221, Update the
checkpoint identity flow around full_checkpoint_name and append_metrics_to_csv
so aggregate CSV rows preserve dataset language. Either add a language column to
the aggregate CSV schema and populate it at both writer call sites, or include
the language in the checkpoint identifier passed to the writer; ensure
multilingual runs remain distinguishable for the same checkpoint and dataset.
| sorted_filewise = sorted(filewise_metrics, key=lambda x: x.get('cer', 0), reverse=True) | ||
| with open(os.path.join(eval_dir, f"{dataset}_filewise_metrics_{repeat_idx}.json"), "w") as f: | ||
| json.dump(sorted_filewise, f, indent=4) | ||
| json.dump(sorted_filewise, f, indent=4, ensure_ascii=False) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Open the JSON file with an explicit UTF-8 encoding.
ensure_ascii=False writes Unicode characters directly, but the surrounding open(..., "w") uses the platform locale. On non-UTF-8 environments this can raise UnicodeEncodeError or corrupt the intended raw text.
Proposed fix
- with open(os.path.join(eval_dir, f"{dataset}_filewise_metrics_{repeat_idx}.json"), "w") as f:
+ with open(
+ os.path.join(eval_dir, f"{dataset}_filewise_metrics_{repeat_idx}.json"),
+ "w",
+ encoding="utf-8",
+ ) as f:
json.dump(sorted_filewise, f, indent=4, ensure_ascii=False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| json.dump(sorted_filewise, f, indent=4, ensure_ascii=False) | |
| with open( | |
| os.path.join(eval_dir, f"{dataset}_filewise_metrics_{repeat_idx}.json"), | |
| "w", | |
| encoding="utf-8", | |
| ) as f: | |
| json.dump(sorted_filewise, f, indent=4, ensure_ascii=False) |
🤖 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 `@examples/tts/magpietts_inference.py` at line 346, Update the JSON output file
opening in the code surrounding json.dump to explicitly use UTF-8 encoding,
while preserving ensure_ascii=False and the existing serialization behavior.
2 participants
What does this PR do ?
Update format of TTS comparison report
Collection: [TTS]
Changelog
Before your PR is "Ready for review"
Pre checks:
PR Type:
Summary by CodeRabbit
New Features
Bug Fixes