Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #604 +/- ##
===========================================
+ Coverage 76.18% 96.38% +20.19%
===========================================
Files 42 43 +1
Lines 2482 4282 +1800
===========================================
+ Hits 1891 4127 +2236
+ Misses 591 155 -436
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Major refactor that replaces dict-based API responses with generated, fully typed Pydantic models across the client, plus a new impit-based HTTP layer.
Changes:
- Introduces new typed resource clients and response wrappers around generated Pydantic models.
- Replaces legacy HTTP client with new
SyncHttpClient/AsyncHttpClientand updates tests/docs accordingly. - Adds extensive new unit/integration coverage and model generation tooling configuration.
Reviewed changes
Copilot reviewed 100 out of 106 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_client_timeouts.py | Updates timeout tests to use new http clients + timedelta. |
| tests/unit/test_client_request_queue.py | Updates request-queue tests for typed responses and explicit API URLs. |
| tests/unit/test_client_headers.py | Removes legacy header tests (likely superseded by new HTTP layer). |
| tests/unit/test_client_errors.py | Switches error tests to new http clients and improves assertion naming. |
| tests/unit/test_actor_start_params.py | Adds unit tests ensuring timeout query param is used with typed client. |
| tests/unit/conftest.py | Removes patch_basic_url fixture; keeps httpserver fixture. |
| tests/unit/README.md | Documents unit-test isolation requirements. |
| tests/integration/test_webhook_dispatch.py | Adds unified sync/async integration coverage for webhook dispatch endpoints. |
| tests/integration/test_webhook.py | Adds unified sync/async integration coverage for webhooks. |
| tests/integration/test_user.py | Adds unified sync/async integration coverage for user endpoints (typed). |
| tests/integration/test_store.py | Reworks store tests into unified sync/async style with typed models. |
| tests/integration/test_schedule.py | Adds unified sync/async integration coverage for schedules. |
| tests/integration/test_run_collection.py | Removes legacy run-collection integration tests (dict-based). |
| tests/integration/test_log.py | Adds unified sync/async integration coverage for logs. |
| tests/integration/test_build.py | Adds unified sync/async integration coverage for builds. |
| tests/integration/test_basic.py | Removes legacy basic integration tests (dict-based). |
| tests/integration/test_apify_client.py | Adds unified basic integration test using typed models. |
| tests/integration/test_actor_version.py | Adds unified sync/async integration coverage for actor versions. |
| tests/integration/test_actor_env_var.py | Adds unified sync/async integration coverage for actor env vars. |
| tests/integration/test_actor.py | Adds unified sync/async integration coverage for actors. |
| tests/integration/integration_test_utils.py | Removes old integration utilities (now likely in conftest). |
| tests/integration/README.md | Documents integration tests’ requirement for real API tokens. |
| src/apify_client/errors.py | Improves error docstrings and hardens JSON error extraction. |
| src/apify_client/clients/resource_clients/run_collection.py | Removes legacy client implementation (dict-based). |
| src/apify_client/clients/resource_clients/build.py | Removes legacy build client implementation (dict-based). |
| src/apify_client/clients/resource_clients/actor_env_var_collection.py | Removes legacy env var collection client (dict-based). |
| src/apify_client/clients/base/resource_collection_client.py | Removes legacy base list/create abstraction (dict-based). |
| src/apify_client/clients/base/resource_client.py | Removes legacy base resource client abstraction (dict-based). |
| src/apify_client/clients/base/base_client.py | Removes legacy base client (replaced by new resource client base). |
| src/apify_client/clients/base/actor_job_base_client.py | Removes legacy polling/abort base (replaced elsewhere). |
| src/apify_client/clients/base/init.py | Removes legacy base exports. |
| src/apify_client/clients/init.py | Removes legacy large re-export module. |
| src/apify_client/_types.py | Removes old JSON/ListPage types (moved/replaced). |
| src/apify_client/_statistics.py | Renames Statistics -> ClientStatistics. |
| src/apify_client/_resource_clients/webhook_dispatch_collection.py | Converts to typed models + new base resource client. |
| src/apify_client/_resource_clients/webhook_dispatch.py | Converts get() to typed models and explicit 404 handling. |
| src/apify_client/_resource_clients/webhook_collection.py | Converts list/create to typed models and new representation helpers. |
| src/apify_client/_resource_clients/webhook.py | Converts get/update/delete/test/dispatches to typed models and new base. |
| src/apify_client/_resource_clients/user.py | Converts user endpoints to typed models and new base. |
| src/apify_client/_resource_clients/store_collection.py | Converts store listing to typed models and new base. |
| src/apify_client/_resource_clients/schedule_collection.py | Converts schedule list/create to typed models and new base. |
| src/apify_client/_resource_clients/schedule.py | Converts schedule get/update/delete/log to typed models and new base. |
| src/apify_client/_resource_clients/run_collection.py | Adds new typed run collection client with pagination iteration. |
| src/apify_client/_resource_clients/request_queue_collection.py | Converts list/get_or_create to typed models and new base. |
| src/apify_client/_resource_clients/log.py | Updates to new base client, typed run usage, and task lifecycle handling. |
| src/apify_client/_resource_clients/key_value_store_collection.py | Converts list/get_or_create to typed models and new base. |
| src/apify_client/_resource_clients/dataset_collection.py | Converts list/get_or_create to typed models and new base. |
| src/apify_client/_resource_clients/build_collection.py | Converts list to typed models and new base. |
| src/apify_client/_resource_clients/build.py | Adds new typed build client with log + wait_for_finish. |
| src/apify_client/_resource_clients/actor_version_collection.py | Converts list/create to typed models and new base. |
| src/apify_client/_resource_clients/actor_env_var_collection.py | Adds new typed actor env var collection client. |
| src/apify_client/_resource_clients/actor_env_var.py | Converts env var get/update/delete to typed models and new base. |
| src/apify_client/_resource_clients/init.py | Expands exports for log streaming/status watchers; updates resource exports. |
| src/apify_client/_logging.py | Refactors logging helpers, adds redirect logger utilities, updates context injection. |
| src/apify_client/_internal_models.py | Adds internal minimal Pydantic models for polling/validation (non-generated). |
| src/apify_client/_http_clients/_sync.py | Adds new synchronous impit-based HTTP client with retries/backoff. |
| src/apify_client/_http_clients/_base.py | Adds shared HTTP base: headers, param conversion, gzip JSON, timeout scaling. |
| src/apify_client/_http_clients/_async.py | Adds new async impit-based HTTP client with retries/backoff. |
| src/apify_client/_http_clients/init.py | Adds package entrypoint for new HTTP clients. |
| src/apify_client/_http_client.py | Removes legacy HTTP client implementation. |
| src/apify_client/_consts.py | Adds shared constants (timeouts, retries, terminal statuses, API URL). |
| src/apify_client/_client_registry.py | Adds sync/async registries for DI and avoiding circular imports. |
| src/apify_client/init.py | Switches public import to new _apify_client module. |
| scripts/utils.py | Improves docstring conversion and typing in scripts. |
| scripts/fix_async_docstrings.py | Skips _http_clients and handles missing sync class/methods. |
| scripts/check_async_docstrings.py | Skips _http_clients and handles missing sync classes. |
| pyproject.toml | Updates dependencies, adds datamodel-codegen config, fixes ruff ignore for generated models, bumps test concurrency, adds generate-models task. |
| docs/03_examples/code/03_retrieve_sync.py | Updates examples to attribute access for typed models. |
| docs/03_examples/code/03_retrieve_async.py | Updates examples to attribute access for typed models. |
| docs/03_examples/code/02_tasks_sync.py | Updates examples to typed Task/Run + attribute access. |
| docs/03_examples/code/02_tasks_async.py | Updates examples to typed Task/Run + attribute access. |
| docs/03_examples/code/01_input_sync.py | Updates timeout usage to timedelta. |
| docs/03_examples/code/01_input_async.py | Updates timeout usage to timedelta. |
| docs/02_concepts/code/05_retries_sync.py | Updates retry/timeout params to timedelta. |
| docs/02_concepts/code/05_retries_async.py | Updates retry/timeout params to timedelta. |
| docs/02_concepts/code/01_async_support.py | Updates example to attribute access for typed run id. |
| docs/01_overview/code/01_usage_sync.py | Updates example to attribute access for typed dataset id. |
| docs/01_overview/code/01_usage_async.py | Updates example to attribute access for typed dataset id. |
| .github/workflows/_tests.yaml | Increases CI test concurrency default to 16. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a745fa3 to
10fb739
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 101 out of 103 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/apify_client/_logging.py:1
- In async streaming, checking
self._streaming_task.done()before raising RuntimeError prevents detecting when a task exists but has already completed. If a completed task exists and start() is called again, it will create a new task, potentially leading to resource leaks if the old task wasn't properly cleaned up. The condition should either always reject ifself._streaming_taskexists, or should setself._streaming_task = Noneafter completion.
from __future__ import annotations
src/apify_client/_logging.py:1
- Same issue as the streaming task - checking
self._logging_task.done()before raising RuntimeError means a completed logging task won't prevent start() from being called again, potentially creating resource leaks. The condition should either always reject ifself._logging_taskexists, or should setself._logging_task = Noneafter completion.
from __future__ import annotations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Everything has been addressed. Feel free to add more feedback or reopen any conversations if you feel something wasn't properly resolved. |
Pijukatel
left a comment
There was a problem hiding this comment.
What is the release plan? Wait for the validator, merge it, and release a new major version?
janbuchar
left a comment
There was a problem hiding this comment.
Could this get an upgrading guide, too?
There was a problem hiding this comment.
Could this be split into multiple files? I know it's autogenerated, but still, there's a good chance someone will want to view it.
There was a problem hiding this comment.
Could this be split into multiple files?
I would say it can be either one module with all the classes or a subpackage with one file per class.
Would you rather prefer a _models subpackage with ~100 modules? I am not really sure. The current state is not that bad.
Merge this. Resolve all other issues for https://github.com/apify/apify-client-python/milestone/560 milestone. Make sure the SDK is adapted. Release the new client. Release the new SDK. |
I'll add it all at once at the end, once all issues from https://github.com/apify/apify-client-python/milestone/560 are resolved. So it can receive a proper review, and it is not released before the actual client release. |
f4c476b to
865ce87
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 102 out of 104 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/unit/test_actor_start_params.py:1
- The
finishedAttimestamp (2019-12-12) is after thestartedAttimestamp (2019-11-30), but the Actor run has status RUNNING which is inconsistent. A running Actor should not have afinishedAtvalue. Either remove thefinishedAtfield or change the status to a terminal state like SUCCEEDED.
from __future__ import annotations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| message = record.getMessage() | ||
| return f'{formatted_logger_name} -> {message}' |
There was a problem hiding this comment.
The RedirectLogFormatter.format() method directly accesses record.getMessage() but doesn't call super().format(), which means it skips standard formatting features like exception formatting and extra fields. Consider calling super().format(record) and prepending the logger name to the result to preserve all standard formatting behavior.
| message = record.getMessage() | |
| return f'{formatted_logger_name} -> {message}' | |
| formatted_message = super().format(record) | |
| return f'{formatted_logger_name} -> {formatted_message}' |
Summary
This is a major refactoring that introduces fully typed Pydantic models throughout the client library. The models are generated from the OpenAPI specifications. All API responses now return typed objects instead of raw dictionaries.
This follows up on apify/apify-docs#2182.
Issues
Packages
Pydantic.apify-shared.Key changes
pyproject.tomlto generate Pydantic models based on the OpenAPI specs.Actor,Task,Run, etc.).apify/apify-sdk-pythonfor details - chore: Adapt to apify-client v3 [WIP] apify-sdk-python#719.Architecture
ClientRegistryto be able to achieve that (because of resource clients-siblings imports).Breaking changes
result['key']) to attribute-style (result.key).Test plan
Next steps