Local paths in common voice

[lhoestq]

Continuation of #3664:

• pass the streaming parameter to _split_generator
• update @anton-l's code to use this parameter for common_voice
• add a comment to explain why we use download_and_extract in non-streaming and iter_archive in streaming

Now the common_voice dataset has a local path back in ds["path"], and this field is None in streaming mode.

cc @patrickvonplaten @anton-l @albertvillanova

Fix #3663.

[patrickvonplaten]

small nit - I'd even pass the streaming flag here to make it super clear that they are two different modes and maybe have both a _generate_examples_streaming(...) and a _generate_examples_non_streaming(...)

[patrickvonplaten]

That looks great to me! Think it's quite easy to understand that there are two parallel ways now on how to download and prepare audio datasets

[albertvillanova]

I think the importance of this PR is fixing Common Voice, once we realize that the approach of non-extracting the archive content in non-streaming mode is not optimal/suitable for users who want to have direct access to audio file paths.

So OK for approving it and merge/fix Common Voice the faster the better.

But on the other hand, IMHO, I think this specific solution adds complexity to handling streaming/non-streaming, and moves this complexity to the loading script and thus to the contributors/users who want to create the loading script for their canonical/community datasets (instead of keeping it hidden form the end users).

Maybe we could have a discussion in the near future to see if it is possible to find other solutions (or not), while keeping the requirement of having access to the audio file paths in non-streaming. It is just a suggestion! :)

[albertvillanova]

I guess you need the DownloadManager instance to find out whether we are in streaming mode or not... Because the builder itself knows nothing about streaming or not...

[lhoestq]

Indeed having this logic inside the builder can be a bit unexpected. An alternative would be to replace the streaming parameter by

streaming = dl_manager.is_streaming

inside the dataset script

[lhoestq]

I just changed to dl_manager.is_streaming rather than an additional parameter streaming that has to be handled by the DatasetBuilder class - this way the streaming logic doesn't interfere with the base builder's code.

I think it's better this way, but let me know if you preferred the previous way and I can revert

But on the other hand, IMHO, I think this specific solution adds complexity to handling streaming/non-streaming, and moves this complexity to the loading script and thus to the contributors/users who want to create the loading script for their canonical/community datasets (instead of keeping it hidden form the end users).

I'm down to discuss this more in the future !

[albertvillanova]

@lhoestq good idea: much cleaner this way! That way each class has its own responsibilities without mixing around...