-
Notifications
You must be signed in to change notification settings - Fork 74
feat(code-interpreter): Add convenience methods for file operations and package management #202
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
base: main
Are you sure you want to change the base?
Conversation
…nd package management
| result = self.invoke("writeFiles", {"content": [file_content]}) | ||
|
|
||
| # Store description as metadata (available for future LLM context) | ||
| if description and hasattr(self, "_file_descriptions"): |
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.
do we need the hasattr check here? I see _file_descriptions is defined above as {}?
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.
will remove this..
| file_contents.append({"path": path, "text": content}) | ||
|
|
||
| self.logger.info("Uploading %d files", len(files)) | ||
| return self.invoke("writeFiles", {"content": file_contents}) |
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.
didn't follow this. we are taking list of files and there is no batch file upload API in code interpreter? so we should just call upload_file utility we have above?
Also need to see how to handle partial failure in this case. Throwing an exception even if one file upload files should be ok and users will have to retry entire batch. Some advanced support would be to instead return list of responses back and let clients handle which ones to retry with
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.
The writeFiles API does accept a list, so it is a batch operation at the API level. But your point about reusing upload_file and handling partial failures is valid.
Two options:
- Keep as-is since writeFiles API handles the batch natively (simpler, atomic)
- Loop through and call upload_file for each, collect results, and let caller handle partial failures
I'd lean toward Option A for now since the API handles it atomically - either all succeed or all fail. We can add advanced partial-failure handling as a follow-up if users request it.
| path: str, | ||
| content: Union[str, bytes], | ||
| description: str = "", | ||
| ) -> Dict[str, Any]: |
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.
are we not able to strengthen the return type further here? what's the expected shape we plan to return here?
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.
The return type is Dict[str, Any] because invoke() returns that. We could define a TypedDict for the response shape, but it couples us to the API response structure. For now, keeping Dict[str, Any] gives flexibility. Can revisit if we want stricter typing across the SDK.
| self.logger.info("Downloading %d files", len(paths)) | ||
| result = self.invoke("readFiles", {"paths": paths}) | ||
|
|
||
| files = {} |
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 re_use the utility method above to download_file?
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.
Yes, makes sense. Will refactor to reuse download_file
| self, | ||
| code: str, | ||
| language: str = "python", | ||
| clear_context: bool = False, |
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.
minor note: this clear_context is only application for python and is not supported for other languages yet.
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.
Good point. Will update the docstring to clarify this limitation.
| code: The code to execute. | ||
| language: Programming language - 'python', 'javascript', or 'typescript'. | ||
| Default is 'python'. | ||
| clear_context: If True, clears all previous variable state before execution. |
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.
should we consider adding a first class method to clear context? so that users can write code such as:
client.executeCode(`x = 10`)
client.executeCode(`x += 1`)
client.clearContext() // this can be a dummy call we run with clearContext = True
client.executeCode(`x = 1`)
so it's easy and clean to clear context instead of always needing to set it along with another execute code call.
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.
cleaner API. Will add.
| }, | ||
| ) | ||
|
|
||
| def execute_shell( |
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.
should we call this exec() or execute_command() to keep consistency with API?
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.
sure, execute_command matches the API name better. Will rename.
| self.logger.info("Installing packages: %s", packages_str) | ||
| return self.invoke("executeCommand", {"command": command}) | ||
|
|
||
| def download_file( |
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.
should we call this read/write file to keep it mapped to API input names? (we may introduce download/upload for some future operations related to blob storage upload/download)
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.
i think upload_file/download_file are more intuitive from a user perspective than write_file/read_file. The existing invoke('writeFiles') and invoke('readFiles') are the low-level API - these convenience methods add some semantic clarity.
Suggest we keep upload_file/download_file for now, and if we add blob storage later, we can name those upload_to_s3/download_from_s3 or similar.
| identifier (str, optional): The code interpreter identifier. | ||
| session_id (str, optional): The active session ID. | ||
| Basic Usage: |
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.
what if we could have subclasses to organize the utility methods based on:
- file system controls
- code execution controls
- command line controls
so we would end up with:
session.file.read()
session.file.write()
session.code.execute()
session.cmd.execute()
session.cmd.startTask()
session.cmd.getTask()
session.cmd.stopTask()
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 keep this as a follow-up enhancement. Current flat structure works and matches how other SDKs typically expose methods. We can layer subclasses on top in a future version without breaking changes.
| self, | ||
| command: str, | ||
| ) -> Dict[str, Any]: | ||
| """Execute a shell command in the interpreter environment. |
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.
we also have few command task related API's we could have wrappers for
this can also be a follow-up
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.
Agree these would be useful. Already have invoke() paths for these. Can add as follow-up since the core file/code operations are the priority.
Summary
Adds developer-friendly convenience methods to
CodeInterpreterclass, reducing boilerplate for common operations while maintaining full backward compatibility.Motivation
Current usage requires verbose
invoke()calls with manual parameter construction:This PR enables cleaner patterns that match developer expectations:
Changes
New Methods
upload_file(path, content, description)upload_files(files)install_packages(packages, upgrade)download_file(path)download_files(paths)execute_code(code, language, clear_context)execute_shell(command)Security
/etc/passwd→ValueError);,&,|,`,$)Backward Compatibility
✅ No breaking changes - All existing
invoke()patterns continue to work unchanged.Testing
invoke()calls