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

set default none decoder for ASR #4917

Merged
merged 16 commits into from Feb 10, 2023
Merged

set default none decoder for ASR #4917

merged 16 commits into from Feb 10, 2023

Conversation

ftshijt
Copy link
Collaborator

@ftshijt ftshijt commented Feb 6, 2023

Tries to fix the issues in #4908

@mergify mergify bot added the ESPnet2 label Feb 6, 2023
@ftshijt ftshijt added the Bugfix label Feb 6, 2023
@sw005320
Copy link
Contributor

sw005320 commented Feb 6, 2023

I added @pyf98 for this PR.

@sw005320 sw005320 added this to the v.202303 milestone Feb 7, 2023
@sw005320
Copy link
Contributor

sw005320 commented Feb 7, 2023

@pyf98, would it solve all issues?
I think making rnn as the default decoder sounds weird, and I think it is correct.

@sw005320
Copy link
Contributor

sw005320 commented Feb 7, 2023

Actually, there are a lot of CI errors, @ftshijt

@ftshijt
Copy link
Collaborator Author

ftshijt commented Feb 7, 2023

Actually, there are a lot of CI errors, @ftshijt

Yeah, sorry that I did not find time to fix the errors. Will try to fix those later

@pyf98
Copy link
Collaborator

pyf98 commented Feb 7, 2023

It can solve the issue when we run new experiments using prior CTC configs. However, for previously uploaded models, the issue still exists? For example, the decoder was set to rnn by default in this model: https://huggingface.co/pyf98/librispeech_100_ctc_e_branchformer/blob/main/exp/asr_train_asr_ctc_e_branchformer_e12_raw_en_bpe5000_sp/config.yaml#L5190

If we download and load it, the decoder will be set to RNN?

@sw005320
Copy link
Contributor

sw005320 commented Feb 7, 2023

It can solve the issue when we run new experiments using prior CTC configs. However, for previously uploaded models, the issue still exists? For example, the decoder was set to rnn by default in this model: https://huggingface.co/pyf98/librispeech_100_ctc_e_branchformer/blob/main/exp/asr_train_asr_ctc_e_branchformer_e12_raw_en_bpe5000_sp/config.yaml#L5190

If we download and load it, the decoder will be set to RNN?

Oh, this is complicated.
But in this case, if we use CTC weight for 1.0 during inference, would the result be the same?

@pyf98
Copy link
Collaborator

pyf98 commented Feb 7, 2023

I think it does not affect the final result. It just causes some redundant parameters in the model, which is not good. Also, it will cause an error for load_state_dict when strict=True.

@pyf98
Copy link
Collaborator

pyf98 commented Feb 7, 2023

I think we can change the default decoder to none, which is a good design for me.

But to keep compatibility, we still need to set the decoder as None in the model code when ctc weight is 1.0.

@sw005320
Copy link
Contributor

sw005320 commented Feb 7, 2023

I see. Good idea.
@ftshijt, can you test it?

@ftshijt
Copy link
Collaborator Author

ftshijt commented Feb 8, 2023

Sure, I agree with Yifan on the potential behaviors. I think the only point here is the initialization with strict loading checks. Do you guys know some of the available pre-trained models with only CTC?

@sw005320
Copy link
Contributor

sw005320 commented Feb 8, 2023

How about this?
https://github.com/espnet/espnet/blob/master/egs2/librispeech_100/asr1/README.md#e-branchformer-with-ctc

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #4917 (8b93196) into master (1564610) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #4917      +/-   ##
==========================================
- Coverage   76.56%   76.56%   -0.01%     
==========================================
  Files         603      603              
  Lines       53753    53756       +3     
==========================================
  Hits        41157    41157              
- Misses      12596    12599       +3     
Flag Coverage Δ
test_integration_espnet1 66.33% <ø> (ø)
test_integration_espnet2 47.47% <33.33%> (-0.11%) ⬇️
test_python 66.44% <33.33%> (-0.01%) ⬇️
test_utils 23.35% <ø> (ø)

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

Impacted Files Coverage Δ
espnet2/asr/pit_espnet_model.py 90.26% <ø> (ø)
espnet2/tasks/asr.py 90.72% <ø> (ø)
espnet2/asr/espnet_model.py 76.83% <33.33%> (-0.51%) ⬇️
espnet2/tts/espnet_model.py 59.45% <0.00%> (-0.91%) ⬇️

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

@mergify mergify bot added the CI Travis, Circle CI, etc label Feb 9, 2023
@sw005320
Copy link
Contributor

sw005320 commented Feb 9, 2023

@ftshijt, I just want to make sure whether it is working with existing pre-trained CTC models without changing the configurations.
Did you confirm it? (I just want to double-check)

@pyf98
Copy link
Collaborator

pyf98 commented Feb 9, 2023

I'm also curious. I don't see the related modifications in the code. How is it solved? Am I missing anything?

@ftshijt
Copy link
Collaborator Author

ftshijt commented Feb 9, 2023

I'm also curious. I don't see the related modifications in the code. How is it solved? Am I missing anything?

Sorry, my bad. I did not push that to remote. The current one should prevent issues when loading previous checkpoints that are trained with ctc only.

@ftshijt
Copy link
Collaborator Author

ftshijt commented Feb 9, 2023

(I checked with the ebranchformer checkpoint on mini_an4)

@ftshijt
Copy link
Collaborator Author

ftshijt commented Feb 10, 2023

This PR is ready to merge

@sw005320
Copy link
Contributor

LGTM.
@pyf98, can you also check it?

@pyf98
Copy link
Collaborator

pyf98 commented Feb 10, 2023

looks good!

@sw005320 sw005320 merged commit 6f0983f into master Feb 10, 2023
@sw005320 sw005320 deleted the ftshijt-patch-1 branch February 10, 2023 16:32
@sw005320
Copy link
Contributor

Thanks, @ftshijt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix CI Travis, Circle CI, etc ESPnet2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants