Skip to content

Conversation

@whoiskatrin
Copy link
Collaborator

@whoiskatrin whoiskatrin commented Oct 31, 2025

Downloading 3.11.14 directly from the source, image size is 1GB now

@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2025

🦋 Changeset detected

Latest commit: c31dd16

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudflare/sandbox Patch

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 31, 2025

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@183

commit: 42e9f5a

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2025

🐳 Docker Image Published

FROM cloudflare/sandbox:0.0.0-pr-183-4d9a1d2

Version: 0.0.0-pr-183-4d9a1d2

You can use this Docker image with the preview package from this PR.

@whoiskatrin whoiskatrin changed the title build Python 3.11.11 build Python 3.11.14 Oct 31, 2025
@claude
Copy link
Contributor

claude bot commented Oct 31, 2025

Claude Code Review

Status: Issues found - recommend addressing before merge

Critical Issues

1. Potential cache mount permission issues (line 82)

The cache mount at /tmp/python-cache may cause permission issues in multi-user CI environments. Cache mounts can persist incorrect permissions between builds.

Recommendation: Use --mount=type=cache,target=/tmp/python-cache,uid=0,gid=0 or avoid caching the download since it's a one-time fetch with wget -nc.

2. Incomplete cleanup (line 100)

rm -rf /tmp/cpython-* only removes files matching that pattern, but the downloaded tarball remains in /tmp/python-cache (cache mount). This doesn't affect final image size but is inconsistent.

Recommendation: Either rely on cache mount cleanup or explicitly clean up within the RUN layer.

Minor Issues

3. --no-cache-dir with cache mount (line 142)

Using --mount=type=cache,target=/root/.cache/pip WITH pip3 install --no-cache-dir is contradictory. The --no-cache-dir flag prevents pip from creating the cache, making the cache mount ineffective.

Recommendation: Remove --no-cache-dir flag to utilize the pip cache mount for faster rebuilds.

4. Image size regression

PR description mentions "image size is 1GB now" - this is a significant increase. Previous approach likely used system packages (~200-300MB lighter). While pre-built Python may offer performance benefits, the size trade-off should be justified.

Recommendation: Document WHY this approach was chosen (performance? version pinning? reliability?) in the PR description or commit message.

Positive Changes

  • SHA256 verification adds security ✓
  • Multi-arch support properly implemented ✓
  • Cleaner Node.js installation via COPY from official image ✓
  • Good use of cache mounts in most places ✓

Testing Recommendations

Before merge, verify:

  1. Multi-arch builds succeed (amd64 + arm64)
  2. Python 3.11.14 version is correct: python3 --version
  3. Python packages install correctly: python3 -c 'import matplotlib, numpy, pandas, IPython'
  4. Image size is acceptable for your use case (check with docker images)
  5. E2E tests pass with new image

@cloudflare cloudflare deleted a comment from claude bot Oct 31, 2025
whoiskatrin and others added 3 commits October 31, 2025 16:50
The file command is required by FileService for MIME type detection.
Without it, all file read operations fail with 500 errors.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@whoiskatrin whoiskatrin marked this pull request as ready for review October 31, 2025 18:13
@jsj
Copy link

jsj commented Oct 31, 2025

Verified this fixed my problem

docker run --rm cloudflare/sandbox:0.0.0-pr-183-4d9a1d2 bash -lc "python3 -m pip install --quiet openai-agents && python3 - <<'PY'
from agents import Agent
print('openai-agents import ok')
PY"
# openai-agents import ok

Copy link
Member

@ghostwriternr ghostwriternr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Just removed the python source cleanup, to avoid unexpected issues with production python codebases that we can't yet anticipate. Can prune later if size becomes a bigger concern.

@ghostwriternr ghostwriternr enabled auto-merge (squash) November 3, 2025 16:20
@ghostwriternr ghostwriternr merged commit ff2fa91 into main Nov 3, 2025
8 of 9 checks passed
@ghostwriternr ghostwriternr deleted the update-python branch November 3, 2025 16:35
@threepointone threepointone mentioned this pull request Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants