(fix)Execution fails if the session_bytes is too long #57
Conversation
cc9730c to
6de5cfc
Compare
|
@christian-bromann @eyurtsev @vbarda @hntrl Also, what's the current status of this repo? Is it maintained at all? As I see, the deps weren't updated for the long time |
|
Not actively maintaining ATM. Might change in the future. Feel free to fork and release your own version |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where execution fails when session_bytes becomes too long, causing an "Argument list too long" error when passed as command-line arguments. The fix changes the approach from passing session bytes as CLI arguments to piping them through stdin.
Key changes:
- Modified command building to use
--session-stdinflag instead of passing bytes as arguments - Updated process execution to pipe session bytes through stdin
- Added support for reading session bytes from stdin in the Deno TypeScript code
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
libs/sandbox-py/tests/unit_tests/test_pyodide_sandbox.py |
Added comprehensive test cases for both async and sync execution with large session bytes |
libs/sandbox-py/langchain_sandbox/pyodide.py |
Modified command building and process execution to use stdin piping instead of CLI arguments |
libs/pyodide-sandbox-js/main.ts |
Added stdin reading support and updated session bytes handling to accept both string and binary formats |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| code_create = "big = 'x' * (2 * 1024 * 1024); print(len(big))" | ||
| first = await sandbox.execute(code_create) | ||
| assert first.status == "success", f"Encountered error: {first.stderr}" | ||
| assert first.stdout == str(2 * 1024 * 1024) |
There was a problem hiding this comment.
The magic number 2 * 1024 * 1024 is repeated multiple times. Consider extracting this to a constant like LARGE_SESSION_SIZE = 2 * 1024 * 1024 to improve maintainability and make the test intention clearer.
Copilot uses AI. Check for mistakes.
| code_create = "big = 'x' * (2 * 1024 * 1024); print(len(big))" | ||
| first = sandbox.execute(code_create) | ||
| assert first.status == "success", f"Encountered error: {first.stderr}" | ||
| assert first.stdout == str(2 * 1024 * 1024) |
There was a problem hiding this comment.
The magic number 2 * 1024 * 1024 is repeated multiple times. Consider extracting this to a constant like LARGE_SESSION_SIZE = 2 * 1024 * 1024 to improve maintainability and make the test intention clearer.
Copilot uses AI. Check for mistakes.
| if (flags["session-stdin"]) { | ||
| const chunks: Uint8Array[] = []; | ||
| for await (const chunk of Deno.stdin.readable) { | ||
| chunks.push(chunk as Uint8Array); |
There was a problem hiding this comment.
The type assertion as Uint8Array could potentially fail if the chunk is not actually a Uint8Array. Consider using a type guard or validation to ensure type safety.
| chunks.push(chunk as Uint8Array); | |
| if (!(chunk instanceof Uint8Array)) { | |
| throw new TypeError("Expected chunk to be a Uint8Array from Deno.stdin.readable"); | |
| } | |
| chunks.push(chunk); |
Copilot uses AI. Check for mistakes.
This fixes a bug that happens when the
session_bytesbecome too long due to large state declared.Before the fix, a long
session_byteswould causeOSError: [Errno 7] Argument list too long: 'deno'as the length of command becomes too long (see added test cases).The fix is to use stdin to pipe the bytes from Python to Deno.