[TTS] Update multi-language eval report format#15915
Conversation
Signed-off-by: Ryan <rlangman@nvidia.com>
|
[RESOLVED] 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.
Signed-off-by: Ryan <rlangman@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Ryan <rlangman@nvidia.com>
| jira_url = f"{JIRA_TICKET_URL_PREFIX}/{jira_id}" | ||
| task_url = f"{TICKET_URL_PREFIX}/{task_id}" | ||
|
|
||
| return TaskInfo( |
There was a problem hiding this comment.
The previous workflow was:
task_idwas the internal task identifier. If not specified, it defaulted toDUMMY_TASK_ID. It was used in the S3 report name and this naming is important for maintenance: it allows us to remove reports for old tasks and to deprioritize reports without explicit task IDs.jira_idwas derived fromtask_idand used only for display in the report sidebar. IfDUMMY_TASK_IDwas used,jira_idbecameNEMOTTS, so the report still showed our Jira space.jira_urlwas derived fromJIRA_TICKET_URL_PREFIXandjira_id, so it always pointed to a valid page: either the real Jira ticket or, when no task ID was provided, the Jira project page.
Currently, task_id is being used for all three purposes:
- S3 report naming,
- displayed task ID in the report,
- and construction of
task_url.
As a result, when task_id is not specified, the report shows DUMMY_TASK_ID and generates a broken POR link.
If we want to keep task_id optional, the previous behavior should be preserved. In other words, we should keep separate values for:
- the identifier used in S3 naming (actual POR ID or
NMP-I-000), - the identifier displayed in the report (actual POR ID or something like
NMP-IorNMP), - and the clickable URL (real POR ticket URL or the default
TICKET_URL_PREFIX).
Sorry for the earlier misunderstanding.
4 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