Asr bleu
State: closed | Pull request: yes
Labels: name: stale

name: ASR

# What does this PR do ?
- Adds BLEU metric for ASR collections.

**Collection**: [Note which collection this PR will affect]
ASR 

# Changelog 
- Adds BLEU metric for ASR collections
- Adds TransformerDecoding module to make transformer_bpe similar to other ASR modules
- Implements BLEU module in transformer_bpe for demonstration


# Jenkins CI
To run Jenkins, a NeMo User with write access must comment `jenkins` on the PR.

# Before your PR is "Ready for review"
**Pre checks**:
- [N ] Make sure you read and followed [Contributor guidelines](https://github.com/NVIDIA/NeMo/blob/main/CONTRIBUTING.md)
- [ N] Did you write any new necessary tests?
- [ N] Did you add or update any necessary documentation?
- [ N] 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**:
- [ Y] 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](https://github.com/NVIDIA/NeMo/blob/main/CONTRIBUTING.md) contains specific people who can review PRs to various areas.

# Additional Information
* Related to # (issue)


@titu1994 Draft PR to show BLEU calls.  Everywhere that the model calls BLEU is where we normally call WER, so can be aliased. The `multi_validation_epoch_end` just needs to be pushed up to `asr_model` (it's an ugly check but is same logic as how it calls WER and can be model agnostic that way.) 

If we do WER updates like BLEU updates here, all metrics now on can be updated in individual models agnostic of task. (Checked with Torch folks, it doesn't lead to any complications for tensor devices.)

(Since it's a draft I just based it off wer_fix, so large amount of file changes are unimportant. Just `metrics.bleu` and `models.transformer_bpe`)

Hmm it's a bit hard to understand what are the changes here. Let's hold ofd on this until we merge the first one 

@titu1994 post wer draft is up. pinging you to check if there's any design/technical problems before I make it more comprehensive throughout.

Main changes:
- makes BLEU metric
- bleu metric operates like wer metric throughout asr module, so no change needed beyond a flag in config
- made wer and bleu give dict outputs to clean up asr models (will make more comprehensive for the rest of the asr collection metrics.) Checked with PyTorch channel and they say there should be no syncing problems. Running my own tests off this branch later in week to confirm.

Forgot about this, sorry. Will review next week 

> Forgot about this, sorry. Will review next week

All good, I can keep on backburner for a bit. Don't need a full on review, just want to catch if there's any design issues before I go more thoroughly through the codebase to unify metrics.

@titu1994 i'm gonna finish up work on this over the week to avoid canary conflicts. Did you have a chance to review before I have to tear up some more of the codebase?

This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days.

Closing since implemented in https://github.com/NVIDIA/NeMo/pull/8363