GeoIP2-python icon indicating copy to clipboard operation
GeoIP2-python copied to clipboard

Removing unused dependency, adding optional dependencies and loosening requests version

Open sg3-141-592 opened this issue 5 years ago • 3 comments

Linked to Issue #101. I noticed when installing the library that the dependency tree is quite deep. I have to install to a non-internet connected environment so I have to download all of the .whls manually which highlighted the issue.

image

I saw some opportunities for making the installation a bit leaner.

  • Have removed unused dependency urllib3.

  • Have made aiohttp and requests optional installs. User gets an error message when trying to import the libraries without requests or aiohttp installed

>>> import geoip2.webservice
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sean/GeoIP2-python/geoip2/webservice.py", line 49, in <module>
    raise ImportError("""To enable geoip2
ImportError: To enable geoip2.webservice, install aiohttp or requests support.
        pip install geoip2[aiohttp]
        pip install geoip2[requests]
        pip install geoip2[all]

Some figures on the size of the install for different configurations

geoip2 - 0.3MB
geoip2[aiohttp] - 12.4MB
geoip2[requests] - 2.3MB
geoip2[all] - 14.4MB

This would be a big change to how this library is installed so this would need some maintainer feedback on this level of change.

  • Also I noticed a very specific version of requests has been picked >=2.24.0 which is from June 2020 onwards. I'm not sure this package needs such a specific version looking the history of the requests package (https://github.com/psf/requests/blob/master/HISTORY.md). And it'll require the majority of people who use it who are already using requests to need to uninstall their existing version and install 2.24.0.

    Since the minimum supported version of Python for this library is Python 3.6, and the earliest version of requests that supports Python 3.6 is 2.14.0 I've suggested updating this dependency to requests>=2.14.0,<3.0.0. I've tested with requests 2.14.0 and all tests are passing with identical results.

sg3-141-592 avatar Nov 08 '20 08:11 sg3-141-592

Although the extra dependencies do make some sense, this would likely be a breaking change for many existing web service users. I don't know if this is easily resolvable with Setuptools. Perhaps at some point, it would make sense to break this up into several packages but it would be nice to that in such a way that doesn't increase maintenance burden.

I believe urllib3 is an indirect dependency and was added as a direct dependency to ensure that our users were not subject to a security issue with earlier versions. Similarly, the requests version specified in this PR has at least one significant security vulnerability. It has been our preference to require a recent version of these deps to make sure users who only depend on the libraries indirectly get security updates and fixes for other issues.

oschwald avatar Nov 10 '20 16:11 oschwald

Although the extra dependencies do make some sense, this would likely be a breaking change for many existing web service users. I don't know if this is easily resolvable with Setuptools. Perhaps at some point, it would make sense to break this up into several packages but it would be nice to that in such a way that doesn't increase maintenance burden.

The extras-feature of setuptools is already a very easy way to "split" the requirements.

I believe urllib3 is an indirect dependency and was added as a direct dependency to ensure that our users were not subject to a security issue with earlier versions.

requests even comes with a built-in version of urllib3, so that won't be effective.

Similarly, the requests version specified in this PR has at least one significant security vulnerability. It has been our preference to require a recent version of these deps to make sure users who only depend on the libraries indirectly get security updates and fixes for other issues.

Although distributions ship "older" versions of requests, they patch the security issues in the library. Requiring very current versions, causes an upgrade of requests during the installation of geoip2, possible causing compatibility issues. Could you instead recommend a higher version (e.g. using an optional requirements.txt) but not strictly requiring (in setup.py/cfg) it?

sebix avatar Jul 13 '21 19:07 sebix