Create FetchConfig for standardizing use of fetch#2828
Conversation
|
Wondering about this PR. Are you aware of any other place external to the speechbrain codebase that are using the |
…te and 100x faster
…mbridge) (speechbrain#2913) Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com>
…#2914) Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com>
Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com>
…e) (speechbrain#2915) Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com>
Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com>
Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com>
…n#2911) Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com>
…ter, Cambridge) (speechbrain#2894) Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com> Co-authored-by: Adel Moumen <88119391+Adel-Moumen@users.noreply.github.com>
Co-authored-by: Rogier van Dalen <r.vandalen@samsung.com> Co-authored-by: Peter Plantinga <plantinga.peter@protonmail.com>
Co-authored-by: Parcollet Titouan <parcollet.titouan@gmail.com>
…rain#2782) Co-authored-by: Adel Moumen <adelmoumen.pro@gmail.com>
Co-authored-by: Adel Moumen <adelmoumen.pro@gmail.com> Co-authored-by: Titouan Parcollet <parcollet.titouan@gmail.com>
… Center Cambridge) (speechbrain#2765) Co-authored-by: Shucong Zhang/Embedded AI /SRUK/Engineer/Samsung Electronics <s1.zhang@sruk-ccn4.eu.corp.samsungelectronics.net> Co-authored-by: Parcollet Titouan <parcollet.titouan@gmail.com>
| huggingface_cache_dir=None, | ||
| local_strategy: LocalStrategy = LocalStrategy.SYMLINK, | ||
| local_strategy=LocalStrategy.SYMLINK, | ||
| fetch_config=FetchConfig(), |
There was a problem hiding this comment.
we are again using FetchConfig() as a default value. Can't we just set it to None and if it is None, then init FetchConfig() ?
There was a problem hiding this comment.
I wonder if I should use a NamedTuple here? This is immutable so we don't have to worry about default values, but it might not play as nicely with type hints if we're going to add those.
There was a problem hiding this comment.
Or, perhaps even better is to use frozen=True
https://docs.python.org/3/library/dataclasses.html#frozen-instances
There was a problem hiding this comment.
Yes, with frozen=True it makes it immutable. I guess that if we don't plan to have mutable arguments in FetchConfig, we can just set it as a default value for fetch_config. Otherwise, if we really want to be safe (but I don't think this is really necessary), we can just do fetch_config: Optional[FetchConfig] = None and:
if fetch_config is None:
fetch_config = FetchConfig()
Both are fine, but fetch_config=FetchConfig() is a bit more idiomatic so we can leave this like that:)
Adel-Moumen
left a comment
There was a problem hiding this comment.
LGTM!
Quick question: is there a way to tests these functions and make sure they do what they are intended to do?
|
The functions are called as part of the tests, several unit tests and doctests download things via fetch. I suppose we could test it more systematically with some unit tests, especially loading from local
…On Tue, Jun 3, 2025 at 4:50 AM, Adel Moumen ***@***.***(mailto:On Tue, Jun 3, 2025 at 4:50 AM, Adel Moumen <<a href=)> wrote:
@Adel-Moumen approved this pull request.
LGTM!
Quick question: is there a way to tests these functions and make sure they do what they are intended to do?
—
Reply to this email directly, [view it on GitHub](#2828 (review)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABBKVUTPTQJ5FI7ORZC3P7D3BVOVXAVCNFSM6AAAAABXFRCQEKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDQOJRGM2TIMRYGI).
You are receiving this because you were assigned.Message ID: ***@***.***>
|
Code for calling
fetchuses a variety of subsets of the actual available options forfetch. This simplifies and standardizes the use offetchby putting the configuration options in a single place, aFetchConfigdataclass. This should probably sit indevelopfor awhile before releasing a new version to ensure nothing is broken.This PR also makes
fetchmultiprocessing-safe (and therefore save to use with DDP).