-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add container filesystem API for sandboxes #2414
base: main
Are you sure you want to change the base?
Conversation
@azliu0 can you convert this to draft until we have worker & server support? Also this will need to be tested somehow. Need to mock the underlying RPCs. |
78b032c
to
cb52f89
Compare
1a4493b
to
c0a8dfc
Compare
Can we improve the changelog a bit here? |
seen_chars.add(char) | ||
|
||
def _handle_error(self, error: api_pb2.SystemErrorMessage) -> None: | ||
if error.error_code == api_pb2.SystemErrorCode.SYSTEM_ERROR_CODE_UNSPECIFIED: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just make this a case in the error mapping
error_class = ERROR_MAPPING.get(error.error_code, FilesystemExecutionError) | ||
raise error_class(error.error_message) | ||
|
||
async def _consume_output(self, exec_id: str) -> AsyncIterator[Optional[bytes]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement retries here similar to how they're implemented in StreamReader
.
lines = cast(str, data).split("\n") | ||
return [line + "\n" for line in lines[:-1]] + ([lines[-1]] if lines[-1] else []) | ||
|
||
async def write(self, data: Union[bytes, str]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we check data size here so we fail before making the req?
self._closed = False | ||
return self | ||
|
||
async def read(self, n: Union[int, None] = None) -> Union[bytes, str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check data size before making the req
@@ -425,6 +426,22 @@ async def exec(self, *cmds: str, pty_info: Optional[api_pb2.PTYInfo] = None): | |||
) | |||
return _ContainerProcess(resp.exec_id, self._client) | |||
|
|||
async def open(self, path: str, mode: str = "r") -> _FileIO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we support the context manager api too?
Describe your changes
Resolves WRK-414 by adding a new container filesystem API.
This API is designed to closely follow
io.FileIO
, supporting most of the major functions (open
,read
,readline
,readlines
,write
,seek
,flush
,close
) along with a few higher-level functions (delete_bytes
,write_replace_bytes
) to help make Sandboxes a bit easier to use.All file modes (e.g. text, bytes, r/w/a/x, etc.) are supported, as well as most common errors/exceptions. If an unknown exception is thrown, then a
modal.exception.FilesystemExecutionError
is thrown instead.Example usage:
Backward/forward compatibility checks
Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Changelog
Sandboxes now support a new filesystem API. The
open()
method returns aFileIO
handle for native file handling in sandboxes.