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

[AERIE-1629] Computed Attributes in Simulation Results #25

Merged

Conversation

mattdailis
Copy link
Collaborator

@mattdailis mattdailis commented Jan 12, 2022

  • Tickets addressed: AERIE-1629
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

I've taken #17 and split it into three branches. I'll PR them sequentially, so we can focus on one at a time.

EDIT: If it would help declutter, I can make a separate PR for the refactoring commits. Let me know if anyone would prefer that.

The focus of this PR is reporting a value as part of simulation results upon completion of an activity instance.

The commits in this PR fall into one of the following phases

  1. Preliminary refactoring (first 5 commits)
  2. Use Supplier instead of Runnable to represent an effect model
  3. Generate mapper and supplier for EffectModels with non-void return types
  4. Reporting:
    • Include ValueSchema in ActivityType
    • Include return value in SimulatedActivity
    • Persist all of that in postgres

We continue to support void return types as before. These activities will have a SerializedValue.UNIT return value.

Verification

I've done some simple manual testing by giving BiteBanana a non-void return type and viewing it via hasura. Some more thorough verification is needed prior to merging.

  • more thorough testing. (unit test?)

Documentation

This work is part of the Activity Computed Attributes working group: https://wiki.jpl.nasa.gov/display/MPSA/Working+Group%3A+Activity+Computed+Attributes

  • We should update some user-facing documentation to explain how to use this feature.

Future work

Step 2: (AERIE-1630) Make this value available to the caller
Step 3. (AERIE-1631) Tag events with activity ids

@mattdailis mattdailis force-pushed the feature/AERIE-1629--computed-attributes-in-sim-results branch from 298597d to 5bff3b3 Compare January 12, 2022 00:47
Copy link
Contributor

@goetzrrGit goetzrrGit left a comment

Choose a reason for hiding this comment

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

I am starting to understand more about this part of AERIE. It is all those walkthroughs you gave me :) Just a general question what are activity computed attributes? Visually I add BiteBanana to the plan, simulate, and get back values that affect the graph/resource at the bottom of the plan. Are those values you get back from the simulation activity computed attributes

Maybe we don't have an example yet?

I give this PR a loose approve someone with more experience can veto this though

@mattdailis
Copy link
Collaborator Author

mattdailis commented Jan 12, 2022

I am starting to understand more about this part of AERIE. It is all those walkthroughs you gave me :)

🥳 yay, let's do more of those, for all the different things people are working on

Just a general question what are activity computed attributes? Visually I add BiteBanana to the plan, simulate, and get back values that affect the graph/resource at the bottom of the plan. Are those values you get back from the simulation activity computed attributes

Yeah, sort of. The charter for our working group described it as:

Currently, the only observable behaviors of an activity are its duration, decomposition, resources affected, and events emitted. This working group seeks to identify use cases for allowing an activity to have a more directly observable "output", and come to a consensus for how this new feature could best fit into Aerie.

So, what you currently see in the UI is resources. Activities emit events, which are interpreted into effects, some of which are serialized and output as resource profiles in simulation results. These resources are "global" in that there is one continuous "profile" that describes the value of that resource at any given time in the simulation.

There are some use cases for which this "global" property of resources is inconvenient - if you have multiple overlapping activities affecting the same resource, it can be difficult to discern the behavior of each individual activity.

The idea of "activity computed attributes" is that we are allowing an activity to return some arbitrary value upon completion. This value will be included in simulation results alongside other attributes of the activity, such as its input parameters, start time duration, etc.

Maybe we don't have an example yet?

Correct - I haven't included an example in this PR, perhaps I should. I've been testing by going into BiteBanana, and changing the return type of run to something else (I usually do boolean for simplicity, but it can also be more complex). Then I recompile banananation and try running a simulation (either via the banananation test, or via the UI). The returnValue should be part of the simulation results, which you can look at through hasura.

Having written that up, I acknowledge that testing this PR is a bit of a pain 😅 . If I have some time later this week I'll see if I can add an example and make it more streamlined.

@goetzrrGit
Copy link
Contributor

@mattdailis thanks for the clarification. This helps a lot!!!

@pcrosemurgy
Copy link
Contributor

more thorough testing. (unit test?)

Would be cool to test this out in an example adaptation like foo-missionmodel. FooActivityTest maybe?

@mattdailis mattdailis force-pushed the feature/AERIE-1629--computed-attributes-in-sim-results branch from 5bff3b3 to 43239aa Compare January 18, 2022 16:20
@mattdailis mattdailis force-pushed the feature/AERIE-1629--computed-attributes-in-sim-results branch from 43239aa to 66a7b7e Compare January 18, 2022 17:31
@mattdailis mattdailis force-pushed the feature/AERIE-1629--computed-attributes-in-sim-results branch from 53ee9e8 to 12a2b2a Compare February 1, 2022 21:34
@Twisol
Copy link
Contributor

Twisol commented Feb 1, 2022

I might not be able to re-review, so feel no need to wait on me 😃 If you've addressed what you think you need to address, 🚀

span is meant to be general, and does not necessarily correspond with
activity instances. The comment on type says "The type of span,
implying the shape of its attributes." There isn't much need to limit
attributes to merlin_argument_set.
@mattdailis mattdailis force-pushed the feature/AERIE-1629--computed-attributes-in-sim-results branch from 12a2b2a to e12d2db Compare February 1, 2022 22:26
@mattdailis
Copy link
Collaborator Author

mattdailis commented Feb 1, 2022

Final force-push was a minor field-name change from "ActivityType.returnValueSchema" to "ActivityType.computedAttributesValueSchema".

Thanks for all the feedback and numerous iterations, everyone! I think we're in a significantly better place than we were when I first opened this. Looking forward to the possibilities this change opens up in the future 🚀

Merging now.

@mattdailis mattdailis merged commit 117ac6d into develop Feb 1, 2022
@mattdailis mattdailis deleted the feature/AERIE-1629--computed-attributes-in-sim-results branch February 1, 2022 22:29
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.

7 participants