fix(python-sdk): keep get_transport accepting the deprecated http2 argument - #1668
fix(python-sdk): keep get_transport accepting the deprecated http2 argument#1668beran-t wants to merge 2 commits into
Conversation
…gument
Any fresh `pip install e2b-code-interpreter` currently raises on the first
`run_code()`:
TypeError: get_transport() got an unexpected keyword argument 'http2'
`e2b_code_interpreter/code_interpreter_sync.py` builds its streaming client with
`get_transport(self.connection_config, http2=False)`. Moving the REST API client
onto pyqwest (#1601) dropped that parameter, correctly, since ALPN negotiates
the HTTP version now and the flag no longer selects anything. But
e2b-code-interpreter pins `e2b>=2.26.0,<3.0.0`, so pip and poetry resolve the
broken pair by default, and 2.9.0, 2.8.1, 2.8.0 and 2.7.0 all call it the same
way. e2b 2.37.0 works, 2.38.0 does not.
`get_transport` is not re-exported at the package root, so it reads as internal,
but a sibling package we ship depends on it and the version range says the
combination is supported. Accepting the argument and ignoring it costs nothing
(it is genuinely inert) and unbreaks every already-published code-interpreter
without anyone having to upgrade two packages in lockstep.
Verified end to end: with this patch installed alongside an unmodified
e2b-code-interpreter 2.9.0, `Sandbox.create().run_code(...)` returns normally
against live E2B.
The follow-up belongs in e2b-dev/code-interpreter, which should move off the
REST transport entirely: that call site is envd streaming traffic, so
`get_envd_transport(config, for_streaming=True)` from #1623 is what it actually
wants, and its `e2b` floor should then move to >=2.38.
🦋 Changeset detectedLatest commit: 50c8d94 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Package ArtifactsBuilt from 59f82bf. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.38.4-fix-get-transport-http2-compat.0.tgzCLI ( npm install ./e2b-cli-2.16.2-fix-get-transport-http2-compat.0.tgzPython SDK ( pip install ./e2b-2.38.0+fix.get.transport.http2.compat-py3-none-any.whl |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a minimal, well-targeted compatibility fix that resolves a real breakage for every currently-published e2b-code-interpreter install.
What was reviewed: the http2 kwarg addition to both get_sync_transport/get_async_transport, confirming it's a no-op accepted only for backward-compat (not silently changing HTTP/2 negotiation behavior); parity between the sync and async implementations; and the new test's coverage of keyword, positional, and omitted forms. The one issue found (missing try/finally cleanup in the new test) is a test-hygiene nit with no functional impact, not a correctness concern.
Extended reasoning...
Overview
This PR adds back an optional, ignored http2: Optional[bool] = None parameter to get_transport in both packages/python-sdk/e2b/api/client_sync/__init__.py and client_async/__init__.py, plus a changeset and one new test. It's a compatibility shim: the pyqwest migration (#1601) dropped the parameter since ALPN now negotiates HTTP version, but every published e2b-code-interpreter still calls get_transport(config, http2=False), so the accepted e2b version range resolves to a broken combination by default (a TypeError on the first run_code()).
Security risks
None. The change only widens a function signature to accept and discard an additional argument — it does not alter request construction, TLS/ALPN behavior, auth, or any data handling. No new attack surface.
Level of scrutiny
Low-to-moderate is appropriate here. This is a narrowly-scoped compatibility fix to an internal (non-root-exported) function, symmetric across sync/async as required by this repo's conventions, accompanied by a changeset and a new test exercising keyword, positional, and omitted-argument call forms. The PR description includes a verified end-to-end repro against a real published e2b-code-interpreter version.
Other factors
The only finding from this review run is a nit (inline comment) about the new test not following the file's established try/finally cleanup convention for resetting transport caches — functionally harmless since every other test in the file resets the caches at its own start. This doesn't rise to a level that should block the fix, which addresses an actively broken install path for users. No CODEOWNER-sensitive paths or security-critical code are touched.
|
|
||
|
|
||
| def test_get_transport_accepts_the_deprecated_http2_kwarg(): | ||
| """Every published e2b-code-interpreter calls | ||
| ``get_transport(config, http2=False)``. The pyqwest move dropped that | ||
| parameter, which turned the first ``run_code()`` of any fresh | ||
| ``pip install e2b-code-interpreter`` into a TypeError, because | ||
| e2b-code-interpreter's ``e2b>=2.26.0,<3.0.0`` range resolves to a version | ||
| that no longer accepts it. The flag is inert now (ALPN negotiates), so it | ||
| is accepted and ignored rather than removed. | ||
| """ | ||
| reset_sync_api_transports() | ||
| reset_async_api_transports() | ||
| config = ConnectionConfig(api_key="test-key") | ||
|
|
||
| assert get_sync_transport(config, http2=False) is get_sync_transport(config) | ||
| assert get_async_transport(config, http2=False) is get_async_transport(config) |
There was a problem hiding this comment.
🟡 The new test_get_transport_accepts_the_deprecated_http2_kwarg test only calls reset_sync_api_transports()/reset_async_api_transports() at the top, unlike every other transport-creating test in this file which wraps the same resets in a try/finally so cleanup runs even after assertions. This leaves a cached PyqwestTransport/AsyncPyqwestTransport in the module-global _transports dicts after the test finishes. Wrapping the assertions in try/finally with the two resets (as test_sync_get_transport_keyed_by_proxy does) would match the file'''s established convention.
Extended reasoning...
What the bug is: packages/python-sdk/tests/test_api_client_transport.py has an established, uniform convention across its ~26 tests: any test that calls get_transport/get_sync_transport/get_async_transport resets the module-global transport caches (reset_sync_api_transports() / reset_async_api_transports()) both before creating a transport and again after, with the trailing reset (and any client close) wrapped in a finally block so it always runs. test_sync_get_transport_keyed_by_proxy, test_sync_api_client_proxy_uses_explicit_transport, test_sync_transport_sends_multipart_bodies, and all their async counterparts follow this shape.
Where it breaks: The new test_get_transport_accepts_the_deprecated_http2_kwarg (lines 613-629) only calls reset_sync_api_transports()/reset_async_api_transports() once, at the top, before creating any transports. It never resets afterward and has no try/finally. As a result, the PyqwestTransport/AsyncPyqwestTransport instances it creates (each backed by its own retrying_http_transport connection pool) stay cached in the module-global _transports dicts after the test completes, instead of being cleared like every sibling test does.
Why nothing currently prevents it: pytest itself has no enforcement of this convention — it'''s purely a manual pattern the test authors have followed consistently, and nothing in the test file or fixtures asserts cleanup happened. Since this is (per current file ordering) the last test in the module, there is also no later test whose leading reset happens to paper over the leak within this run.
Impact: Functionally harmless in the current suite: every sibling test resets the caches at its own top before using them, so a stale cached transport from this test does not cause incorrect behavior or failures elsewhere. The impact is purely a broken convention plus a leaked (never-closed) pooled transport object sitting in a global dict for the remainder of the process — a latent test-hygiene issue rather than a correctness bug.
Step-by-step proof:
- Test starts:
reset_sync_api_transports()andreset_async_api_transports()clearclient_sync._transports/client_async._transports(both empty dicts). get_sync_transport(config, http2=False)is called — since the cache is empty, it builds a newPyqwestTransport(wrapping a freshretrying_http_transportconnection pool) and stores it inclient_sync._transports[proxy].- Likewise,
get_async_transport(config, http2=False)builds and caches a newAsyncPyqwestTransportinclient_async._transports[proxy]. - The four assertions run and pass.
- The test function returns. Unlike every sibling test, there is no
finallyblock callingreset_sync_api_transports()/reset_async_api_transports()again, soclient_sync._transportsandclient_async._transportsstill hold live transport objects when the test session moves on — the sole exception to the file'''s otherwise-universal teardown pattern.
Fix: Wrap the four assertions in a try/finally, with reset_sync_api_transports() and reset_async_api_transports() in the finally block, mirroring test_sync_get_transport_keyed_by_proxy, which also creates no httpx client and still follows this shape.
Every other transport-creating test in test_api_client_transport.py resets the module-global transport caches in a finally block; the new one only reset at the top, leaving a live pooled transport in _transports after it finished. Also takes the test_api_key fixture instead of hardcoding a key, matching its neighbours.
|
Good catch on the cleanup convention - fixed in 50c8d94. Wrapped the assertions in |
|
The fix is up on the other repo, but let me check |
|
superseded by #1671 |
2 participants
Summary
Any fresh
pip install e2b-code-interpretercurrently raises on the firstrun_code():e2b_code_interpreter/code_interpreter_sync.py:84builds its streaming client withget_transport(self.connection_config, http2=False). Moving the REST API client onto pyqwest (#1601) dropped that parameter — correctly, since ALPN negotiates the HTTP version now and the flag no longer selects anything. Bute2b-code-interpreterdeclarese2b>=2.26.0,<3.0.0, so pip and poetry resolve the broken pair by default.e2b2.37.0 works, 2.38.0 does not, and code-interpreter 2.9.0, 2.8.1, 2.8.0 and 2.7.0 all call it the same way — so there is no currently-publishable combination a user gets by default that works.This accepts
http2again on both the sync and asyncget_transport, documented as deprecated and ignored.get_transportis not re-exported at the package root, so it reads as internal — but a sibling package we ship depends on it, and the declared version range says the combination is supported. Since the flag is genuinely inert, accepting and ignoring it costs nothing and unbreaks every already-published code-interpreter without users having to upgrade two packages in lockstep.Repro, before and after:
Verified end to end: with this branch installed alongside an unmodified
e2b-code-interpreter==2.9.0, that same script returns['hello\n']against live E2B. The existingtests/test_api_client_transport.pysuite passes (26 tests), plus one added case covering the keyword, positional and omitted forms.The real follow-up belongs in
e2b-dev/code-interpreterrather than here: that call site is envd streaming traffic, soget_envd_transport(config, for_streaming=True)from #1623 is what it actually wants, and itse2bfloor should move to>=2.38once it does. This PR is the compatibility half, so existing installs stop failing in the meantime. Happy to close it in favour of a code-interpreter-only fix if you would rather not carry the shim — the tradeoff is that everyone stays broken until they upgrade both packages.