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

raise more useful error in espnet2/asr/frontend/s3prl.py if s3prl is not installed #4480

Merged
merged 3 commits into from
Jul 10, 2022

Conversation

popcornell
Copy link
Contributor

No description provided.

@mergify mergify bot added the ESPnet2 label Jun 30, 2022
@@ -63,7 +63,8 @@ def _get_upstream(self, frontend_conf):
if p.endswith("s3prl"):
s3prl_path = p
break
assert s3prl_path is not None
assert s3prl_path is not None, "Cannot find s3prl in your PYTHONPATH, " \
"you can install it via pip install s3prl"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you can refer the installer in tools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right. s3prl_path should be ${YOUR_PATH2ESPNet}/espnet/tools/s3prl.
And the code above can find it rightly when you source path.sh in an ESPNet recipe.
However it cannot do it when you just pip install espnet-model-zoo and try to use the pretrained model without espnet or s3prl. We need to devise a method to point out to a s3prl git cloned directory (pip package won't work I tried).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means that espnet model zoo models that rely on s3prl front-ends are broken outside recipes

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. I think we need to deal with this more smart way for easy inference.
@simpleoier, can you consider it?

At this moment, we can just add meaningful error messages through an installer and path.sh
@popcornell and @ftshijt, can you do it?

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 ll handle the error message right now.
Then raise an issue related to this so it can be fixed in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code now works without installing espnet (only model-zoo) but you need to git clone s3prl locally and install it from the repo. I guess it is a reasonable compromise.

@sw005320 sw005320 added the SSL self-supervised learning label Jul 10, 2022
@sw005320 sw005320 added this to the v.202207 milestone Jul 10, 2022
@codecov
Copy link

codecov bot commented Jul 10, 2022

Codecov Report

Merging #4480 (e7ae12d) into master (d837c97) will decrease coverage by 0.00%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #4480      +/-   ##
==========================================
- Coverage   82.41%   82.40%   -0.01%     
==========================================
  Files         481      481              
  Lines       41237    41238       +1     
==========================================
- Hits        33984    33982       -2     
- Misses       7253     7256       +3     
Flag Coverage Δ
test_integration_espnet1 66.38% <ø> (ø)
test_integration_espnet2 49.17% <12.50%> (+<0.01%) ⬆️
test_python 69.40% <62.50%> (-0.01%) ⬇️
test_utils 23.30% <ø> (ø)

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

Impacted Files Coverage Δ
espnet2/asr/frontend/s3prl.py 81.57% <62.50%> (-3.76%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@sw005320 sw005320 added the auto-merge Enable auto-merge label Jul 10, 2022
@mergify mergify bot merged commit f2778f7 into espnet:master Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Enable auto-merge ESPnet2 SSL self-supervised learning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants