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

feat: implement agent global state #400

Merged
merged 29 commits into from
Apr 29, 2023

Conversation

Jshen123
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Apr 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
agent-gpt ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2023 7:46pm

Comment on lines 14 to 15
setIsAgentStopped: () => void;
setAgent: (newAgent: AutonomousAgent | null) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

an interface of

removeAgent: () => void;
setAgent: (newAgent: AutonomousAgent) => void;

Might make more sense. Remove agent would agent.stop(), setAgent(null) and also update isAgentStopped

src/pages/index.tsx Outdated Show resolved Hide resolved
@@ -115,7 +114,6 @@ const Home: NextPage = () => {

const handleStopAgent = () => {
agent?.stopAgent();
setIsAgentStopped();
Copy link
Contributor Author

@Jshen123 Jshen123 Apr 29, 2023

Choose a reason for hiding this comment

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

@asim-shrestha removed setIsAgentStopped() since there is already a useEffect hook that update IsAgentStopped whenever agent is removed

useEffect(() => {
setIsAgentStopped();
}, [agent, setIsAgentStopped]);

Copy link
Contributor

@awtkns awtkns left a comment

Choose a reason for hiding this comment

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

looking good! I will wait for @asim-shrestha though

Comment on lines 70 to 82
export const [AGENT_STATUS_RUNNING, AGENT_STATUS_STOPPED] = [
"running" as const,
"stopped" as const,
];

const AgentStatusSchema = z.union([
z.literal(AGENT_STATUS_RUNNING),
z.literal(AGENT_STATUS_STOPPED),
z.literal(""),
]);

export type AgentStatus = z.infer<typeof AgentStatusSchema>;

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a hard to work with. Any reason we cant do the below?

Suggested change
export const [AGENT_STATUS_RUNNING, AGENT_STATUS_STOPPED] = [
"running" as const,
"stopped" as const,
];
const AgentStatusSchema = z.union([
z.literal(AGENT_STATUS_RUNNING),
z.literal(AGENT_STATUS_STOPPED),
z.literal(""),
]);
export type AgentStatus = z.infer<typeof AgentStatusSchema>;
export type AgentStatus = "running" | "stopped"

Copy link
Contributor Author

@Jshen123 Jshen123 Apr 29, 2023

Choose a reason for hiding this comment

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

made it more concise 🎉

export const [AGENT_STATUS_RUNNING, AGENT_STATUS_STOPPED] = [
"running" as const,
"stopped" as const,
];
export type AgentStatus =
| typeof AGENT_STATUS_RUNNING
| typeof AGENT_STATUS_STOPPED;

keepingAGENT_STATUS_RUNNING and AGENT_STATUS_STOPPED because the strings "running" and "stopped" are referenced in many places.

onClick={handleStopAgent}
enabledClassName={"bg-red-600 hover:bg-red-400"}
>
{shouldAgentStop ? (
{!isAgentStopped && agent === null ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this makes semantic sense. The above is saying:
"If the agent is not stopped and the agent exists then show stopping"
^ not sure that is right semantically

Copy link
Contributor Author

@Jshen123 Jshen123 Apr 29, 2023

Choose a reason for hiding this comment

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

agreed, I could be wrong but this might be outdated code and imo we should just remove this completely.

my understanding is that this is handling a state where agent has been shutdown (i.e. set to null) but the agent?.isRunning has not been set to false

In today's code, agent?.isRunning and setAgent(null) happen simultaneous. I don't think this state would happen

stopAgent() {
this.sendManualShutdownMessage();
this.isRunning = false;
this.shutdown();
return;
}

CC: @asim-shrestha

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah lets refactor this after

@asim-shrestha asim-shrestha merged commit cec2531 into main Apr 29, 2023
2 checks passed
@asim-shrestha asim-shrestha deleted the feat/implement_agent_global_state branch May 29, 2023 04:48
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.

None yet

3 participants