Repository navigation
feat: add zstd request body compression - #1218
apify-factory wants to merge 21 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1218 +/- ##
==========================================
- Coverage 95.76% 95.66% -0.10%
==========================================
Files 63 64 +1
Lines 6676 6717 +41
==========================================
+ Hits 6393 6426 +33
- Misses 283 291 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vdusek
left a comment
There was a problem hiding this comment.
Could you add a self-review / staff review to the porting process?
vdusek
left a comment
There was a problem hiding this comment.
Note: compressor_case in tests/unit/test_http_clients.py (outside the diff) still covers only gzip and brotli. Consider adding a zstd case: pytest.param((ZstdHttpCompressor(), 'zstd', zstd.decompress), id='zstd').
✍️ Drafted by Claude Code
| else: | ||
| # Not `from backports import zstd`: when another `backports.*` package is installed but this one is not, that | ||
| # raises a plain `ImportError` instead of the `ModuleNotFoundError` the optional-extra handling expects. | ||
| import backports.zstd as zstd # noqa: PLR0402 |
There was a problem hiding this comment.
Note: If backports.zstd is installed but its C extension fails to load, this raises a plain ImportError. try_import catches only ModuleNotFoundError, so import apify_client fails for every user, including gzip users. The 3.14+ branch above handles this case. Should this branch handle it the same way, or is it fine because brotli behaves the same?
✍️ Drafted by Claude Code
There was a problem hiding this comment.
Good catch, handled in b114508 and db6ed4e. On Python < 3.14, a plain ImportError from backports.zstd is now re-raised as ModuleNotFoundError(name='backports.zstd'), so import apify_client keeps working and only ZstdHttpCompressor raises. The error says the package is installed but cannot be loaded, and suggests pip install --force-reinstall backports.zstd or gzip/brotli. try_import no longer appends the "install the extra" hint to an error raised from another one, since installing the extra again doesn't help in that case. Both cases have unit tests (test_zstd_import_hook_raises_clear_error_when_backport_fails_to_load, test_try_import_keeps_the_message_of_a_dependency_that_fails_to_load). brotli is left as it is.
| if sys.version_info >= (3, 14): | ||
| from compression import zstd | ||
| else: | ||
| import backports.zstd as zstd # noqa: PLR0402 - the same import form as `http_compressors/_zstd.py` |
There was a problem hiding this comment.
Nit: The reason for this import form in _zstd.py is to get a ModuleNotFoundError that try_import can catch. Tests don't go through try_import, so the plain form works and the noqa can go.
| import backports.zstd as zstd # noqa: PLR0402 - the same import form as `http_compressors/_zstd.py` | |
| from backports import zstd |
✍️ Drafted by Claude Code
There was a problem hiding this comment.
I tried this in 345e092 but reverted it in 7964ffa. Type checking runs with python-version = "3.11", so ty checks this branch even on 3.14/3.15, where backports.zstd isn't installed. allowed-unresolved-imports = ["backports.zstd"] covers import backports.zstd. from backports import zstd resolves the module backports, so it fails with Cannot resolve imported module 'backports' and would break the 3.14 and 3.15 type_check jobs. Allowing all of backports would also work, but it's broader. I kept the dotted form and made the noqa reason say why.
| if sys.version_info >= (3, 14): | ||
| from compression import zstd | ||
| else: | ||
| import backports.zstd as zstd # noqa: PLR0402 - the same import form as `http_compressors/_zstd.py` |
There was a problem hiding this comment.
Nit: The reason for this import form in _zstd.py is to get a ModuleNotFoundError that try_import can catch. Tests don't go through try_import, so the plain form works and the noqa can go.
| import backports.zstd as zstd # noqa: PLR0402 - the same import form as `http_compressors/_zstd.py` | |
| from backports import zstd |
✍️ Drafted by Claude Code
There was a problem hiding this comment.
Same as in test_http_compressors.py: I kept import backports.zstd as zstd, because from backports import zstd fails ty on 3.14+ (it resolves backports, which the allowed-unresolved-imports entry doesn't cover). The noqa now gives the reason.
| ``` | ||
|
|
||
| To compress with `zstd`, pass `compression='zstd'`. Python 3.14 and newer have it in the standard library. On older | ||
| versions, install the optional `zstd` extra: `pip install "apify-client[zstd]"`. |
There was a problem hiding this comment.
Nit: The brotli section above shows both pip install and uv add. Show uv add "apify-client[zstd]" here too. The same applies to docs/01_introduction/index.mdx.
✍️ Drafted by Claude Code
There was a problem hiding this comment.
Added uv add "apify-client[zstd]" here and in docs/01_introduction/index.mdx in 7fe7fe1.
| @@ -14,9 +14,14 @@ | |||
|
|
|||
There was a problem hiding this comment.
Could we add integration test that sends a zstd body to the live API?
There was a problem hiding this comment.
Not added in this PR: I can't run tests against the live API from here, and I don't want to add an integration test that has never been run. The natural place is next to test_key_value_store_set_and_get_pre_compressed_record in tests/integration/test_key_value_store.py: a client with compression='zstd' pushing a large record or dataset items and reading them back. It needs someone with API tokens to add and run it, either in this PR or as a follow-up.
|
Re the review note on |
Cover a key-value store record and a dataset push through a client that compresses request bodies with zstd, for both transports and both the sync and async client. Co-Authored-By: Claude Fable 5.2 <[email protected]> Claude-Session: https://claude.ai/code/session_017ocuAK2fAevKdoyPCB2HvC
Reference change
apify/apify-client-js#1200
Ported change
HttpCompressionAlgorithm(apify_client.types) now accepts'zstd', andresolve_compressormaps it to the new compressor. Thecompressionargument ofApifyClient/ApifyClientAsyncand ofActorServerClient.requesttherefore accepts'zstd'.apify_client.http_compressors.ZstdHttpCompressor(http_compressors/_zstd.py), modeled on the gzip and brotli compressors. It reportscontent_encoding = 'zstd'and takes a keyword-onlyqualityfrom1to19, defaulting to3. Levels 20–22 are rejected because their window exceeds the 8 MB that RFC 9659 requires HTTP decoders to support. The body-selection rules (minimum size, already-compressed content types, caller-setContent-Encoding) are the same as for the other algorithms.compression.zstd. Older versions use the officialbackports.zstdpackage through a new optionalzstdextra (pip install "apify-client[zstd]"), following the pattern of thebrotliextra.import apify_clientstill works. Only accessingZstdHttpCompressoror passingcompression='zstd'raises a clearImportError.try_importnow treats a missing parent package of a dotted dependency name as the dependency being missing, and acceptsextra_name=Nonefor dependencies no extra can install.[tool.ty.analysis] allowed-unresolved-importslistsbackports.zstd, because ty checks against Python 3.11 even in 3.14+ environments, where the package is not installed.ValueError, like the existing Python compressors, rather than the JSArgumentValidationError.