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

Procedural Scheduling #1496

Merged
merged 108 commits into from
Sep 9, 2024
Merged

Procedural Scheduling #1496

merged 108 commits into from
Sep 9, 2024

Conversation

skovati
Copy link
Contributor

@skovati skovati commented Jul 8, 2024

Description

This PR expands upon #1423 and #1299 to introduce the capability for users to define scheduling procedures in Java, upload them to Aerie, include them in a scheduling specification, and execute them as part of a scheduling run.

Closes #1534

Use gov.nasa.ammos instead of gov.nasa.jpl for new packages

In the interest of reducing rework in the upcoming Great Renaming, we've chosen to move all procedural scheduling code into the ammos namespace.

Update timeline library

TODO explain this change

Use ActivityDirectiveId and ActivityInstanceId in timeline

This PR takes the improvements made by #1505 and propagates them into the timeline library.

Return initial sim results if latest is null

Previously, getting the latest simulation results would return Optional.empty() in the case where an initial simulation has been run, but no subsequent simulations have been run. This change makes the initial simulation results available through the same method as subsequent simulation results.

Goal Invocations

This PR allows the same goal_id to appear in a scheduling specification more than once.

Detailed design

Primary Key changes

scheduling_specification_goals:

primary key (goal_invocation_id),

goal_analysis:

primary key (analysis_id, goal_invocation_id)

goal_analysis_created_activities:

primary key (analysis_id, goal_invocation_id, activity_id)

goal_analysis_satisfying_activities:

primary key (analysis_id, goal_invocation_id, activity_id)

Foreign Key changes

created_activites and satisfying_activities now FK to goal_analysis instead of just scheduling_run, since goal_analysis holds the "as run" data for that goal invocation (e.g. arguments, revision).

We were duplicating this data into each of created_activites and satisfying_activites before (i.e. storing goal_revision, goal_id, etc), but I think with the addition of arguments, it makes sense to centralize this into a goal_analysis entry per goal invocation, and then have created_activites and satisying_activities be pure association tables between directives and a goal_analysis entry.

I believe this is how we had hasura metadata set up anyways, this is just codifying this into DB FKs and simplifying this scheduling_run sub-schema

Untitled-2023-11-30-1236

Procedural Scheduling Database Changes

We introduce the notion of a goal type, which can either be EDSL or JAR. A goal definition may now be one of those types. If it is a JAR goal, it will refer to the JAR via a new uploaded_jar_id column.

We also introduce a parameter_schema column to the goal definition, and an arguments column to goal invocations in scheduling_specification_goals.

Hook to update scheduling procedure parameters

Similar to activity types, we add a hasura event trigger on insert of a new scheduling procedure. The scheduler server loads the procedure, extracts the value schema of its parameters, and updates the corresponding database entry.

Procedural Scheduling Java Implementation

Scheduling procedure annotation processor

Scheduling procedure examples

Verification

Documentation

Future work

This PR represents the procedural scheduling MVP. There is a list of features we've deemed out of scope for this work, but would like to start working on immediately after release.

@skovati skovati added the feature A new feature or feature request label Jul 8, 2024
@skovati skovati requested a review from a team as a code owner July 8, 2024 16:44
@skovati skovati self-assigned this Jul 8, 2024
Copy link
Contributor

@Mythicaeda Mythicaeda left a comment

Choose a reason for hiding this comment

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

Reminder: DB migrations are missing.

@skovati
Copy link
Contributor Author

skovati commented Jul 8, 2024

Yep, working on them right now using CI tests

@skovati skovati force-pushed the feature/procedural-scheduling branch from 4447fd2 to b070e2a Compare July 8, 2024 21:56
Copy link
Collaborator

@mattdailis mattdailis left a comment

Choose a reason for hiding this comment

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

Joel walked me through the Kotlin files. We added back isEqualTo to Duration to preserve backwards compatibility, but marked it as @Deprecated. Approval predicated on Dan successfully running another manual test.

Copy link
Collaborator

@dandelany dandelany left a comment

Choose a reason for hiding this comment

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

Thanks all - just to summarize the above:

  • @Mythicaeda has completed review of all Non-Kotlin code & all her feedback has been resolved
  • @skovati added 3 more e2e test cases to cover cases brought up in @Mythicaeda 's review
  • @JoelCourtney @mattdailis @skovati & I reviewed any new Kotlin code that has been modified since the original library PR
  • I ran another manual test locally using this branch + the UI branch + the banananation model + the StayWellFed goal and all seems to be working correctly

I think we're 👍 to merge this finally!

@JoelCourtney JoelCourtney merged commit 605c023 into develop Sep 9, 2024
26 checks passed
@JoelCourtney JoelCourtney deleted the feature/procedural-scheduling branch September 9, 2024 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A change that will require updating downstream code feature A new feature or feature request publish Tells GH to publish docker images for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complex test case for Procedural Scheduling
5 participants