Make aiohttp and requests optional dependencies#177
Conversation
|
As discussed in the issue and other PR, this is a breaking change for web service users. I'd prefer either a solution that allows you to opt-out of the dependencies rather than opt-in or one that breaks the package into several packages where you could depend on the parts that you need with |
Yes, in semver terms this would be a major version bump. (In fact, the reason why I looked into this was that we have a downstream application that has pinned GeoIP to a much older version because the new version would pull in aiohttp and other things we don't want.) However, practically, chances are that web service users will already have
Unfortunately that's not possible with Python package extras.
In my view, it would be significantly more packaging and infra work for little profit. However, this practically does that already: the core package is |
6ac3f9c to
0eebbbd
Compare
|
Rebased on 5.0.1. |
e014f68 to
8e25cdf
Compare
|
Rebased. Looking at the changelog for 5.0.0, this could easily be just another "BREAKING" change for 6.0.0? |
|
Thanks for the PR! Looking at it, I think the approach still has the concerns @oschwald mentioned, in that I think this could break things for existing users as they would need to install new packages in situations where they are not already installing I understand the current situation isn't ideal. And there may not be a great alternative with the current packaging system. However I think the status quo is preferable until we have a solution that is less disruptive. |
|
In my opinion, asking users to change a For me and the company I work for, this is a supply chain vettability and possible security issue. While installing the library from this PR's HEAD (and in fact, this is what we currently do in production): |
|
I understand the concern, but we'd prefer to be conservative about potential breakage. The gain here doesn't seem big enough to warrant that. |
📝 WalkthroughWalkthroughThe package now declares ChangesOptional HTTP dependencies
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant AsyncClient
participant geoip2.webservice
participant aiohttp
AsyncClient->>geoip2.webservice: Call _session()
geoip2.webservice->>aiohttp: Check availability and create session
aiohttp-->>AsyncClient: Return HTTP session
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Around line 106-112: Remove aiohttp and requests from the
[project.dependencies] section, keeping them only under
[project.optional-dependencies] with their existing version constraints. Ensure
the base installation no longer installs either package while the named extras
continue to provide them.
In `@README.rst`:
- Around line 20-21: Update the README installation snippet to quote both
individual extras using shell-safe single quotes and add a third example
demonstrating the combined aiohttp and requests extras in the supported
comma-separated form.
In `@src/geoip2/webservice.py`:
- Around line 36-40: Update Client.__init__ to check whether the optional
requests import is unavailable before calling requests.Session() or
requests.utils.default_user_agent(). Raise an ImportError immediately when
requests is None, while preserving the existing initialization path when the
dependency is installed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc690fde-006e-4fbc-a7e7-ed77239abf63
📒 Files selected for processing (6)
README.rstpyproject.tomlsrc/geoip2/webservice.pytests/database_test.pytests/models_test.pytests/webservice_test.py
💤 Files with no reviewable changes (2)
- tests/models_test.py
- tests/database_test.py
Closes maxmind#104 (supersedes it) Fixes maxmind#101
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/webservice_test.py`:
- Line 406: Add focused tests in the relevant webservice test cases to simulate
missing aiohttp and requests imports, then assert that the resulting ImportError
contains the actionable installation guidance. Keep the existing
pytest.importorskip calls in the HTTP integration tests so core-only
environments continue to skip those tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9ce2d8c-60cc-4147-b50a-3c284c005d59
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
README.rstpyproject.tomlsrc/geoip2/webservice.pytests/webservice_test.py
| client: Client | ||
|
|
||
| def setUp(self) -> None: | ||
| pytest.importorskip("requests") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover missing-dependency behavior instead of only skipping it.
importorskip keeps core-only runs green, but skips every test that could exercise the new actionable ImportError path. Add focused tests that simulate missing aiohttp or requests and assert the installation guidance, while retaining these skips for HTTP integration tests.
Also applies to: 420-420
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/webservice_test.py` at line 406, Add focused tests in the relevant
webservice test cases to simulate missing aiohttp and requests imports, then
assert that the resulting ImportError contains the actionable installation
guidance. Keep the existing pytest.importorskip calls in the HTTP integration
tests so core-only environments continue to skip those tests.
3 participants
This PR makes
aiohttpandrequests, only required forgeoip2.webservice, optional dependencies.They can be installed separately (many projects will likely have
requestsanyway), or using the extras provided, i.e.to use the version range specified by requirements.
The Tox configuration is adjusted so tests are run both with and without the libraries installed.
This fixes #101 (makes it possible to install without the webservice bits).
This closes #104 (supersedes it to work with the current packaging and testing infrastructure).
Summary by CodeRabbit
geoip2[aiohttp]andgeoip2[requests].