torchvision conda package missing requests dependency

[malfet]

🐛 Describe the bug

torchvision depends on requests, but it is not specified as conda package dependency, which results in the following failures:

% python
Python 3.8.11 (default, Aug  6 2021, 08:56:27) 
[Clang 10.0.0 ] :: Anaconda, Inc. on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from torchvision import datasets
>>> calldetch_ds = datasets.Caltech101("datasets", download=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/nshulga/miniconda3/envs/py_38/lib/python3.8/site-packages/torchvision/datasets/caltech.py", line 52, in __init__
    self.download()
  File "/Users/nshulga/miniconda3/envs/py_38/lib/python3.8/site-packages/torchvision/datasets/caltech.py", line 124, in download
    download_and_extract_archive(
  File "/Users/nshulga/miniconda3/envs/py_38/lib/python3.8/site-packages/torchvision/datasets/utils.py", line 427, in download_and_extract_archive
    download_url(url, download_root, filename, md5)
  File "/Users/nshulga/miniconda3/envs/py_38/lib/python3.8/site-packages/torchvision/datasets/utils.py", line 135, in download_url
    return download_file_from_google_drive(file_id, root, filename, md5)
  File "/Users/nshulga/miniconda3/envs/py_38/lib/python3.8/site-packages/torchvision/datasets/utils.py", line 204, in download_file_from_google_drive
    import requests
ModuleNotFoundError: No module named 'requests'

Versions

0.11.1 on MacOS

[datumbox]

I believe the dependency is on purpose not in the mandatory list. It's considered optional (see old issue at #2618).

The only use of the requests is at:

vision/torchvision/datasets/utils.py

Lines 203 to 217 in d367a01

	import requests
	url = "https://docs.google.com/uc?export=download"
	root = os.path.expanduser(root)
	if not filename:
	filename = file_id
	fpath = os.path.join(root, filename)
	os.makedirs(root, exist_ok=True)
	if os.path.isfile(fpath) and check_integrity(fpath, md5):
	print("Using downloaded and verified file: " + fpath)
	else:
	session = requests.Session()

[malfet]

@datumbox in that case it should be add to extra_deps, same as scipy, shouldn't it?

[datumbox]

@malfet I've had a quick look to see what other optional dependencies we are using. Here is a list:

• IO:
  • accimage
  • av
• Datasets:
  • torchdata (Prototype-only)
  • scipy
  • pycocotools
  • lmdb
  • requests

Judging from the discussion at #2618, it seems that there is consensus on keeping track of the optional dependencies but also maintaining lazy imports. Not sure what's the best way to achieve this in Conda, any thoughts?

cc @fmassa @pmeier @vfdev-5

[malfet]

Don't know about conda, but we can totally do it for pip. Also, a nicer error message would be great (and perhaps in case of requests, one can use urllib instead)

[fmassa]

If the only use of requests is in that function, I think I would go with @malfet suggestion of re-implementing that function instead using urllib. We already use urllib in most of that file.

About the dependencies on scipy / pycocotools / lmdb / etc, they are all dataset-dependent and in some sense fair requirements for the user to manually install those libraries if they want to use those datasets.

But for download_file_from_google_drive, I would indeed just re-implement this function in this case.

[datumbox]

@pmeier Could you have a look if it makes sense replacing requests with urllib? I assume you are using requests.Session to handle the cookies for you. I wonder how much extra code we need to maintain to replace it.

Concerning the original question at hand, do we want to declare the optional dependencies somehow? The benefit of tracking them explicitly is that we don't have to scan the code-base to find dependencies. As @malfet said, setuptools has a way to do it using the extras_require field.

We could add something like the following to keep track of them:

        extras_require={
            "scipy": ["scipy"],
            "pycocotools": ["pycocotools"],
            "lmdb": ["lmdb"],
            "requests": ["requests"],
            "accimage": ["accimage"],
            "av": ["av"],
        },

An alternative approach is to group them by usage. Something like:

        extras_require={
            "datasets": ["scipy", "pycocotools", "lmdb", "requests"],
            "io": ["accimage", "av"],
        },

I would probably prefer the 1st approach as it gives more control but I would be OK with the latter as well. Thoughts?

[pmeier]

Could you have a look if it makes sense replacing requests with urllib? I assume you are using requests.Session to handle the cookies for you. I wonder how much extra code we need to maintain to replace it.

If you look at the linked StackOverflow thread

vision/torchvision/datasets/utils.py

Line 202 in 30f4d10

# Based on https://stackoverflow.com/questions/38511444/python-download-files-from-google-drive-using-url

you see that this is almost a 1-to-1 copy. I'm fairly certain it wouldn't be hard to replicate using only urllib.

We could add something like the following to keep track of them:

I would prefer option 2. Going from pip install torchvision scipy pycocotools to pip install torchvision[scipy, pycocotools] isn't really much easier. Plus, if the requirements change in the future, we don't need to change the docs, since pip install torchvision[datasets] would still give you anything you need to run the datasets.

[datumbox]

I'm fairly certain it wouldn't be hard to replicate using only urllib.

Sounds great, would you mind creating a low priority issue to fix this? Until then, we could add this requests as an optional dependency.

I would prefer option 2.

No strong opinions. My motivation for adding them there, is to keep an explicit list of all dependencies, because now we need to scan the code-base fo figure them out. @fmassa thoughts?

Once we get an agreement of where to put this, it would be good to send a PR that updates also the scripts of CI and installs the optional dependency using the proposed solution, instead of adding them one by one (like here). This will help us maintain parity between different environments (linux vs windows) and avoid defining the dependencies over and over.

[NicolasHug]

I would go with option 2 as well

[datumbox]

@pmeier Would you be interested in sending a PR that fixes this? Perhaps taking the time to update the CI scripts from installing the dependencies separately?