Skip to content
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

CHiME-7 DASR adding evaluation inference + adding support to use diarization baseline "pre-computed" JSONs #5183

Closed
wants to merge 42 commits into from

Conversation

popcornell
Copy link
Contributor

@Emrys365 does this fix it ?

@mergify mergify bot added the ESPnet2 label May 24, 2023
@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #5183 (fc344e8) into master (0216f85) will increase coverage by 8.60%.
The diff coverage is n/a.

❗ Current head fc344e8 differs from pull request most recent head afeecc1. Consider uploading reports for the commit afeecc1 to get more accurate results

@@            Coverage Diff             @@
##           master    #5183      +/-   ##
==========================================
+ Coverage   65.94%   74.54%   +8.60%     
==========================================
  Files         640      640              
  Lines       57267    57267              
==========================================
+ Hits        37762    42688    +4926     
+ Misses      19505    14579    -4926     
Flag Coverage Δ
test_integration_espnet1 66.28% <ø> (?)
test_integration_espnet2 47.58% <ø> (?)
test_python 65.28% <ø> (ø)
test_utils 23.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 107 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Emrys365
Copy link
Collaborator

I think we can ignore this issue. Because the default value of use_chime6_falign in egs2/chime7_task1/asr1/run.sh is 0. So the CHiME7_DASR_falign will not be used.

But probably some descriptions should be added to mention when we should set this argument to 1.

@mergify mergify bot added README CI Travis, Circle CI, etc Installation labels May 28, 2023
@sw005320 sw005320 added this to the v.202307 milestone May 30, 2023
@sw005320 sw005320 requested a review from simpleoier May 30, 2023 14:17
@popcornell popcornell changed the title Prevent using forced alignment annotation to train the ASR model CHiME-7 DASR adding evaluation inference + adding support to use diarization baseline "pre-computed" JSONs May 30, 2023
@popcornell
Copy link
Contributor Author

FYI changed the scope of this PR to be more broad.

  1. adds support for evaluation set inference by fixing some minor things.
  2. adds support for using pre-computed baseline diarization output JSONs

@popcornell popcornell marked this pull request as draft May 30, 2023 14:27
Copy link
Collaborator

@simpleoier simpleoier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments in the code, which may cause some issues. But I'm not 100% sure myself.

@@ -51,7 +52,7 @@ gss_max_batch_dur=90

# ASR config
use_pretrained=
decode_only=1
decode_only=
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it is empty, I'm afraid line 182 will raise an error.

--decode_only $decode_only --gss-max-batch-dur $gss_max_batch_dur \

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add quotes, good catch

@@ -166,5 +181,5 @@ if [ ${stage} -le 4 ] && [ $stop_stage -ge 4 ]; then
--use-pretrained $use_pretrained \
--decode_only $decode_only --gss-max-batch-dur $gss_max_batch_dur \
--gss-iterations 5 \
--diar-score 1
--diar-score 1 \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base on my previous experience, this extra \ will raise an error. You need an extra blank line.

log "Using organizer-provided JSON manifests from the baseline diarization system."
#git clone
#mv
stage=4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to double check if we can skip stage 3 if we have this. How can we download baseline diarization results?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you are right I changed it.
Now I also added the downloading part

@@ -147,14 +147,17 @@ You can also play with diarization hyperparameters such as:

as said merge-closer can have quite an impact on the final WER.

**NOTE**
We found the diarization baseline to be highly sensitive to the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a complete note?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completed

@@ -67,7 +67,7 @@ nbpe=500
asr_max_epochs=8
# put popcornell/chime7_task1_asr1_baseline if you want to test with pretrained model
use_pretrained=
decode_only=0
decode_only=
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that when it is empty, line 206 data_opts+=" --decode-only $decode_only" will raise some error, e.g. Error: No positional arguments are required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now it is an empty string.

@popcornell popcornell marked this pull request as ready for review June 1, 2023 09:30
@popcornell
Copy link
Contributor Author

I think this PR is ready.
@simpleoier I have addressed your comments

@popcornell
Copy link
Contributor Author

FYI: there is still the numpy problem:

ERROR: Could not install packages due to an OSError: [Errno 2] No such file or directory: '/Users/runner/work/espnet/espnet/tools/venv/envs/espnet/lib/python3.10/site-packages/numpy-1.24.3.dist-info/METADATA'

@popcornell
Copy link
Contributor Author

@simpleoier Can we merge this ?

Copy link
Collaborator

@simpleoier simpleoier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
But there are many changes in non-relevant files. Probably due to black version. Also, @popcornell, there is one failure test: check_kaldi_symlinks, can you solve it?
@sw005320 Can we merge this?

@popcornell
Copy link
Contributor Author

I can rechange them back.
What black version are you using btw ?

@simpleoier
Copy link
Collaborator

@popcornell my black is black, 23.3.0 (compiled: yes)
BTW, here are the requirement for espnet test tools.

        "pytest>=3.3.0",
        "pytest-timeouts>=1.2.1",
        "pytest-pythonpath>=0.7.3",
        "pytest-cov>=2.7.1",
        "hacking>=2.0.0",
        "mock>=2.0.0",
        "pycodestyle",
        "jsondiff<2.0.0,>=1.2.0",
        "flake8>=3.7.8",
        "flake8-docstrings>=1.3.1",
        "black",
        "isort",

@popcornell
Copy link
Contributor Author

I guess then it is fine ?! Like the black version is not enforced explicitly.
Maybe we should fix it however and upgrade it explicitly by one PR once in a while (with explicit reformatting of everything).
I think these questions can be answered only by @sw005320

@Emrys365
Copy link
Collaborator

Emrys365 commented Jun 7, 2023

The black formatting is now always enforced in https://github.com/espnet/espnet/blob/master/.pre-commit-config.yaml#L16-L20, which will be done every time you push a new commit.

@sw005320
Copy link
Contributor

sw005320 commented Jun 8, 2023

Yes, this part is not an issue.
However, CI says that egs2/TEMPLATE/asr1/utils/data/extend_segment_times.py and tools/kaldi/egs/wsj/s5/utils/data/extend_segment_times.py are different.
https://github.com/espnet/espnet/actions/runs/5192602992/jobs/9361995304?pr=5183

If they are necessary changes, it would be better to move this file to the local (recipe-specific) directory.
If their changes are due to the format only, it would be better to change it.

The issue is that they look very different due to the different formats, and I could not figure out the above changes.

@popcornell
Copy link
Contributor Author

Made a fresh PR because it is faster. Moved to #5228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants