-
Notifications
You must be signed in to change notification settings - Fork 1
CFT-3212 - Feature/async #9
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
base: main
Are you sure you want to change the base?
Conversation
Feature/async retry
aligned with the latest changes in the cookiecutter template 🍪
de527c3 to
4a4877f
Compare
4a4877f to
841709f
Compare
bacb974 to
e15824c
Compare
cd52e02 to
a733f7c
Compare
a733f7c to
f9db5cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds an asynchronous HTTP client alongside the existing synchronous client, migrates packaging to pyproject.toml with uv-based workflows, modernizes type hints, and expands documentation and examples to cover async usage and CI changes.
- Introduces AsyncHttpClient with retry, backoff, optional rate limiting, and batch processing.
- Refactors sync client type hints / formatting and replaces setup.py with pyproject.toml plus updated CI workflows.
- Expands README and examples for async usage (some documentation inconsistencies introduced).
Reviewed Changes
Copilot reviewed 15 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_async.py | New test suite for AsyncHttpClient (params/header/auth/retry handling; lacks coverage for some new behaviors). |
| src/keboola/http_client/http.py | Type hint modernization, import adjustments, formatting; minor logic unchanged except stylistic edits. |
| src/keboola/http_client/async_client.py | New asynchronous client implementation with retries, backoff, rate limiting, and batch processing. |
| src/keboola/http_client/init.py | Exposes AsyncHttpClient at package root. |
| pyproject.toml | New project metadata replacing setup.py; changes project name and dependency management. |
| flake8.cfg | Updated lint configuration (ignore list and limits). |
| docs/examples/* | Added sync and async usage examples, including batch processing and rate limiting demos. |
| README.md | Extended documentation for async client and reorganized sections (contains inaccuracies). |
| .github/workflows/*.yml | Updated CI to use uv, add Python versions, automate version injection and publishing. |
| setup.py | Removed in favor of pyproject.toml. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Default parameters | ||
| if self._default_params is not None: | ||
| all_pars = {**params, **self._default_params} |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per-call parameters are being overridden by default parameters when keys overlap (defaults take precedence). To allow explicit request parameters to override defaults, reverse the merge order: all_pars = {**self._default_params, **params}.
| all_pars = {**params, **self._default_params} | |
| all_pars = {**self._default_params, **params} |
Copilot uses AI. Check for mistakes.
| from http.cookiejar import CookieJar # noqa: F401 - false positive caused by stringified type annotation | ||
| from urllib.parse import quote, urlencode, urljoin, urlparse | ||
|
|
||
| import requests | ||
| from requests.adapters import HTTPAdapter | ||
| from requests.packages.urllib3.util.retry import Retry | ||
| from urllib3.util import Retry | ||
|
|
||
| Cookie = Union[Dict[str, str], CookieJar] | ||
| Cookie = "dict[str, str] | CookieJar" |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a string literal for the union type defeats static type checking; since from future import annotations is enabled, you can safely use a real annotation: Cookie = dict[str, str] | CookieJar.
Copilot uses AI. Check for mistakes.
| else: | ||
| raise type(e)(error_msg) from e | ||
|
|
||
| backoff = self.backoff_factor**retry_attempt |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backoff starts at 1 (self.backoff_factor**0) causing an unnecessary delay before the first retry; typical exponential backoff uses 0 initial delay and a multiplicative factor (e.g. backoff = self.backoff_factor * (2 ** (retry_attempt - 1)) for retry_attempt > 0). Consider adjusting to avoid penalizing the first retry.
| backoff = self.backoff_factor**retry_attempt | |
| backoff = 0 if retry_attempt == 0 else self.backoff_factor * (2 ** (retry_attempt - 1)) |
Copilot uses AI. Check for mistakes.
| if isinstance(e, httpx.HTTPStatusError): | ||
| raise | ||
| else: | ||
| raise type(e)(error_msg) from e |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent final error behavior: custom error_msg is only used for non-HTTPStatusError exceptions; HTTPStatusError path silently discards the enriched context. Either apply unified enriched messaging or remove the unused construction of error_msg for the status error path.
| if isinstance(e, httpx.HTTPStatusError): | |
| raise | |
| else: | |
| raise type(e)(error_msg) from e | |
| raise type(e)(error_msg) from e |
Copilot uses AI. Check for mistakes.
|
|
||
| return url | ||
|
|
||
| async def update_auth_header(self, updated_header: dict, overwrite: bool = False): |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method contains no awaits and performs synchronous mutations only; consider making it a regular def to simplify usage (so callers need not await it) and align with the synchronous client's API.
| async def update_auth_header(self, updated_header: dict, overwrite: bool = False): | |
| def update_auth_header(self, updated_header: dict, overwrite: bool = False): |
Copilot uses AI. Check for mistakes.
| if overwrite is False: | ||
| self._auth_header.update(updated_header) | ||
| else: | ||
| self._auth_header = updated_header |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method contains no awaits and performs synchronous mutations only; consider making it a regular def to simplify usage (so callers need not await it) and align with the synchronous client's API.
Copilot uses AI. Check for mistakes.
| ### AsyncHttpClient | ||
|
|
||
| The package also provides an asynchronous version of the HTTP client called AsyncHttpClient. | ||
| It allows you to make asynchronous requests using async/await syntax. To use the AsyncHttpClient, import it from keboola.http_client_async: |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text references a non-existent module keboola.http_client_async; import path is keboola.http_client (as shown in the code snippet). Update the sentence to match the actual import.
| It allows you to make asynchronous requests using async/await syntax. To use the AsyncHttpClient, import it from keboola.http_client_async: | |
| It allows you to make asynchronous requests using async/await syntax. To use the AsyncHttpClient, import it from keboola.http_client: |
Copilot uses AI. Check for mistakes.
| if response.status_code == 200: | ||
| data = response.json() | ||
| # Process the response data | ||
| else: | ||
| # Handle the error | ||
| pass | ||
|
|
||
|
|
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AsyncHttpClient.get returns a parsed dict (not an httpx.Response), so response.status_code and response.json() will fail; either use get_raw for a Response or remove those accesses and treat the result as already-decoded JSON.
| if response.status_code == 200: | |
| data = response.json() | |
| # Process the response data | |
| else: | |
| # Handle the error | |
| pass | |
| # response is already a parsed dict (JSON data) | |
| data = response | |
| # Process the response data | |
| # If you need status code or raw response, use client.get_raw(...) | |
| # For example: | |
| # raw_response = await client.get_raw("endpoint") | |
| # if raw_response.status_code == 200: | |
| # data = raw_response.json() |
Copilot uses AI. Check for mistakes.
| @@ -0,0 +1,50 @@ | |||
| [project] | |||
| name = "keboola.http-client" | |||
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Project name changed from 'keboola.http_client' (previous distribution) to 'keboola.http-client'; this may create a new distinct package on PyPI and break upgrade paths. Confirm intentional rename or revert to preserve continuity.
| name = "keboola.http-client" | |
| name = "keboola.http_client" |
Copilot uses AI. Check for mistakes.
| is_absolute_path = kwargs.pop("is_absolute_path", False) | ||
| url = await self._build_url(endpoint, is_absolute_path) | ||
|
|
||
| all_params = {**self.default_params, **(params or {})} |
Copilot
AI
Oct 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter precedence here (defaults overridden by per-call params) differs from synchronous client (where defaults override per-call params); once the synchronous client bug is fixed, ensure both clients use consistent precedence (per-call should override defaults).
Copilot uses AI. Check for mistakes.
added Async client
changed detailed exception message