Skip to content

Conversation

@lhoestq
Copy link
Member

@lhoestq lhoestq commented Jun 15, 2022

#4384 moved datasets.utils.download_manager to datasets.download.download_manager

This breaks evaluate which imports DownloadMode from datasets.utils.download_manager

This PR re-adds datasets.utils.download_manager without circular imports.

We could also show a message that says that accessing it is deprecated, but I think we can do this in a subsequent PR, and just focus on doing a patch release for now

@lhoestq lhoestq requested a review from albertvillanova June 15, 2022 09:44
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 15, 2022

The documentation is not available anymore as the PR was closed or merged.

@albertvillanova
Copy link
Member

Thanks for the fix.

I'm wondering how this fixes backward compatibility...

Executing this code:

from datasets.utils.download_manager import DownloadMode

we will have

DownloadMode = None

If afterwards we use something like:

if download_mode == DownloadMode.FORCE_REDOWNLOAD

that will raise an exception.

@lhoestq
Copy link
Member Author

lhoestq commented Jun 15, 2022

It works fine on my side:

>>> from datasets.utils.download_manager import DownloadMode
>>> DownloadMode is not None
True

@lhoestq
Copy link
Member Author

lhoestq commented Jun 15, 2022

As reported in huggingface/evaluate#143

from datasets.utils import DownloadConfig

is also missing, I'm re-adding it

@lhoestq lhoestq merged commit 73868ff into master Jun 15, 2022
@lhoestq lhoestq deleted the readd-old-download_manager-in-utils branch June 15, 2022 10:23
@lhoestq
Copy link
Member Author

lhoestq commented Jun 15, 2022

Took the liberty of merging this one, to do a patch release soon. If we think of a better approach we can improve it later

khushmeeet pushed a commit to khushmeeet/datasets that referenced this pull request Jun 23, 2022
* re-add download_manager module in utils

* re-add DownloadConfig as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants