openenv e2b backend: declare the SDK as an extra, floor it at 2.12 - #2809
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
1955fae to
0ba6d08
Compare
0ba6d08 to
0bc7a12
Compare
|
@claude review always |
Nothing said which e2b to install. Releases before 2.12 send the template name as the deprecated `alias`, which self-hosted AgentENV does not read, so every Template.build fails with a bare 400. Add an e2b extra to setup.py (e2b>=2.12) and check the floor in the launch preflight, so a hand-installed or preinstalled old SDK names its version as the cause. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A vendored copy imports fine but has no dist-info, so importlib.metadata.version raised PackageNotFoundError instead of the actionable error this preflight exists to give. The version cannot be verified there, and blocking a possibly-fine install is worse than leaving a too-old one to fail at the provider — skip the check with a printed note. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0bc7a12 to
b643a43
Compare
There was a problem hiding this comment.
Looks good — the unguarded importlib.metadata.version() call I flagged earlier is now wrapped in try/except PackageNotFoundError, falling back to a skip-with-notice instead of raising a confusing traceback, and a new test (test_preflight_skips_floor_without_package_metadata) covers that path directly.
Extended reasoning...
This is a follow-up to my prior review on this PR. My earlier inline comment noted that preflight_sdk's call to importlib.metadata.version(module) was unguarded and would raise PackageNotFoundError (instead of the intended actionable RuntimeError) for SDKs installed without proper dist-info — exactly the "installed by hand or preinstalled in an image" case the docstring calls out as the target scenario.
The subsequent commits (64f94ca, b643a43) address this directly: the version lookup is now wrapped in try/except importlib.metadata.PackageNotFoundError, printing "{module} has no package metadata; cannot verify {module}>={min_version}" and returning early rather than raising. This matches the fix I suggested. A new test, test_preflight_skips_floor_without_package_metadata, exercises this exact path by monkeypatching version to raise PackageNotFoundError and asserting the function returns without error and prints the expected notice.
The rest of the diff is unchanged from what I reviewed before: setup.py's new e2b extra, the sdk_min_version spec field and _version_tuple parser, and the README updates. Tests remain comprehensive (min-version enforcement below/at/above floor, no-floor no-check, and version-tuple parsing including prerelease suffixes like 2.12.0rc1).
Since my only substantive concern is now resolved and I see no other outstanding third-party objections in the timeline (no CHANGES_REQUESTED, and the only review activity is my own), I'm posting a short acknowledgment rather than staying fully silent, since it confirms the fix was verified against the actual diff rather than just trusting the PR description.
approve
Declare the e2b SDK as an extra with a version floor, checked at launch.
Why
The openenv per-episode backends import their provider SDK lazily and nothing in the repo said which version to install; the READMEs said
pip install e2b. With e2b ≤ 2.11 (the last release for Python 3.9 is 2.10.2),Template.buildsends the template name in the deprecatedaliasfield, which a self-hosted AgentENV does not read: every bake fails with a bare400 template name must be provided, and nothing points at the SDK version. 2.12 and later sendnameand work (verified 2026-08-28: e2b 2.46.0, an unbaked task, golden reward 1.0 on the dev sandbox-service cluster).What
setup.py: extrae2b(e2b>=2.12), next to the existingfsdp/mlflow/dashboard. Nothing is added to the core install; the extra is pulled in only by users of that backend.miles.rollout.agentic.credentials.preflight_sdk(the SDK preflight moved there in rollout: shared miles-side layer for agent-function legs (session URL, sandbox credentials) #2805; this PR originally targeted its old home inopenenv_launch_common): an optional minimum version per backend, checked withimportlib.metadataafter the import succeeds. Set for e2b (2.12). This catches an SDK installed by hand or preinstalled in an image, and names the cause in the error. If the module imports but has no dist-info (a vendored copy), the floor check is skipped with a note instead of blocking a possibly-fine install (review nit).pip install -e '<miles>[e2b]'.tests/fast/rollout/agentic/test_credentials.py): the version floor (below → error naming both versions; at/above → passes; no floor → no check; missing metadata → skipped with a note) and the version parser.Verification
CI was green on the pre-rebase
0bc7a125d(pre-commit and all CPU stages, 588 fast tests); rebased onto #2805 and re-pushed asb643a433b, tests re-run locally in the new location. The first push failed one of the suite's own checks —test_credential_spec_is_completerequires each backend's spec to have exactly the known keys — which now admits the optionalsdk_min_version.🤖 Generated with Claude Code