Add text and line buffering mode to sandbox exec

[azliu0]

Describe your changes

Resolves WRK-424. In Sandbox.exec, this change gives users the option to switch between byte and text reads. This change also gives users the option to switch between streaming output and single lines at a time.

Changelog

Sandbox.exec now accepts arguments text and bufsize for streaming output, which controls text output and line buffering.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 1ff8ea6 in 1 minute and 22 seconds

More details

• Looked at 198 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. test/io_streams_test.py:152

• Draft comment:
  The function test_stream_reader_bytes_mode should be defined as an asynchronous function to use await on stdout.read().
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment seems to be incorrect because the stdout.read() is used in a synchronous context, and there is no evidence that it should be awaited. The pattern in the file shows synchronous test functions, which supports the idea that the current implementation is correct. Without evidence that stdout.read() must be awaited, the comment should be removed.
  I might be missing specific knowledge about the StreamReader class and its methods. If read() is indeed asynchronous, the comment would be valid.
  The context of the file and the usage pattern suggest that read() is synchronous. Without explicit evidence that read() is asynchronous, the comment should be considered incorrect.
  The comment should be deleted because there is no strong evidence that stdout.read() needs to be awaited, and the function is consistent with the rest of the file's pattern.

2. test/sandbox_test.py:236

• Draft comment:
  The function test_sandbox_stdout_bytes_mode should be defined as an asynchronous function to use await on p.stdout.read().
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment seems to suggest a change that is not required. The function is consistent with the synchronous style used in other parts of the file. The use of await is not needed for the functionality being tested, which is reading from a stream in bytes mode. The comment does not provide strong evidence for a necessary change.
  I might be missing some context about the Sandbox class and its methods, but based on the provided code, the function seems correctly implemented as a synchronous function.
  The function is consistent with the synchronous style used in other parts of the file, and the use of await is not needed for the functionality being tested.
  The comment should be deleted as it suggests an unnecessary change. The function is correctly implemented as a synchronous function.

Workflow ID: wflow_01RUp5IRhbIJbNnQ

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 8831064 in 1 minute and 5 seconds

More details

• Looked at 198 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. test/io_streams_test.py:179

• Draft comment:
  The read method is asynchronous and should be awaited.

            assert await stdout.read() == b"foo\n"

• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is about a change made in the diff, specifically the new test function. However, without the implementation of StreamReader, it's speculative to assume read is asynchronous. The comment lacks strong evidence and could be incorrect.
  I might be missing the context of StreamReader's implementation, which could confirm if read is asynchronous. Without this, the decision is based on incomplete information.
  Given the rules, if there's no strong evidence from the diff itself, the comment should be removed. The implementation details of StreamReader are not provided, so the comment remains speculative.
  Remove the comment due to lack of strong evidence that read is asynchronous. The comment is speculative without the implementation context.

2. test/sandbox_test.py:242

• Draft comment:
  The read method is asynchronous and should be awaited.

    assert await p.stdout.read() == b"foo\n"

• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment suggests a change that would require the function to be asynchronous, which is not the case here. The read method is used synchronously in other parts of the file, indicating that it might not be asynchronous. The comment might be incorrect if the read method is indeed synchronous in this context.
  I might be missing the context of whether the read method is asynchronous in other parts of the codebase. However, based on the current file, it seems to be used synchronously.
  The context provided in the file suggests that the read method is used synchronously, and there is no indication that it should be awaited in this specific test function.
  Delete the comment as it suggests an incorrect change. The read method is used synchronously in this context, and the function is not asynchronous.

Workflow ID: wflow_0v83ONiV1cTtTVPq

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[freider]

Some thoughts. I don't have the background on this change so let me know if something doesn't make sense :)

[freider]

Am I missing something or is by_line not implemented here? 🤔 Should perhaps add a test for that too (if we want to keep it)

[azliu0]

Am I missing something or is by_line not implemented here?

It gets passed into the ContainerProcess handle, which is subsequently passed into the StreamReaders.

[ellipsis-dev]

❌ Changes requested. Incremental review on 47ca039 in 43 seconds

More details

• Looked at 133 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_6lOOMGS7BMhCCY4P

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 5d8af6d in 1 minute and 14 seconds

More details

• Looked at 392 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. modal/container_process.py:79

• Draft comment:
  The return type of returncode should be Optional[int] instead of int because it can be None if the process hasn't finished yet.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment seems incorrect because the 'returncode' property raises an error if '_returncode' is 'None', ensuring that 'returncode' will always return an 'int'. Therefore, the return type 'int' is appropriate.
  I might be missing some context about how 'returncode' is used elsewhere, but based on the provided code, the comment seems incorrect.
  The code clearly shows that 'returncode' will not return 'None', so the return type 'int' is correct.
  The comment should be deleted because the return type 'int' for 'returncode' is appropriate given the current implementation.

2. modal/io_streams.py:122

• Draft comment:
  The return type of read should be Optional[T] instead of T because it can return None when EOF is reached.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment suggests changing the return type to Optional[T], but the function read does not return None; it returns an empty string or bytes. Therefore, the comment is incorrect because the function's behavior does not align with the suggested change. The return type T is appropriate as it covers both str and bytes, and the function does not return None.
  I might be missing some context about how None is handled elsewhere in the code, but based on the provided code, read does not return None.
  The function's implementation clearly shows that it returns an empty string or bytes, not None, so the comment is incorrect.
  The comment is incorrect because the read function does not return None; it returns an empty string or bytes. Therefore, the return type T is appropriate, and the comment should be deleted.

3. modal/io_streams.py:292

• Draft comment:
  The return type of __anext__ should be Optional[Union[bytes, str]] instead of Union[bytes, str] because it can return None when EOF is reached.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment seems incorrect because __anext__ does not return None; it raises StopAsyncIteration when None is encountered. This is the expected behavior for async iterators, so the return type should not include Optional.
  I might be missing some context about how __anext__ is expected to behave in this specific codebase, but based on standard Python behavior, the comment seems incorrect.
  The standard behavior of async iterators is to raise StopAsyncIteration rather than return None, so the comment is likely incorrect.
  The comment should be deleted because it incorrectly suggests a change to the return type of __anext__.

Workflow ID: wflow_eRLN6WcYvMUYoNIa

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[azliu0]

@pawalt @freider This is ready for another look. The changes in https://github.com/modal-labs/modal/pull/17149, https://github.com/modal-labs/modal/pull/17148 make it so that raw bytes are streamed back to the client by default (and should be fully rolled out at this point). I've opted not to add these to Sandbox.create as doing the same for the Sandbox logs touches more fundamental parts of the system—I've created a ticket to support this eventually, though.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 2ca0906 in 14 seconds

More details

• Looked at 49 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. test/io_streams_test.py:168

• Draft comment:
  Ensure the type hint for stdout matches the usage of text=False by using StreamReader[bytes].
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_wWnmQiVp1Vzo88M6

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on edc4e88 in 13 seconds

More details

• Looked at 18 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/io_streams.py:302

• Draft comment:
  The comment on line 302 is redundant. The type of the value is already checked in the code.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The comment on line 302 is redundant and doesn't add value. The type of the value is already checked in the code.

Workflow ID: wflow_BXSOWZWHCdFuoWwX

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 15edece in 15 seconds

More details

• Looked at 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/sandbox.py:415

• Draft comment:
  Changing the default value of bufsize from 1 to -1 is a breaking change. Consider documenting this change clearly in the changelog or documentation to inform users.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_w6047SUMzP30n39n

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 5fea951 in 29 seconds

More details

• Looked at 193 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_9tItyC03Zb51awjw

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on b0d4a6f in 15 seconds

More details

• Looked at 31 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/io_streams.py:21

• Draft comment:
  Changing file_descriptor from api_pb2.FileDescriptor.ValueType to int might cause issues if the expected type is not an integer. Ensure this change aligns with the expected input types.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_5wcifeyHN746QRQe

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[freider]

Some comments/questions

[ellipsis-dev]

❌ Changes requested. Incremental review on 1b1582d in 49 seconds

More details

• Looked at 191 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. modal/io_streams.py:287

• Draft comment:
  The return type of __anext__ should be Union[str, bytes] to accommodate both text and byte modes.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The current implementation uses a TypeVar T which is already defined to be either str or bytes. The return type of __anext__ is correctly annotated as T, which covers both cases. The comment suggests using Union[str, bytes], but this is redundant given the definition of T. The comment does not suggest a necessary change, as the current implementation is correct.
  I might be overlooking a subtlety in the use of TypeVar T versus Union[str, bytes], but given the context, T is appropriately defined and used. The comment does not provide strong evidence for a required change.
  The use of TypeVar T is a common pattern in Python typing to allow for flexible return types, and it is correctly applied here. The comment does not add value as the current implementation is already correct.
  The comment is not necessary because the current return type annotation using TypeVar T is correct. The comment should be deleted.

2. modal/io_streams.py:296

• Draft comment:
  The __anext__ method should handle the case where value is already a string, as it might be when text is True and the object type is sandbox.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_XRLuf0v7uBrXUtJs

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 4933489 in 19 seconds

More details

• Looked at 176 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_GIuCCrJ6S7WxVFXW

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 4d72c1c in 41 seconds

More details

• Looked at 29 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. modal/io_streams.py:215

• Draft comment:
  Update the docstring to reflect that _get_logs now returns Optional[bytes] instead of Optional[Union[bytes, str]].
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. modal/container_process.py:150

• Draft comment:
  Document the usage pattern of writing to stdin and then calling drain() in the function docstring.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The _handle_input function in modal/container_process.py writes data to stdin and then calls drain(). This is a correct usage pattern, but it should be documented in the function docstring for clarity.

Workflow ID: wflow_WN7KCFA1qbKZvfG6

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 93daa6d in 26 seconds

More details

• Looked at 22 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/sandbox.py:413

• Draft comment:
  The default value for text should be True to match the implementation.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment seems to be suggesting a change that has already been made in the diff. The default value for 'text' is set to 'True' in the first overload, which matches the implementation. The second overload correctly sets it to 'False'. The comment is redundant because the change has already been implemented.
  I might be missing the context of why the comment was made, but based on the diff, the change aligns with the implementation, making the comment unnecessary.
  The diff clearly shows that the default values have been set correctly, so the comment is not needed.
  The comment should be deleted because the change it suggests has already been implemented in the diff.

Workflow ID: wflow_yC1rH7clBkKkBXbi

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 76ec1bb in 21 seconds

More details

• Looked at 24 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/container_process.py:154

• Draft comment:
  The TODO comment indicates that handling non-zero exit statuses is not working. Consider addressing this issue or updating the comment to reflect the current state of the code.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The comment on line 150 mentions a TODO that is not addressed in this PR. It is important to ensure that the code is either fixed or the comment is updated to reflect the current state of the code.

Workflow ID: wflow_G4XncANOU5nUUCmp

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[azliu0]

@freider this should be ready for another look. I've verified locally that static type checkers work correctly in type_assertions.py. Let me know if there's anything else you'd like to see changed!

[ellipsis-dev]

👍 Looks good to me! Incremental review on f8d99bc in 16 seconds

More details

• Looked at 42 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/io_streams.py:263

• Draft comment:
  The docstring should mention that this method now yields bytes instead of strings, as the line buffer and messages are handled as bytes.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The change from string to bytes in the line buffer is consistent with the rest of the code changes. However, the docstring should reflect that the method now yields bytes instead of strings.

Workflow ID: wflow_lSVsAXJLacCeSkdb

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[freider]

I think you need to add type annotations to the Sandbox.stdout properties etc. as well - I'm getting Any as the type on those, and it would be good to add some actual assertions to the test file as regression tests. Here is what I added at the end of the type_assertions.py to find the issue (could add some more I suppose):

# check sandboxes
sandbox = modal.Sandbox.create("dummy")
assert_type(sandbox.stdout.read(), str)

cmd = sandbox.exec("other")
typing_extensions.reveal_type(cmd)
assert_type(cmd.stdout.read(), str)

cmd2 = sandbox.exec("other_bin", text=False)
assert_type(cmd2.stdout.read(), bytes)

What about stdin btw? Should/is that also affected by the text flag?

[ellipsis-dev]

👍 Looks good to me! Incremental review on 48b7034 in 41 seconds

More details

• Looked at 55 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. test/supports/type_assertions.py:76

• Draft comment:
  sandbox.stdout.read() should be awaited to get the correct type. Use await sandbox.stdout.read() if inside an async function.
• Reason this comment was not posted:
  Marked as duplicate.

2. test/supports/type_assertions.py:79

• Draft comment:
  cmd.stdout.read() should be awaited to get the correct type. Use await cmd.stdout.read() if inside an async function.
• Reason this comment was not posted:
  Marked as duplicate.

3. test/supports/type_assertions.py:82

• Draft comment:
  cmd2.stdout.read() should be awaited to get the correct type. Use await cmd2.stdout.read() if inside an async function.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment seems to address a potential issue with the use of cmd2.stdout.read(). If cmd2.stdout.read() is an asynchronous function, the comment is valid and should be kept. However, without additional context or documentation, it's unclear if this function is asynchronous. The comment is specific and actionable if correct.
  The comment assumes cmd2.stdout.read() is asynchronous without providing evidence. It could be incorrect if cmd2.stdout.read() is synchronous, in which case the comment would be misleading.
  The comment is specific about the need to await the function, which suggests it might be based on knowledge of the function's behavior. However, without explicit evidence, it's safer to assume the comment might be incorrect.
  Delete the comment unless there is strong evidence that cmd2.stdout.read() is asynchronous and requires awaiting.

Workflow ID: wflow_CFKdG3xgT3IiRTN9

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on f0b3bd4 in 25 seconds

More details

• Looked at 36 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. modal/io_streams.py:278

• Draft comment:
  __aiter__ should return an async iterator, not self. Consider returning self._stream instead.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment seems to misunderstand the purpose of __aiter__. In an async iterable class, __aiter__ typically returns self if the class implements __anext__, which is the case here. The suggestion to return self._stream is incorrect because self._stream is an async generator, not an async iterator object that can be used directly in an async for loop.
  I might be missing some context about how self._stream is used elsewhere, but based on the provided code, the current implementation of __aiter__ seems correct.
  The code provided shows that self is indeed an async iterator, as it implements __anext__. Therefore, returning self in __aiter__ is appropriate.
  The comment is incorrect because __aiter__ should return self in this context, as self is an async iterator. The comment should be deleted.

Workflow ID: wflow_Kfb6YtsXTI5r44bf

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[azliu0]

Thanks, added the tests. By default, stdin accepts any type. This behavior seems ok to me for now, as we're only adding the text flag to exec and not Sandbox.create, and retaining the optionality for both seems better—also, modifying the behavior to restrict input type would be backwards-incompatible.

[freider]

Thanks for the cleanup! Looks great!

I realized we don't actually check the implementation types unless we add the file to the pyright allowlist so I tried that locally and added a new PR on top of this one that fixes the few outstanding type annotations (found no implementation issues). I'll merge that after you merge this 👍