agent spawn child subprocess not grandchild #2583
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @rkoumis, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses an issue where the agent was creating the analyzer as a grandchild process instead of a direct child process. To resolve this, I've replaced the multiprocessing.Process() call with subprocess.Popen() in agent/agent.py. Additionally, I've updated the test case in agent/test_agent.py to reflect this change, ensuring that the mock subprocess is now a subprocess.Popen object instead of a multiprocessing.Process object. I also updated the way the exit code is retrieved to use poll() instead of exitcode.
Highlights
- Process Spawning: Replaced
multiprocessing.Process()withsubprocess.Popen()to ensure the analyzer is a direct child process of the agent. - Test Update: Updated the test case to mock
subprocess.Popeninstead ofmultiprocessing.Processand usepoll()for exit code retrieval. - Exit Code Retrieval: Changed the method of retrieving the exit code from
exitcodetopoll()inagent/agent.py.
Changelog
- agent/agent.py
- Replaced
multiprocessing.Processwithsubprocess.Popenfor spawning the analyzer process. - Modified the
spawnfunction to usesubprocess.Popenwith specified stdout, stderr, and shell arguments. - Changed
async_subprocess.exitcodetoasync_subprocess.poll()to get the subprocess exit code.
- Replaced
- agent/test_agent.py
- Imported the
subprocessmodule. - Updated the test case
test_get_subprocess_259to mocksubprocess.Popeninstead ofmultiprocessing.Process. - Modified the test case to use
mock_subprocess.pollinstead ofmock_subprocess.exitcode.
- Imported the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A child process, not grandchild,
Spawned with Popen, no longer beguiled.
Tests updated with care,
Exit code to compare,
Agent's task, now reconciled.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request addresses an issue where the agent was creating the analyzer as a grandchild process instead of a child process. The changes involve using subprocess.Popen() instead of multiprocessing.Process() to spawn the analyzer process. The code changes seem reasonable and address the issue described in the pull request description.
Summary of Findings
- Process status check: The code uses
async_subprocess.poll()to check the subprocess status. It's important to ensure that this method is appropriate for all platforms and provides accurate status information. - Error handling: The code lacks explicit error handling for the
subprocess.Popen()call. It's crucial to handle potential exceptions during subprocess creation to prevent unexpected behavior. - Missing return statement: The
spawnfunction does not have a return statement, which may lead to unexpected behavior. It's recommended to return a success or failure indicator.
Merge Readiness
The pull request addresses a valid issue and the code changes seem to resolve it. However, there are some concerns regarding error handling and the return statement of the spawn function. I recommend addressing these issues before merging. I am unable to directly approve this pull request, and other reviewers should review and approve this code before merging.
| run_subprocess_args = [args, cwd, base64_encode, shell] | ||
| proc = multiprocessing.Process(target=background_subprocess, name=f"child process {args[1]}", args=run_subprocess_args) | ||
| proc.start() | ||
| proc = subprocess.Popen(args, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell) |
There was a problem hiding this comment.
Consider adding error handling for the subprocess.Popen() call. What happens if the subprocess fails to start? Should we catch the exception and update the agent's state accordingly?
try:
proc = subprocess.Popen(args, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell)
except Exception as e:
state["status"] = Status.FAILED
state["description"] = f"Failed to start subprocess: {e}"
return json_error(500, f"Failed to start subprocess: {e}")| @@ -378,7 +378,7 @@ def get_subprocess_status(): | |||
| """Return the subprocess status.""" | |||
| async_subprocess = state.get("async_subprocess") | |||
| message = "Analysis status" | |||
| exitcode = async_subprocess.exitcode | |||
| exitcode = async_subprocess.poll() | |||
There was a problem hiding this comment.
Consider adding a comment explaining why poll() is used instead of exitcode here. Also, it might be good to check if poll() is the right method for all platforms.
# Use poll() to get the exit code of the subprocess.
# This is more reliable than accessing async_subprocess.exitcode directly.
exitcode = async_subprocess.poll()| run_subprocess_args = [args, cwd, base64_encode, shell] | ||
| proc = multiprocessing.Process(target=background_subprocess, name=f"child process {args[1]}", args=run_subprocess_args) | ||
| proc.start() | ||
| proc = subprocess.Popen(args, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell) | ||
| state["status"] = Status.RUNNING | ||
| state["description"] = "" | ||
| state["async_subprocess"] = proc |
There was a problem hiding this comment.
The function doesn't return anything. Should it return a success or failure indicator?
state["async_subprocess"] = proc
return json_success("Successfully spawned command", process_id=proc.pid)
|
Thanks a lot - will test tomorrow 👍 |
|
Unfortunately it's not quite working yet.... Seems to bomb when launching auxiliary modules: Not sure why disguise is problematic but not the preceding modules... |
|
I don't see any signs of an issue in the logs @kevoreilly dropped above. I exercised the basic execpy agent workflow, didn't see any problems. I'll have a look at the full analysis workflow with a proper analysis package and capemon and see what shakes out. |
|
I tried removing I also removed the following snippet: to see if I could spot the error in the console output. Alas all that it showed was the status get requests from the server, nothing from the analyzer. I'm not quite sure what I need to do to get it to spit the verbose output to the console. |
The agent is creating the analyzer as a grandchild process rather than just a child.
The analyzer protects its own pid and its parent.
Use
subprocess.Popen()instead ofmultiprocessing.Process()