Enhance and clarify options for download function
#1147
Replies: 1 comment
-
|
Generally, I think this would be a better method of a results class than a stand alone method. That way,
These make sense to me and I think That said, I think it would make more sense to freeze (and eventually deprecate) this package-level method, and focus on results class method that would unburden us from the existing API. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
This discussion stems from this thread: #1142 (comment)
We would like to enhance the capabilities of the
downloadfunction, but also want to take care not to make it more difficult/confusing to use that it may already be.Here's the current signature:
The
threadsparameter is already a bit odd now thatpqdm_kwargswas added (at a later point thanthreadswas initially added), becausepqdm_kwargssupports ann_jobssetting that is now whatthreadsapplies to. This implicit (and undocumented) relationship can already lead to confusion, so we should not add yet another such parameter relationship (as proposed by @ana-sher in the thread linked above for handling/ignoring download errors).What we might want to consider at this point is some way to both deprecate some things as well as better consolidate/clarify various options that we want to support.
Specifically, I'd like to see
threadssomehow deprecated as a positional parameter, now that we have 3 keyword-only parameters, andpqdm_kwargssupportsn_jobswhich now "duplicates" the purpose ofthreads.Also,
show_progresseffectively duplicates thepqdm_kwargs.disableboolean, so that's another implicit and undocumented relationship.Since we have 2 parameters that duplicate the purpose of 2
pqdm_kwargsvalues (but with arguably better names:threadsinstead ofn_jobsandshow_progressinstead ofdisable), perhaps we should consider surfacing some or all of the otherpqdm_kwargsand deprecatepqdm_kwargsitself.However,
pqdm_kwargssupports quite a few options, whichpqdmpasses through totqdm, as documented in tqdm Objects. Therefore, it might not make sense to attempt to surface them all, but then how do we choose which ones to surface? This is why providingpqdm_kwargsas a parameter completely avoids this question, allowing the user to pass anything thattqdmsupports, both now and in the future.Perhaps a happy medium would be to keep things much as they are now, but properly document that
threadsautomatically setspqdm_kwargs.n_jobsandshow_progressautomatically setspqdm_kwargs.disable(inversely), and simply attempt to cleanly deprecatethreadsas a positional parameter in favor of making it a keyword-only parameter.Beyond that, per the linked thread from @ana-sher, how might we provide a cleaner way for users to filter out any failed downloads without raising an error?
Beta Was this translation helpful? Give feedback.
All reactions