Adding plan model migration support

[joshhaug]

• Tickets addressed: AERIE-1530
• Review: By file
• Merge strategy: Merge (no squash)

Description

This PR implements backend support for migrating plans from one model to another. It is designed to be used in conjunction with aerie-ui PR #1663, which provides frontend support.

Summary of Changes

This PR adds two Hasura functions:

• check_model_compatability checks whether a plan with _plan_id can be migrated to model _new_model_id by querying for:
  • any activity directives in the plan with a type that does not exist in the new model
  • any activity directives in the plan with parameter schema mismatches to the activity type in the new model
  • This function returns these stats to the UI to display. There may another way to format this info that will result in more useful messages to the user (e.g. which parameters are mismatched? which activity types are missing?).
• migrate_plan_to_model checks for open merge requests, and if none exist, it:
  • creates a uniquely-named snapshot of the plan, then
  • updates the plan to a the new model

Both of these functions have fine-grained permissions; only a plan owner or collaborator can perform these queries.

Verification

Ran tests in conjunction with the aerie-ui changes to upload incompatible and compatible banananation models.

Documentation

I believe the docs will need to be uploaded to include these new Hasura functions.

Future work

• Better formatting for migration checks
• The checks I listed above + sim-level args are all I could think of that could pose an issue for a plan model migration, but there very well could be others.

[dandelany]

@mattdailis & I created an updated version of the Banananation model in order to test edge cases of mission model migration - attached here banananation-v2.jar.zip as it will be useful for future testing. It has the following changes vs. the Banananation model in the repo:

• added newOptionalParameter to BiteBanana (as optional parameter with default)
• addednewRequiredParameter (string) to ChangeProducerActivity
• removed growingDuration from GrowBanana
• deleted an activity type - removed BananaNapActivity
• in PeelBanana - changed parameter PeelDirection from enum type to string
• PickBanana - changed quantity from number to string
• in bakeBananaBread - changed temperature to struct (w an enum) from double
• DurationParameterActivity - renamed to NewDurationParameterActivity
• in LineCount - renamed "path" input to "file" (Path object)

[dandelany]

I pushed the change to the validator mentioned above - now when you migrate the model, it automatically shows the errors in the new plan when you open it.

Handing this off to @AaronPlave for further testing & identifying any more issues... Others we noted:

• the table on Change Mission Model Modal should have a "version" column
• when showing expected conflicts, should either show more info, or not be selectable
  • track down what are the expected conflicts - some may not actually be

[mattdailis]

If anyone wants to fiddle around with the modified banananation activity types, I've pushed them to a branch here: feature/plan-model-migration--testing

[AaronPlave]

If anyone wants to fiddle around with the modified banananation activity types, I've pushed them to a branch here: feature/plan-model-migration--testing

@mattdailis and others – Any thoughts on how we might store this sort of altered bananation modal for end to end testing in the UI (not sure if the backend needs such a model for testing). Easy enough to store this altered model as it is now in the UI repo but if the model needs to be updated in the future things could get weird.

[AaronPlave]

This may be a different chunk of work but is there a way to determine which model was used for a simulation? Now that a plan can change models at any time there is no stable reference to a model ID used for a simulation (if I'm not mistaken).

[joshhaug]

Added plan snapshot support in d4ecbe169dea727786603f3ca938cebe7e995324. See https://github.com/NASA-AMMOS/aerie/pull/1665#discussion_r2046988746 for the bug this fixes.

[joshhaug]

@AaronPlave some breaking changes to the backend, both concerning the check_model_compatability function:

• The function now takes parameters (_old_model_id integer, _new_model_id integer) and does not require the _plan_id.
• The output of this function is now a json object like:

{
    "removed_activity_types": {}
    "altered_activity_types": { "BiteBanana" : {"old_parameter_schema" : {"biteSize": {"order": 0, "schema": {"type": "real"}}}, "new_parameter_schema" : {"biteSize": {"order": 0, "schema": {"type": "real", "metadata": {"unit": {"value": "m"}}}}, "secondParameter": {"order": 1, "schema": {"type": "real", "metadata": {"unit": {"value": "m"}}}}}} } 
}

If you'd like me to update the UI for these changes just lmk, I'm happy to take a stab at it.

[AaronPlave]

@AaronPlave some breaking changes to the backend, both concerning the check_model_compatability function:

• The function now takes parameters (_old_model_id integer, _new_model_id integer) and does not require the _plan_id.
• The output of this function is now a json object like:

{
    "removed_activity_types": {}
    "altered_activity_types": { "BiteBanana" : {"old_parameter_schema" : {"biteSize": {"order": 0, "schema": {"type": "real"}}}, "new_parameter_schema" : {"biteSize": {"order": 0, "schema": {"type": "real", "metadata": {"unit": {"value": "m"}}}}, "secondParameter": {"order": 1, "schema": {"type": "real", "metadata": {"unit": {"value": "m"}}}}}} } 
}

If you'd like me to update the UI for these changes just lmk, I'm happy to take a stab at it.

Thanks! I'll take a crack at it and will let you know if I run into any issues.

[AaronPlave]

@AaronPlave some breaking changes to the backend, both concerning the check_model_compatability function:

• The function now takes parameters (_old_model_id integer, _new_model_id integer) and does not require the _plan_id.
• The output of this function is now a json object like:

{
    "removed_activity_types": {}
    "altered_activity_types": { "BiteBanana" : {"old_parameter_schema" : {"biteSize": {"order": 0, "schema": {"type": "real"}}}, "new_parameter_schema" : {"biteSize": {"order": 0, "schema": {"type": "real", "metadata": {"unit": {"value": "m"}}}}, "secondParameter": {"order": 1, "schema": {"type": "real", "metadata": {"unit": {"value": "m"}}}}}} } 
}

If you'd like me to update the UI for these changes just lmk, I'm happy to take a stab at it.

Thanks! I'll take a crack at it and will let you know if I run into any issues.

@joshhaug Seeing an issue where altered_activity_types and removed_activity_types seem to be strings instead of actual json objects.

Also would it be possible to add in a list of directives that are impacted for the plan? My gut here is that users would probably be after getting a sense of how many problems they will have when migrating and for each category (altered and removed), which types are involved plus a count of those types. Could let users dive a little deeper into which parameters have actually changed but that feels out of scope. The general diff between two models is definitely useful but I don't think it is quite enough in the context of a specific plan when trying to figure out if migration is roughly feasible.

[joshhaug]

@AaronPlave There's another function available as of 11466fa8ca665624d806ae5f56187ae1b90b8bbb, function name is check_model_compatability_for_plan and takes a plan id and new model id. It returns something like the following:

{
  "data": {
    "check_model_compatability_for_plan": {
      "result": {
        "removed_activity_types": [],
        "altered_activity_types": {
          "BiteBanana": {
            "old_parameter_schema": {
              "biteSize": {
                "order": 0,
                "schema": {
                  "type": "real"
                }
              }
            },
            "new_parameter_schema": {
              "biteSize": {
                "order": 0,
                "schema": {
                  "type": "real",
                  "metadata": {
                    "unit": {
                      "value": "m"
                    }
                  }
                }
              },
              "secondParameter": {
                "order": 1,
                "schema": {
                  "type": "real",
                  "metadata": {
                    "unit": {
                      "value": "m"
                    }
                  }
                }
              }
            }
          }
        },
        "problematic_directives": [
          {
            "activity_directive": {
              "id": 3,
              "plan_id": 1,
              "name": "BiteBanana",
              "source_scheduling_goal_id": null,
              "created_at": "2025-05-01T22:54:59.010601+00:00",
              "created_by": "josh",
              "last_modified_at": "2025-05-01T22:54:59.010601+00:00",
              "last_modified_by": "josh",
              "start_offset": "09:18:35.044",
              "type": "BiteBanana",
              "arguments": {},
              "last_modified_arguments_at": "2025-05-01T22:54:59.010601+00:00",
              "metadata": {},
              "anchor_id": null,
              "anchored_to_start": true
            },
            "issue": "altered"
          }
        ]
      }
    }
  }
}

[AaronPlave]

While I was testing with the UI branch I discovered that I was able to make a merge request from a branch with a different model than the parent and successfully merge that branch. Not sure if this has been talked about in a comment somewhere or if this behavior is desired (probably not?) so wanted to mention it here to see if the backend could prevent those merge requests from being submitted/merged.

[joshhaug]

While I was testing with the UI branch I discovered that I was able to make a merge request from a branch with a different model than the parent and successfully merge that branch. Not sure if this has been talked about in a comment somewhere or if this behavior is desired (probably not?) so wanted to mention it here to see if the backend could prevent those merge requests from being submitted/merged.

We do check to see whether there are any open merge requests when performing migration, but we don't check for this specific use case. Is it reasonable to say that merges should only be allowed between plans with the same model_id?

[joshhaug]

Added model_id column to simulation_dataset in dfbe4a12b1dad072565f60a53d97432d9f75af50. This may need some extra review, I modified the trigger function to auto-populate this field but I was unable to find other locations in the codebase where this is being explicitly set.

[mattdailis]

Is it reasonable to say that merges should only be allowed between plans with the same model_id?

I think that's a reasonable restriction for the time being. The remedy is to migrate one or both plans to the same model prior to initiating a merge - that seems like a straightforward story to tell users

[Mythicaeda]

Is it reasonable to say that merges should only be allowed between plans with the same model_id?

I think that's a reasonable restriction for the time being. The remedy is to migrate one or both plans to the same model prior to initiating a merge - that seems like a straightforward story to tell users

I agree, and it's one of the blocking issues I left in my initial review

[mattdailis]

A question from demos: what happens if a model is deleted?

[Mythicaeda]

A question from demos: what happens if a model is deleted?

This is ill-defined in Aerie in general, but the general current practice is to set null rather than cascade-delete

[dandelany]

@mattdailis & I did some work on this yesterday and:

• Added a backend check to disallow beginning a merge request if the branches have different model_ids
• Fixed most of the migration checks, though a few "down" checks are still broken

Additionally, @joshhaug has added (since last review):

• tracking model_id in simulation dataset
• updating compatibility check to check plan compatibility, so it only shows relevant errors

@Mythicaeda I think all of your "majors" have been addressed, we just need to wrap up fixing any remaining broken tests

[dandelany]

@Mythicaeda will review final DB changes

@dandelany will work on fixing remaining pgcmp-down test failures

@joshhaug will look into E2E test failure - per @Mythicaeda we suspect a missing cast to jsonb on a string entry.

[joshhaug]

@Mythicaeda if all your comments are resolved, would you mind adding an approval? 🙏

[Mythicaeda]

@joshhaug Yes, I would mind adding an approval, because the DB migrations are not passing, meaning the DB code is not ready. Additionally, I know that you will need to update your DB code following the merge of https://github.com/NASA-AMMOS/aerie/pull/1689, as that will update the plan snapshot functions, which your code touches.

[dandelany]

As of now all checks are passing on this branch, blockers for merging this are:

• #1689 should be merged when ready
• After that merges, the migration number on this branch will need to be incremented one more time & any new pgcmp failures fixed - & then I will approve
• Per standup @Mythicaeda will double check that her blocker issues have been addressed & then she will approve

[joshhaug]

@Mythicaeda @dandelany PR has been rebased/updated to include the latest changes

[dandelany]

@Mythicaeda I've addressed all of your blockers above and most of the smaller ones - there are a few remaining, mostly testing-related, that would be good to discuss with @joshhaug next week. Tests are passing and this is ready for another look from you

[joshhaug]

Ready for a final look from @Mythicaeda!

[joshhaug]

Addressed all remaining comments 👍

[sonarqubecloud[bot]]

Quality Gate failed

Failed conditions
11.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud