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(platform) : scheduling agent runner #8634

Merged

Conversation

Abhi1992002
Copy link
Contributor

@Abhi1992002 Abhi1992002 commented Nov 13, 2024

Feature #8461

  • Created a table on the monitor page to display all schedules.

  • This table includes options to filter schedules based on graphs and to create a new schedule.

  • Each graph page now has a button to create a schedule.

Changes 🏗️

  • Removed isEnabled from autogpt_platform/backend/backend/data/schedule.py because it was not fetching the disabled schedules, which prevented them from ending as cron jobs.

  • Added a schedule table on the monitor page to display all schedules:

    • Allows filtering of schedules by agent name.
    • Option to create a new schedule.
  • Used cronExpressionManager.ts to create cron expressions and descriptions for each cron expression.

  • Created types and requests for schedule endpoints.

  • Reused the run button component to enable rescheduling of graphs.

Some Screenshots

Screenshot 2024-11-13 at 5 04 02 PM Screenshot 2024-11-13 at 5 03 36 PM Screenshot 2024-11-13 at 5 04 11 PM

Video

Untitled.video.-.Made.with.Clipchamp.mp4

@Abhi1992002 Abhi1992002 requested a review from a team as a code owner November 13, 2024 12:05
@Abhi1992002 Abhi1992002 requested review from Bentlybro and kcze and removed request for a team November 13, 2024 12:05
@github-actions github-actions bot added platform/frontend AutoGPT Platform - Front end platform/backend AutoGPT Platform - Back end conflicts Automatically applied to PRs with merge conflicts labels Nov 13, 2024
Copy link
Contributor

This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request.

Copy link

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Bug
Removing isEnabled filter from get_active_schedules() query could return unwanted disabled schedules. Verify this change doesn't cause issues with cron job management.

Incomplete Error Handling
The schedule creation and toggle functionality lacks proper error handling and user feedback for failure cases. This could lead to a confusing user experience if API calls fail.

Input Validation
The cron expression generation lacks comprehensive input validation, which could allow invalid cron expressions to be created. Need to validate all input parameters.

Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit efcd558
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/6735dcdbad23f200088bca06

@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Nov 13, 2024
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@Abhi1992002 Abhi1992002 changed the title Feature/scheduling agent runner feat(platform) : scheduling agent runner Nov 13, 2024
@Torantulino
Copy link
Member

Amazing contribution @Abhi1992002, well done!
The would pair very well with #8501

Copy link
Contributor

@majdyz majdyz left a comment

Choose a reason for hiding this comment

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

Thank you for the contributions! I've added a couple of comments related to the API, let me know your thoughts!


const api = useMemo(() => new AutoGPTServerAPI(), []);

const fetchSchedules = useCallback(async () => {
const schedulesData: Schedule[] = [];
for (const flow of flows) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you did this due to non-optimized backend, I've improved the API here so you can do GET /schedules and get all the schedules for a user without firing multiple requests and add more information on the GET response:
#8649

I'm happy with any of the ordering, we can first get this merged and improve it and update the routing on my PR, or you can rebase your PR based on the above PR.

@@ -0,0 +1,239 @@
export class CronExpressionManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be any simpler if we utilize https://www.npmjs.com/package/cron ?

} from "@/components/ui/dialog";
import { Separator } from "./ui/separator";
import { CronExpressionManager } from "@/lib/monitor/cronExpressionManager";
// import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Remove this?

<Separator />
{/*

On the backend, we are using standard cron expressions,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really good idea, I can accommodate this on my API #8649, what field do you think you need for this? I can add end_time on the GET PUT & POST request wdyt?

title: "Agent scheduling successful",
});

// if schduling is done from the monitor page, then redirect to monitor page after successful scheduling
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typo

return this._request("PUT", `/graphs/schedules/${scheduleId}`, update);
}

async getSchedules(graphId: string): Promise<{ [key: string]: string }> {
Copy link
Contributor

Choose a reason for hiding this comment

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

After seeing it used by the client, I think it makes much more sense to make the API route simply /schedules without the graph intervention, I can add graph_id as a query parameter for GET request as an optional filtering clause.

@@ -329,3 +329,19 @@ export type AnalyticsDetails = {
data: { [key: string]: any };
index: string;
};

// Schedule types
Copy link
Contributor

@majdyz majdyz Nov 14, 2024

Choose a reason for hiding this comment

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

nit: the comment, except the cron expression, might not be necessary

@majdyz majdyz disabled auto-merge November 14, 2024 10:26
@Swiftyos
Copy link
Contributor

Just tested it, this is a fantastic contribution @Abhi1992002 Thank you!

@Swiftyos Swiftyos merged commit dd0081a into Significant-Gravitas:dev Nov 14, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/backend AutoGPT Platform - Back end platform/frontend AutoGPT Platform - Front end Review effort [1-5]: 4 size/xl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants