Refactor download
NONE
_The documentation is not available anymore as the PR was closed or merged._
2022-05-21T10:37:46Z
https://github.com/huggingface/datasets/pull/4384#issuecomment-1133597485
https://api.github.com/repos/huggingface/datasets/issues/4384
IC_kwDODunzps5DkVMt
https://api.github.com/repos/huggingface/datasets/issues/comments/1133597485/reactions
2022-05-25T10:52:02Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1133597485
https://avatars.githubusercontent.com/u/99929124?v=4
https://api.github.com/users/HuggingFaceDocBuilderDev/events{/privacy}
https://api.github.com/users/HuggingFaceDocBuilderDev/followers
https://api.github.com/users/HuggingFaceDocBuilderDev/following{/other_user}
https://api.github.com/users/HuggingFaceDocBuilderDev/gists{/gist_id}
https://github.com/HuggingFaceDocBuilderDev
HuggingFaceDocBuilderDev
U_kgDOBfTMJA
https://api.github.com/users/HuggingFaceDocBuilderDev/orgs
https://api.github.com/users/HuggingFaceDocBuilderDev/received_events
https://api.github.com/users/HuggingFaceDocBuilderDev/repos
https://api.github.com/users/HuggingFaceDocBuilderDev/starred{/owner}{/repo}
https://api.github.com/users/HuggingFaceDocBuilderDev/subscriptions
User
https://api.github.com/users/HuggingFaceDocBuilderDev
public
MEMBER
This looks like a breaking change no ?
Also could you explain why it would be better this way ?
2022-05-24T14:42:55Z
https://github.com/huggingface/datasets/pull/4384#issuecomment-1136021545
https://api.github.com/repos/huggingface/datasets/issues/4384
IC_kwDODunzps5DtlAp
https://api.github.com/repos/huggingface/datasets/issues/comments/1136021545/reactions
2022-05-24T15:03:26Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1136021545
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
NONE
The might be only there to help type checkers, but I am not too familiar with the code base to know for sure. I think this might be useful:

https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING
2022-05-24T15:51:46Z
https://github.com/huggingface/datasets/pull/4384#issuecomment-1136102749
https://api.github.com/repos/huggingface/datasets/issues/4384
IC_kwDODunzps5Dt41d
https://api.github.com/repos/huggingface/datasets/issues/comments/1136102749/reactions
2022-05-24T15:51:46Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1136102749
https://avatars.githubusercontent.com/u/42706068?v=4
https://api.github.com/users/anivegesana/events{/privacy}
https://api.github.com/users/anivegesana/followers
https://api.github.com/users/anivegesana/following{/other_user}
https://api.github.com/users/anivegesana/gists{/gist_id}
https://github.com/anivegesana
anivegesana
MDQ6VXNlcjQyNzA2MDY4
https://api.github.com/users/anivegesana/orgs
https://api.github.com/users/anivegesana/received_events
https://api.github.com/users/anivegesana/repos
https://api.github.com/users/anivegesana/starred{/owner}{/repo}
https://api.github.com/users/anivegesana/subscriptions
User
https://api.github.com/users/anivegesana
public
MEMBER
> This looks like a breaking change no ?
> Also could you explain why it would be better this way ?

Sorry, @lhoestq, I naively thought it was obvious. I have tried to give some arguments in the motivation of this PR (see above). I can give additional arguments if needed. 
2022-05-25T09:50:45Z
https://github.com/huggingface/datasets/pull/4384#issuecomment-1137033702
https://api.github.com/repos/huggingface/datasets/issues/4384
IC_kwDODunzps5DxcHm
https://api.github.com/repos/huggingface/datasets/issues/comments/1137033702/reactions
2022-05-25T09:50:45Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1137033702
https://avatars.githubusercontent.com/u/8515462?v=4
https://api.github.com/users/albertvillanova/events{/privacy}
https://api.github.com/users/albertvillanova/followers
https://api.github.com/users/albertvillanova/following{/other_user}
https://api.github.com/users/albertvillanova/gists{/gist_id}
https://github.com/albertvillanova
albertvillanova
MDQ6VXNlcjg1MTU0NjI=
https://api.github.com/users/albertvillanova/orgs
https://api.github.com/users/albertvillanova/received_events
https://api.github.com/users/albertvillanova/repos
https://api.github.com/users/albertvillanova/starred{/owner}{/repo}
https://api.github.com/users/albertvillanova/subscriptions
User
https://api.github.com/users/albertvillanova
public
This PR performs a refactoring of the download functionalities, by proposing a modular solution and moving them to their own package "download". Some motivating arguments:
- understandability: from a logical partitioning of the library, it makes sense to have all download functionalities grouped together instead of scattered in a much larger directory containing many more different functionalities
- abstraction: the level of abstraction of "download" (higher) is not the same as "utils" (lower); putting different levels of abstraction together, makes dependencies more intricate (potential circular dependencies) and the system more tightly coupled; when the levels of abstraction are clearly separated, the dependencies flow in a neat direction from higher to lower
- architectural: "download" is a domain-specific functionality of our library/application (a dataset builder performs several actions: download, generate dataset and cache it); these functionalities are at the core of our library; on the other hand, "utils" are always a low-level set of functionalities, not directly related to our domain/business core logic (all libraries have "utils"), thus at the periphery of our lib architecture

Also note that when a library is not architecturally designed following simple, neat, clean principles, this has a negative impact on extensibility, making more and more difficult to make enhancements.

As a concrete example in this case, please see: https://app.circleci.com/pipelines/github/huggingface/datasets/12185/workflows/ff25a790-8e3f-45a1-aadd-9d79dfb73c4d/jobs/72860
- After an extension, a circular import is found
- Diving into the cause of this circular import, see the dependency flow, which should be from higher to lower levels of abstraction:
```
ImportError while loading conftest '/home/circleci/datasets/tests/conftest.py'.
tests/conftest.py:12: in <module>
    import datasets
../.pyenv/versions/3.6.15/lib/python3.6/site-packages/datasets/__init__.py:37: in <module>
    from .arrow_dataset import Dataset, concatenate_datasets
../.pyenv/versions/3.6.15/lib/python3.6/site-packages/datasets/arrow_dataset.py:59: in <module>
    from . import config
../.pyenv/versions/3.6.15/lib/python3.6/site-packages/datasets/config.py:8: in <module>
    from .utils.logging import get_logger
../.pyenv/versions/3.6.15/lib/python3.6/site-packages/datasets/utils/__init__.py:30: in <module>
    from .download_manager import DownloadConfig, DownloadManager, DownloadMode
../.pyenv/versions/3.6.15/lib/python3.6/site-packages/datasets/utils/download_manager.py:39: in <module>
    from .py_utils import NestedDataStructure, map_nested, size_str
../.pyenv/versions/3.6.15/lib/python3.6/site-packages/datasets/utils/py_utils.py:608: in <module>
    if config.DILL_VERSION < version.parse("0.3.5"):
E   AttributeError: module 'datasets.config' has no attribute 'DILL_VERSION'
```

Imports:
- datasets
  - Dataset: lower level than datasets
    - config: lower level than Dataset
      - logger: lower level than config
        - DownloadManager: !!! HIGHER level of abstraction than logger!!

Why when importing logger we require importing DownloadManager?!?
- Logically, it does not make sense
- This is due to an error in the design/architecture of our library:
  - To import the logger, we need to import it from `.utils.logging`
  - To import `.utils.logging` we need to import `.utils`
  - The import of `.utils` require the import of all its submodules defined in `utils.__init__.py`, among them: `.utils.download_manager`!

When putting `logging` and `download_manager` both inside `utils`, in order to import `logging` we need to import `download_manager` first: this is a strong coupling between modules and moreover between modules at different levels of abstraction (to import a lower level module, we require to import a higher level module). Additionally, it is clear that is makes no sense that in order to import `logging` we require to import `download_manager` first.