-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ASR evaluator #5728
ASR evaluator #5728
Conversation
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <36722593+fayejf@users.noreply.github.com>
for more information, see https://pre-commit.ci
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, just a few small comments. We should make sure that scripts inside nemo.collection
do not depend on scripts in examples/
and functions in NeMo be language-agnostic.
@@ -182,6 +186,9 @@ def transcribe( | |||
'channel_selector': channel_selector, | |||
} | |||
|
|||
if augmentor_config: | |||
config['augmentor_config'] = augmentor_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'augmentor_config'
can be simplified to just 'augmentor'
, since it's part of the config
variable we can drop the _config
suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense. updated.
if (cfg.model_path and cfg.pretrained_name) or (not cfg.model_path and not cfg.pretrained_name): | ||
raise ValueError("Please specify either cfg.model_path or cfg.pretrained_name!") | ||
|
||
if cfg.inference_mode.mode == "offline": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cfg.inference_mode.mode
seems a bit redundant, can we change to just cfg.inference.mode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
f"model_stride={cfg.inference_mode.model_stride} ", | ||
shell=True, | ||
check=True, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unsafe for things inside NeMo collections to depend on NeMo example scripts, since examples may not be installed or can be lost/modified easily. If this function has to use something from the example folder, it's better to move the whole function to an example scripts other than putting it in NeMo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points! Moved eval_utils.py to utils.py in tools/asr_evaluator
f"eval_config_yaml={temp_eval_config_yaml_file} ", | ||
shell=True, | ||
check=True, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, It's unsafe for things inside NeMo collections to depend on NeMo example scripts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
cfg.output_filename = model_name + "-" + dataset_name + "-" + mode_name + ".json" | ||
|
||
temp_eval_config_yaml_file = "temp_eval_config.yaml" | ||
with open(temp_eval_config_yaml_file, "w") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider (but not mandatory) using tempfile.NamedTemporaryFile
, which automatically manages temp files:
import tempfile
with tempfile.NamedTemporaryFile(mode='w', encoding='utf-8') as f:
OmegaConf.save(cfg, f)
f.seek(0) # reset file pointer
subprocess.run(
xxxxxxx
eval_config_yaml=f.name
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea! updated
return cfg, total_res | ||
|
||
|
||
def target_metadata_wer(manifest: str, target: str, meta_cfg: DictConfig, eval_metric: str = "wer",) -> dict: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe cal_target_metadata_wer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
|
||
words = sample["words"] | ||
wer_each_class[target_class]["words"] += words | ||
wer_each_class[target_class]["errors"] += words * sample[eval_metric] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to check whether the input eval_metric
is supported, and raise exceptions if it's not valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
logging.info(f"metadata '{target}' does not present in manifest. Skipping! ") | ||
return None | ||
|
||
values = ['samples', 'words', 'errors', 'inss', 'dels', 'subs'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some doctrings on each value to explain what each one means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in function docstring
tools/asr_evaluator/asr_evaluator.py
Outdated
logging.info(f'Hydra config: {OmegaConf.to_yaml(cfg)}') | ||
|
||
# Set and save random seed and git hash for reproducibility | ||
random.seed(cfg.env.random_seed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about numpy and pytorch random seeds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated. Need to fix rng in perturb in another PR.
) | ||
cfg = run_chunked_inference(cfg) | ||
|
||
elif cfg.inference_mode.mode == "offline_by_chunked": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the fundamental difference between "chunked" and "offline_by_chunked"? It seems that the only difference is only that "offline_by_chunked" will set default parameters while "chunked" will not. Could you please add more docstrings to explain the different use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what you said is totally correct. Added more explanation in docstring.
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
Signed-off-by: fayejf <fayejf07@gmail.com>
tools/asr_evaluator/README.md
Outdated
- **ENGINE**. To conduct ASR inference. | ||
- **ANALYST**. To evaluate model performance based on predictions. | ||
|
||
In Analyst, you can evaluate on all metadata if it presents in manifest. For exmaple, you can evaluate the peformance of model based on duration of each sample, such as how's the model peforms on samples smaller than 5s and longer than 5s by [[0,5][5,100000]] and get wer/cer of each slot. Or how's the model performs on postive (happy, laugh) or neural (neural) or negative mood (sad) as below. And if you set save_wer_per_class=True, it will calculate wer for all (i.e. above 5 classes + cry) classes presented in the data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean "neutral"? The meanings for metadata needs more explanation. For example, we can say that, with the following config, we can calculate WERs for audios in different interval groups, where each group (in seconds) is defined by [[0,2],[2,5],[5,10],[10,20],[20,100000]]
. Also, we calculate the WERs for three groups of emotions, where each group is defined by [['happy','laugh'],['neural'],['sad']]
(words in the same group will be treated as the same).
tools/asr_evaluator/README.md
Outdated
|
||
emotion: | ||
enable: True | ||
slot: [['happy','laugh'],['neural'],['sad']] # we could have 'cry' in data but not in slot we focus on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean "neutral"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
* backbone Signed-off-by: fayejf <fayejf07@gmail.com> * engineer and analyzer Signed-off-by: fayejf <fayejf07@gmail.com> * offline_by_chunked Signed-off-by: fayejf <fayejf07@gmail.com> * test_ds wip Signed-off-by: fayejf <fayejf07@gmail.com> * temp remove inference Signed-off-by: fayejf <fayejf07@gmail.com> * mandarin yaml Signed-off-by: fayejf <fayejf07@gmail.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * augmentor and a few updates Signed-off-by: fayejf <fayejf07@gmail.com> * address alerts and revert unnecessary changes Signed-off-by: fayejf <fayejf07@gmail.com> * Add readme Signed-off-by: fayejf <fayejf07@gmail.com> * rename Signed-off-by: fayejf <fayejf07@gmail.com> * typo fix Signed-off-by: fayejf <fayejf07@gmail.com> * small fix Signed-off-by: fayejf <fayejf07@gmail.com> * add missing header Signed-off-by: fayejf <fayejf07@gmail.com> * rename augmentor_config to augmentor Signed-off-by: fayejf <fayejf07@gmail.com> * raname inference_mode to inference Signed-off-by: fayejf <fayejf07@gmail.com> * move utils.py Signed-off-by: fayejf <fayejf07@gmail.com> * update temp file Signed-off-by: fayejf <fayejf07@gmail.com> * make wer cer clear Signed-off-by: fayejf <fayejf07@gmail.com> * seed_everything Signed-off-by: fayejf <fayejf07@gmail.com> * fix missing rn augmentor_config in rnnt Signed-off-by: fayejf <fayejf07@gmail.com> * fix rnnt transcribe Signed-off-by: fayejf <fayejf07@gmail.com> * add more docstring and style fix Signed-off-by: fayejf <fayejf07@gmail.com> * address codeQL Signed-off-by: fayejf <fayejf07@gmail.com> * reflect comments Signed-off-by: fayejf <fayejf07@gmail.com> * update readme Signed-off-by: fayejf <fayejf07@gmail.com> * clearer Signed-off-by: fayejf <fayejf07@gmail.com> Signed-off-by: fayejf <fayejf07@gmail.com> Signed-off-by: fayejf <36722593+fayejf@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: He Huang (Steve) <105218074+stevehuang52@users.noreply.github.com> Signed-off-by: Elena Rastorgueva <erastorgueva@nvidia.com>
* backbone Signed-off-by: fayejf <fayejf07@gmail.com> * engineer and analyzer Signed-off-by: fayejf <fayejf07@gmail.com> * offline_by_chunked Signed-off-by: fayejf <fayejf07@gmail.com> * test_ds wip Signed-off-by: fayejf <fayejf07@gmail.com> * temp remove inference Signed-off-by: fayejf <fayejf07@gmail.com> * mandarin yaml Signed-off-by: fayejf <fayejf07@gmail.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * augmentor and a few updates Signed-off-by: fayejf <fayejf07@gmail.com> * address alerts and revert unnecessary changes Signed-off-by: fayejf <fayejf07@gmail.com> * Add readme Signed-off-by: fayejf <fayejf07@gmail.com> * rename Signed-off-by: fayejf <fayejf07@gmail.com> * typo fix Signed-off-by: fayejf <fayejf07@gmail.com> * small fix Signed-off-by: fayejf <fayejf07@gmail.com> * add missing header Signed-off-by: fayejf <fayejf07@gmail.com> * rename augmentor_config to augmentor Signed-off-by: fayejf <fayejf07@gmail.com> * raname inference_mode to inference Signed-off-by: fayejf <fayejf07@gmail.com> * move utils.py Signed-off-by: fayejf <fayejf07@gmail.com> * update temp file Signed-off-by: fayejf <fayejf07@gmail.com> * make wer cer clear Signed-off-by: fayejf <fayejf07@gmail.com> * seed_everything Signed-off-by: fayejf <fayejf07@gmail.com> * fix missing rn augmentor_config in rnnt Signed-off-by: fayejf <fayejf07@gmail.com> * fix rnnt transcribe Signed-off-by: fayejf <fayejf07@gmail.com> * add more docstring and style fix Signed-off-by: fayejf <fayejf07@gmail.com> * address codeQL Signed-off-by: fayejf <fayejf07@gmail.com> * reflect comments Signed-off-by: fayejf <fayejf07@gmail.com> * update readme Signed-off-by: fayejf <fayejf07@gmail.com> * clearer Signed-off-by: fayejf <fayejf07@gmail.com> Signed-off-by: fayejf <fayejf07@gmail.com> Signed-off-by: fayejf <36722593+fayejf@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: He Huang (Steve) <105218074+stevehuang52@users.noreply.github.com>
What does this PR do ?
A tool to thoroughly evaluate an ASR model.
Collection: ASR
Changelog
Usage
Before your PR is "Ready for review"
Pre checks:
PR Type:
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
Note because the PR is getting bigger, on-the-fly augmentation for buffered inference mode would be included in another PR soon.