-
Notifications
You must be signed in to change notification settings - Fork 225
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
AudioCache: caching for "command" type of audio files #1050
Conversation
Thanks! The code looks good to me but I'm wondering if we should really merge it. We already have two mechanisms that can be used instead:
Is it possible to achieve your goal by using either of those? If not, can you elaborate? |
Hi Piotr, I tried to use the dynamic_lru_cache, the process takes some 25GB of memory, but apparently it is calling the same audio loading "command" again and again for all its segments, and the progress is slow. What could work is to put lru_cache on this And recording.move_to_memory() and cut.move_to_memory() method does not seem to be a solution. The I import the data folder with these steps:
Example record from feature manifest is here:
My PR made the runtime of What would you suggest ? |
I played with it a bit more and tried the This is what i tried (created a
|
I checked that script -- in general it has another issue, it's not using dataloader with background workers to load the audio, so it's bound to be slow. But I understand how the cache could help here. I am OK to merge but I'd like to change the following:
WDYT?
It wouldn't work because every time you enter this function, you are creating a new closure with a new cache. You'd need a global cached function instead. |
Okay, i'll continue working on it. I'll have a longer weekend, so I'll continue on wednesday. |
Hi Piotr,
And the code of And new unit tests were created. Should I also remove the old unit tests? Cheers, |
- reduces overhead when reading lots of segments from 1 audio file - cache-size is limited to 500MB or no more than 100 files - by default it is enabled - thread-safety is secured by threading.Lock Tested by running `./pruned_transducer_stateless7_streaming/streaming_decode.py` with dataset imported from kaldi format (locally). Question to @pzelasko : should the cache be used also for 'url' type of audio files ?
74bd0e5
to
0c29ac1
Compare
- disabled by default (decided not to introduce env variable) - inter-connecting `AudioCache` with `caching.py`, so `set_caching_enabled()` activates/deactivates AudioCache - cache is freed upon disabling - removed @dynamic_lru_cache from `audio.py` (in `features/io.py` it is not removed) The code of `AudioCache` code was moved to `caching.py`. And the new unit tests were created. Should I also remove the old unit tests? `test_audio_caching_disabled_works` `test_audio_caching_enabled_works` Cheers, Karel
0c29ac1
to
f3af054
Compare
lhotse/audio.py
Outdated
@@ -210,6 +216,7 @@ def load_audio( | |||
"Expect large I/O overhead if you are going to read many chunks like these, " | |||
"since every time we will download the whole file rather than its subset." | |||
) | |||
# Should AudioCache be used also for 'url' type ? (url never contains a live-stream ?) |
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.
Yes, I think it makes sense to cache URL as well
test/test_audio_reads.py
Outdated
@@ -121,6 +121,7 @@ def test_opus_torchaudio_vs_ffmpeg_with_resampling(path, force_opus_sampling_rat | |||
np.testing.assert_almost_equal(audio_ta, audio_ff, decimal=1) | |||
|
|||
|
|||
@pytest.mark.skip(reason="The audio caching by @lru_cache_optional was removed.") |
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.
Let's just remove the test
Thanks Karel! I left two last comments, if you could resolve those then LGTM |
- apply AudioCache also to URL reads - remove the unit test of the no-longer existing @lru_cache_optional audio caching
Okay, I did the two changes. Thanks Piotr! K. |
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.
Thanks Karel! LGTM, will just commit some cosmetics
Tested by running
./pruned_transducer_stateless7_streaming/streaming_decode.py
with dataset imported from kaldi format (locally).Question to @pzelasko : should the cache be used also for 'url' type of audio files ?
(locally I see 3x XFAIL on
pytest test/test_audio_reads.py -v
, but happens also for currentmaster
branch)