Fix Canary not stripping prompt from reference + more test coverage #9987

pzelasko · 2024-07-31T17:23:14Z

What does this PR do ?

TL;DR: fix for a bug I accidentally introduced in #9901

Context:

Fix for scoring when Canary outputs a lot of endoftext tokens. #9901 changed multi-task beam search decoding module to strip the prompt and EOS away form recognitions; it removed "remove_special_tokens" sprinkled in various places in our codebase (transcribe, wer, bleu, etc.)
That fix was actually porting a part of my changes in an old closed PR Natural language prompting for Canary #8833. However, I forgot that we also have to handle the dataset part properly - return GT transcripts with stripped prompts and EOS (which was done there). So I ended up introduced a bug that reference has the prompt, but recognition doesn't in scoring.
I ported the necessary additional changes from Natural language prompting for Canary #8833 and adapted them to the current codebase.
Since I wanted to improve the test coverage of the affected parts, I also pulled the changes from Canary greedy and temperature decoding #8885 (another PR I forgot to merge) where I already wrote these tests, and extended them.

Collection: ASR

Changelog

Add specific line by line info of high level changes in this PR.

Usage

You can potentially add a usage example below

# Add a code snippet demonstrating how to use this

GitHub Actions CI

The Jenkins CI system has been replaced by GitHub Actions self-hosted runners.

The GitHub Actions CI will run automatically when the "Run CICD" label is added to the PR.
To re-run CI remove and add the label again.
To run CI on an untrusted fork, a NeMo user with write access must first click "Approve and run".

Before your PR is "Ready for review"

Pre checks:

Make sure you read and followed Contributor guidelines
Did you write any new necessary tests?
Did you add or update any necessary documentation?
Does the PR affect components that are optional to install? (Ex: Numba, Pynini, Apex etc)
- Reviewer: Does the PR have correct import guards for all optional libraries?

PR Type:

New Feature
Bugfix
Documentation

If you haven't finished some of the above items you can still open "Draft" PR.

Who can review?

Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.

Additional Information

Related to # (issue)

… coverage Signed-off-by: Piotr Żelasko <petezor@gmail.com>

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

nemo/collections/asr/parts/submodules/multitask_greedy_decoding.py

nemo/collections/asr/parts/submodules/multitask_decoding.py

bonham79

Left comments, also for sanity checking, can you run a toy model against a stable release and this branch just to ensure nothing pops up?

nemo/collections/asr/data/audio_to_text_lhotse_prompted.py

nemo/collections/asr/models/aed_multitask_models.py

nemo/collections/asr/data/audio_to_text_lhotse_prompted.py

nemo/collections/asr/modules/transformer/transformer_generators.py

nemo/collections/asr/parts/submodules/token_classifier.py

nemo/collections/common/parts/multi_layer_perceptron.py

tests/collections/asr/decoding/test_multi_task_decoding.py

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

nemo/collections/asr/parts/submodules/token_classifier.py

 from torch import nn as nn

 from nemo.collections.asr.parts.submodules.classifier import Classifier
 from nemo.collections.common.parts import MultiLayerPerceptron
 from nemo.core.classes import typecheck
-from nemo.core.neural_types import LogitsType, LogprobsType, NeuralType
+from nemo.core.neural_types import ChannelType, FloatType, LogitsType, LogprobsType, NeuralType


tbartley94 · 2024-07-31T19:27:49Z

nemo/collections/asr/modules/transformer/transformer_generators.py

@@ -75,6 +85,7 @@ def _one_step_forward(
        encoder_input_mask=None,
        decoder_mems_list=None,
        pos=0,
+        need_scores: bool = True,


Can we change the variable to return_scores to be consistent with other commands like this?

tbartley94

One name change, fix import flags from the CI.

Btw, how are you feeding temperature to the MLP now?

pzelasko · 2024-07-31T20:02:01Z

One name change, fix import flags from the CI.

OK

Btw, how are you feeding temperature to the MLP now?

It's applied directly in Categorical

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

…VIDIA#9987) * Fix not stripping Canary prompt from the reference and add extra test coverage Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix transcripts for Canary containing EOS Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix validation_pass Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Review Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Revie 2 Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix non-deterministic unit test assertions Signed-off-by: Piotr Żelasko <petezor@gmail.com> --------- Signed-off-by: Piotr Żelasko <petezor@gmail.com> Signed-off-by: adityavavre <aditya.vavre@gmail.com>

…9987) * Fix not stripping Canary prompt from the reference and add extra test coverage Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix transcripts for Canary containing EOS Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix validation_pass Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Review Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Revie 2 Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix non-deterministic unit test assertions Signed-off-by: Piotr Żelasko <petezor@gmail.com> --------- Signed-off-by: Piotr Żelasko <petezor@gmail.com>

…VIDIA#9987) * Fix not stripping Canary prompt from the reference and add extra test coverage Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix transcripts for Canary containing EOS Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix validation_pass Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Review Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Revie 2 Signed-off-by: Piotr Żelasko <petezor@gmail.com> * Fix non-deterministic unit test assertions Signed-off-by: Piotr Żelasko <petezor@gmail.com> --------- Signed-off-by: Piotr Żelasko <petezor@gmail.com> Signed-off-by: Hainan Xu <hainanx@nvidia.com>

pzelasko added 3 commits July 31, 2024 12:46

Fix not stripping Canary prompt from the reference and add extra test…

faf3730

… coverage Signed-off-by: Piotr Żelasko <petezor@gmail.com>

Fix transcripts for Canary containing EOS

5b0d0b6

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

Fix validation_pass

176603e

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

pzelasko requested a review from tbartley94 July 31, 2024 17:23

github-actions bot added ASR common labels Jul 31, 2024

pzelasko added Run CICD and removed ASR common labels Jul 31, 2024

github-advanced-security bot found potential problems Jul 31, 2024

View reviewed changes

nemo/collections/asr/parts/submodules/multitask_greedy_decoding.py Dismissed Show dismissed Hide dismissed

nemo/collections/asr/parts/submodules/multitask_decoding.py Dismissed Show dismissed Hide dismissed

bonham79 reviewed Jul 31, 2024

View reviewed changes

github-actions bot added the ASR label Jul 31, 2024

Review

3008378

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

pzelasko force-pushed the fix-canary-validation branch from ac30118 to 3008378 Compare July 31, 2024 19:16

github-advanced-security bot found potential problems Jul 31, 2024

View reviewed changes

tbartley94 reviewed Jul 31, 2024

View reviewed changes

tbartley94 previously approved these changes Jul 31, 2024

View reviewed changes

Revie 2

e4a7fd0

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

pzelasko dismissed tbartley94’s stale review via e4a7fd0 July 31, 2024 20:02

pzelasko added Run CICD and removed Run CICD labels Jul 31, 2024

Fix non-deterministic unit test assertions

51a40b0

Signed-off-by: Piotr Żelasko <petezor@gmail.com>

pzelasko added Run CICD and removed Run CICD labels Jul 31, 2024

tbartley94 approved these changes Jul 31, 2024

View reviewed changes

pzelasko merged commit 876c851 into main Aug 1, 2024
223 checks passed

pzelasko deleted the fix-canary-validation branch August 1, 2024 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Canary not stripping prompt from reference + more test coverage #9987

Fix Canary not stripping prompt from reference + more test coverage #9987

pzelasko commented Jul 31, 2024

bonham79 left a comment

tbartley94 Jul 31, 2024

tbartley94 left a comment

pzelasko commented Jul 31, 2024

Fix Canary not stripping prompt from reference + more test coverage #9987

Fix Canary not stripping prompt from reference + more test coverage #9987

Conversation

pzelasko commented Jul 31, 2024

What does this PR do ?

Changelog

Usage

GitHub Actions CI

Before your PR is "Ready for review"

Who can review?

Additional Information

bonham79 left a comment

Choose a reason for hiding this comment

tbartley94 Jul 31, 2024

Choose a reason for hiding this comment

tbartley94 left a comment

Choose a reason for hiding this comment

pzelasko commented Jul 31, 2024