add ssl feature extractor #881
Conversation
lhotse/features/ssl.py
Outdated
| assert ( | ||
| sampling_rate == 16000 | ||
| ), f"All the upstream models in S3PRL now only support 16 kHz audio." | ||
| feature_dim = 1024 |
There was a problem hiding this comment.
You might want to move feature_dim, sampling_rate, and frame_shift to the config to let people override it for different models (even if today they all use the same setting)
|
Shall we add tests for it? |
|
I'm afraid that it will require downloading a big pretrained model from somewhere, which won't be very convenient either for CI or local testing. WDYT? |
|
In icefall we upload the pretrained models to huggingface and download them in CI for testing using git lfs. A file an be several hundred MB and it's very fast to download in the CI. |
|
Cool! @DongjiGao could you contribute a test for it? You can use a decorator on the test function to make the test optional. Then add Decorator example: |
Sure. |
|
@DongjiGao I'm wondering if perhaps you could add a comment somewhere (maybe as a docstring for the class), with a link to where users could find other pretrained models (and their configs like frame_shift, feature_dim, etc.)? |
test/features/test_s3prl.py
Outdated
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| not is_module_available("s3prl.hub"), reason="The test requires s3prl to run." |
There was a problem hiding this comment.
| not is_module_available("s3prl.hub"), reason="The test requires s3prl to run." | |
| not is_module_available("s3prl"), reason="The test requires s3prl to run." |
test/features/test_s3prl.py
Outdated
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| not is_module_available("s3prl.hub"), reason="The test requires s3prl to run." |
There was a problem hiding this comment.
| not is_module_available("s3prl.hub"), reason="The test requires s3prl to run." | |
| not is_module_available("s3prl"), reason="The test requires s3prl to run." |
|
Also please remember about adding s3prl installation to the CI |
…ig, small fix of s3prl test script
Adding it now. |
Sorry I was using a personal token previously and was blocked to change that file. I have added the s3prl installation now. |
|
@pzelasko @csukuangfj @desh2608 Do you think the code is good to check in? |
|
LGTM. |
The SSL extractor is supported by S3PRL