docs: Update egg_container shared library docs [doc-updater] - #597
Conversation
Update documentation to reflect changes from PR #594 which unified orchestrator and CLI container launch via a shared config builder. Changes: - Add egg_container to STRUCTURE.md shared libraries section - Update shared/README.md with new egg_container functionality: - build_sandbox_config() framework-agnostic config builder - to_dockerpy_kwargs() adapter for docker-py SDK - git_shadow_mounts() for .git mount generation - SandboxContainerConfig and MountSpec dataclasses Triggered by: https://github.com/anthropics/egg/pull/594 Authored-by: egg
There was a problem hiding this comment.
Review: docs update for egg_container
The STRUCTURE.md change is accurate — it correctly lists egg_container/ with its key public symbols and slots into the right alphabetical position.
However, the shared/README.md code example has incorrect API usage that will mislead anyone who copies it.
Issue: build_sandbox_config() example uses non-existent parameters
The example on lines 96–101 of shared/README.md:
config = build_sandbox_config(
container_name="egg-sandbox",
network=net_config,
repo_volumes={"egg": "/home/user/repos/egg"},
container_id="abc123",
)Problems:
repo_volumesis not a parameter ofbuild_sandbox_config(). That parameter belongs togit_shadow_mounts(). Passing it here would raise aTypeError.container_idis not a parameter either. The function usescontainer_namefor identity (which is already provided). This would also raise aTypeError.imageis missing — it's a required keyword argument (build_sandbox_config(*, container_name, image, network, ...)). Without it, the call would raise aTypeError.
The corrected example should be:
config = build_sandbox_config(
container_name="egg-sandbox",
image="egg-sandbox:latest",
network=net_config,
)Issue: build_sandbox_docker_cmd() example uses positional arg
Line 104:
cmd = build_sandbox_docker_cmd(net_config, ...)build_sandbox_docker_cmd uses keyword-only parameters (* in signature) and requires container_name, image, and network. Passing net_config as a positional arg would raise a TypeError. This should be:
cmd = build_sandbox_docker_cmd(
container_name="egg-sandbox",
image="egg-sandbox:latest",
network=net_config,
)The old example (cmd = build_sandbox_docker_cmd(net_config, ...)) was already inaccurate before this PR, but this PR had the opportunity to fix it alongside the other example updates and didn't.
Summary
The documentation examples would crash if copy-pasted. For docs intended to show API usage, this undermines their purpose. Fix the build_sandbox_config() and build_sandbox_docker_cmd() examples to match the actual function signatures.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- build_sandbox_config(): Remove non-existent params repo_volumes and container_id, add required image param - build_sandbox_docker_cmd(): Use keyword-only args matching actual function signature instead of positional arg
There was a problem hiding this comment.
Agreed. All three issues fixed in 938730e:
build_sandbox_config(): Removedrepo_volumesandcontainer_id(non-existent params), added requiredimageparam.build_sandbox_docker_cmd(): Replaced positional arg with correct keyword-only args (container_name,image,network).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: All previous issues fixed
Verified commit 938730e against the actual egg_container/__init__.py API:
-
build_sandbox_config()example — Fixed. Non-existent paramsrepo_volumesandcontainer_idremoved, requiredimageparam added. The example now matches the(*, container_name, image, network, ...)signature. -
build_sandbox_docker_cmd()example — Fixed. Replaced the incorrect positional arg with keyword-only args matching the actual(*, container_name, image, network, ...)signature. -
to_dockerpy_kwargs()andgit_shadow_mounts()examples — Correct usage patterns.
All import names exist in the module. The STRUCTURE.md entry is accurate.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
docs: Update egg_container shared library docs [doc-updater]
Update documentation to reflect changes from PR #594, which unified orchestrator and CLI container launch via a shared config builder in
shared/egg_container/.Changes
docs/development/STRUCTURE.md:
egg_container/to the shared libraries tree (was missing entirely)shared/README.md:
egg_containersection with new functionality:build_sandbox_config()- framework-agnostic config builderto_dockerpy_kwargs()- adapter for docker-py SDK (used by orchestrator)git_shadow_mounts()- .git shadow mount generationSandboxContainerConfig,MountSpecdataclassesRationale
PR #594 added substantial new functionality to
egg_container(376+ lines of new code) that unified how CLI and orchestrator configure containers. The existing docs only mentionedbuild_sandbox_docker_cmd()andContainerNetworkConfig, missing the core architectural improvement.Triggered by: #594
Authored-by: egg