NASA-AMMOS / aerie

A software framework for modeling spacecraft.
https://nasa-ammos.github.io/aerie-docs/
MIT License
73 stars 19 forks source link

Procedural Scheduling #1496

Closed skovati closed 2 months ago

skovati commented 4 months ago

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`: ```sql primary key (goal_invocation_id), ``` `goal_analysis`: ```sql primary key (analysis_id, goal_invocation_id) ``` `goal_analysis_created_activities`: ```sql primary key (analysis_id, goal_invocation_id, activity_id) ``` `goal_analysis_satisfying_activities`: ```sql 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](https://github.com/user-attachments/assets/abbe5576-a4dd-4bc7-8c28-6947fe4e68ec)

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 commented 4 months ago

Yep, working on them right now using CI tests

skovati commented 4 months ago

@Mythicaeda Ah I didn't push my latest commit with comments / cleanup. Will push when @JoelCourtney is finished force pushing the bug fix

mattdailis commented 3 months ago

I've gone ahead and rebased this branch to clean up the history. The original history is preserved at https://github.com/NASA-AMMOS/aerie/compare/develop...feature/procedural-scheduling--pre-rebase

This link I think is the best summary for what has changed since @Mythicaeda last reviewed. (based on the timestamps, I think 724263c1a is a good reference point). The biggest difference is I split the one database change into two steps: first add goal invocations, and then add the rest of the procedural scheduling machinery. https://github.com/NASA-AMMOS/aerie/compare/724263c1a..feature/procedural-scheduling

dandelany commented 2 months ago

Met with @skovati and @mattdailis and we reviewed all of @Mythicaeda 's remaining unresolved comments.

Those marked with a 👍 reaction are considered in scope and either already fixed, or will be on Monday. Those marked with 👀 will be split off into a new future PR & are out of scope for this release - these are mainly related to fleshing out our e2e tests, and while we all agree there are improvements to be made, it's not worth holding up the release of this feature. We'll work on improved tests starting next week.

We noticed that after our latest round of fix commits, the tests are back to being broken - we'll defer release until Monday & @mattdailis will look at fixing these.

@Mythicaeda let me know if you want to tag up on any specifics

Mythicaeda commented 2 months ago

@dandelany I'm specifically concerned with completing a basic test suite being considered "out of scope" for a major feature. The fact that the existing tests have been easy to break via changes this whole PR makes me explicitly concerned that there may be bugs that were added during the review but we're unaware of because the PR was "extensively manually tested" around when it was opened.

I understand that we're trying to get this feature out urgently, which is why I kept the scope of my test requests limited.