Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing unused dependency, adding optional dependencies and loosening requests version #104

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sg3-141-592
Copy link

@sg3-141-592 sg3-141-592 commented Nov 8, 2020

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.

…ement and making aiohttp and requests optional installs
@oschwald
Copy link
Member

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.

Base automatically changed from master to main January 20, 2021 18:27
Copy link

@sebix sebix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also possible without checking sys.modules.

import aiohttp
import aiohttp.http
except ImportError:
pass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pass
aiohttp = None

@@ -27,12 +27,21 @@

import ipaddress
import json
import sys
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import sys

import requests
import requests.utils
except ImportError:
pass
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pass
requests = None

)
# If neither requests or aiohttp is installed then inform user how to
# install them
if "aiohttp" not in sys.modules and "requests" not in sys.modules:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if "aiohttp" not in sys.modules and "requests" not in sys.modules:
if not requests and not aiohttp:

)


if "aiohttp" in sys.modules:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if "aiohttp" in sys.modules:
if aiohttp:

f"GeoIP2-Python-Client/{geoip2.__version__} {aiohttp.http.SERVER_SOFTWARE}"
)

if "requests" in sys.modules:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if "requests" in sys.modules:
if requests:

@sebix
Copy link

sebix commented Jul 13, 2021

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?

@akx
Copy link

akx commented Aug 27, 2024

This would be lovely to merge.

I was considering bumping geoip2 in a downstream project, but it would have added 3 new deps (aiohappyeyeballs==2.4.0, aiohttp==3.10.5, aiosignal==1.3.1) I don't need whatsoever.

EDIT: See #177 for a modernized version of this PR for the current packaging.

akx added a commit to akx/GeoIP2-python that referenced this pull request Aug 27, 2024
akx added a commit to akx/GeoIP2-python that referenced this pull request Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants