-
Notifications
You must be signed in to change notification settings - Fork 5.2k
MOD: close subprocess and its file handles when reset #476
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
Conversation
Would this also be desired for %undo if there were new handle openings that were undone? |
I think if the code-interpreter subprocess is already started before %undo, whether to shut down this child process or not, depending on preferences for code.
I would suggest a Chat session as a management unit to prevent certain context failures. But, eventually a decision needs to be made by the owner of this code repository. |
interpreter/code_interpreter.py
Outdated
|
||
def stop(self): | ||
""" | ||
Stops the interpreter process. | ||
""" | ||
if self.proc: | ||
self.proc.terminate() | ||
self.proc.stdin.close() | ||
self.proc.stdout.close() |
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 these changes probably need to move to interpreter/code_interpreters/subprocess_code_interpreter.py
now that the code has been refactored.
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.
Of course, I'll come back to modify this commit.
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 do you think about turning off stdin and stdout directly in the terminate method?
interpreter/interpreter.py
Outdated
for code_interpreter in self.code_interpreters.values(): | ||
code_interpreter.stop() |
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 this needs to move to interpreter/core/core.py
now that the codebase has been refactored.
Hey there, @coderfengyun! I think this is a really great suggestion and am happy to help resolve the merge conflicts and move the code to the refactored codebase if you want to hand it off. I think we just need to add the |
Changes were made as per your REVIEW comments. @ericrallen @AncalagonX |
Great work @coderfengyun, very straightforward and helpful! Merging now, then renaming Long time coming and I very much appreciate your edits post-refactor. Thank you @ericrallen for your work here too. |
Thanks to all the contributors to open-interpreter for giving us the opportunity to explore the use of code-interpreter. What I did was make a modest contribution. |
MOD: close subprocess and its file handles when reset Former-commit-id: d3c6da9 Former-commit-id: 8d162229bec7bdc656a40e21a6da37e563cc6329 Former-commit-id: 966267c33f55016cedee8552dcd77797701d81a1 [formerly 08860dd1f6b01ec0019e562aab8b339ebd5b97a9] Former-commit-id: bcd10fa587b5d33067641f4bd9874571b7d0fd47
MOD: close subprocess and its file handles when reset
Describe the changes you have made:
Problem I encountered: When I use the same Interpreter instance to complete multiple code-writing tasks in a row within a process, I try to use Interpreter.reset() to reset the context and Interpreter state between each task. However, after several runs, I ran into the problem of too many open file handles.
Solution I provide: So in this pr, Interpreter.reset closes the child process and closes its open file handles.
Reference any relevant issue (Fixes #000)
I have tested the code on the following OS:
AI Language Model (if applicable)