Skip to content
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

Feature/iss 198 sub process #90

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

shide1989
Copy link
Contributor

@shide1989 shide1989 commented Dec 11, 2024

🚀 Pull Request Overview

What does this PR do? 🤔

This PR allows an AI agent to start and check the status and logs of a background process.
For ex, you can start a docker build, and check the status a few minutes later, the agent will have all the info (Process ID, Logs etc.)

Related Issues 📝

💻 Changes Summary

  • Process Management: updated the run_shell action to allow a process to be ran in the background.
    • NB: added the sync argument for the run_shell command. This way AI Agents less tend to create commands that run in the background by themselves. See Engine for more details.
  • Query: We now send the status and logs of of the active processes
  • Workspace: The workspace config now stores the background running processes info.
  • Logs: A background process's logs are stored under the .2501/logs directory. This might be subject to change

✅ Checklist

  • I have tested that the feature works on OpenAI and Llama drivers.
  • We will need to make some adjustments for Windows.
  • I Will test on more drivers

📈 Impact Analysis

  • Affected modules: Query command mainly.
  • Breaking changes: No. Retro-compatible friendly

@@ -134,3 +149,49 @@ export async function browse_url(args: { url: string }) {
${md.replace(/\s+/g, '')}
`;
}

export async function check_process_status(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now this check_process_status function is unused, I'm thinking about keeping it or not since it can help the agent to retrieve the info faster 🤔

.split('\n')
.find((line) => line.includes(`export UNIQUE_PROCESS_ID=${uniqueID}`));
if (!line) {
throw new Error('Child process not found');
Copy link
Contributor Author

@shide1989 shide1989 Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we should throw an error, or simply return a shellProcess with a 'terminated' status, to inform that the process has been terminated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for now I would rather have the CLI send an error in some edge cases, than having a silent error and having to dig the code to find why some shellprocesses would be terminated unexpectedly

@shide1989 shide1989 requested a review from alxpereira December 11, 2024 14:55
@BonnardValentin
Copy link
Contributor

must be tested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants