[DRAFT] Activity computed attributes

[mattdailis]

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

Description

This draft PR includes all the pieces I think are necessary to allow activity instances to have return values. "Having return values" involves two things:

1. Reporting a value as part of simulation results upon completion of an activity instance
2. Make this value available to the caller

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

1. Preliminary refactoring
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 return ValueSchema in ActivityType, and return value in SimulatedActivity, persist in postgres
5. Tag events with activity ids
6. Make return values available to caller
7. Add example activities to show usage (TODO)

Some open questions remain:

• 1. How do we represent void return values? We could use a custom NoReturnValue, Optional.empty, or null.

DECISION: There's no need for NoReturnValue, since the TaskSpecType::serializeReturnValue method returns an Optional.

• 2. Is Scheduler an appropriate place for the getTaskReturnValue method?
• 3. Do these changes maintain our bounded context boundaries appropriately?
• 4. Activities can be called in a Serialized way, such that it is impossible to statically determine what its return type will be. Should we have those return void? Or Object, and let the mission model cast it? Or SerializedValue and let them deserialize?

DECISION: Let's not worry about calling activities in a serialized way, and focus on supporting the case where we do know the type of the child activity.

• 5. Should we serialize and deserialize return value before providing it? That's what we do with parameters, but I think it's strange.
• 6. Should it be possible to raise an exception in a child activity and catch it in the parent? Related to this: should we get rid of waitFor and defer in favor of call?
• 7. What does OneShotTask do? Should it have a return value?

Unrelated to this, but an additional question I have:

• 8. Can we rename parameters to arguments in the getSimulationResults api response?

I've also added a commit tagging events with activity ids, but that'll need some more scrutiny as well (especially with decomposing activities... I'm not sure if child activities have "directives").

Verification

I've done some simple manual testing by giving BiteBanana a non-void return type and viewing it via hasura.

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

Future work

[Twisol]

I haven't dug into the code itself yet (need to find some time), but here are some quick thoughts on the open questions:

Make this value available to the caller

Incidentally, I recall this being more of a stretch goal / future feature than core requirement. Did that change?

DECISION: There's no need for NoReturnValue, since the TaskSpecType::serializeReturnValue method returns an Optional.

Is this lack of return value manifest at the schema level, or could an activity in principle return empty() under some conditions and of(_) under others? To show my hand here, I think an activity type should either always have a return value or never have a return value, and having an Optional<> return seems to allow it to vacillate.

Is Scheduler an appropriate place for the getTaskReturnValue method?

The methods exposed by Scheduler are provided by the driver for use by the model. By the name, it sounds like getTaskReturnValue would be used by the driver, so no. Perhaps naïvely, I'd expect the return value to be threaded through the terminating TaskStatus somehow.

Do these changes maintain our bounded context boundaries appropriately?

Is this a DDD-type question? What bounded context boundaries do you have in view? I suspect my note adjacently above is part of what you're asking.

• Activities can be called in a Serialized way, such that it is impossible to statically determine what its return type will be. Should we have those return void? Or Object, and let the mission model cast it? Or SerializedValue and let them deserialize?

Activity output values should be handled the same as activity input values, IMHO -- they should have an associated mapper that the driver uses to get schema information from statically, and to serialize the actual return value in the same way we serialize effective arguments.

Should we serialize and deserialize return value before providing it? That's what we do with parameters, but I think it's strange.

It is very strange, yes, and one of the refactors I tried to embark upon a few months ago was to make this not be that way anymore. IMHO, I'd recommend following how arguments are handled just to reduce the cognitive load of what all is happening in the engine. I might take a look at refactoring this once my grad school applications are done.

• Should it be possible to raise an exception in a child activity and catch it in the parent? Related to this: should we get rid of waitFor and defer in favor of call?

IMHO (as you know), I think waitFor(child) has very unfortunate interactions with exceptions. Either exceptions escaping an activity boundary should abort the simulation (so the "view from inside" has no exceptions, really), or we should treat exceptions holistically with return values. Under waitFor(child), we will have to (a) hold onto any exception and continue the simulation in case an activity later waits for (and thus accepts) the child's exception, and (b) deal with the possibility of multiple activities waiting on another, and hence having multiple opportunities for the exception to propagate. Without waitFor(child), a child activity is either called by a waiting parent, or spawned with nobody waiting, so we always know immediately what to do with any return value or exception. (That is, there's no need to store the return value or exception for some indeterminate amount of time.)

I think many use-cases involving waitFor(child), like spawning many children and then waiting for them all to complete, can be implemented in terms of the remaining primitives. For instance, var c1 = spawn(_); var c2 = spawn(_); waitFor(c1); waitFor(c2); can be implemented as call(() -> { spawn(_); spawn(_); });. Having a single consistent chain of communication between activities and their descendants makes a number of mechanisms much simpler to implement.

What does OneShotTask do? Should it have a return value?

I think this is related to my questions above about Optional. I'd argue that it should have a return value, whose type is Unit and value is Unit.UNIT, as in enum Unit { UNIT }. It may also be good to have a case in SerializedValue and ValueSchema for this type (i.e. a pair of ValueSchema ofUnit() and SerializedValue ofUnit() factories), as I think that would address my Optional concerns and make it very clear to the client what information they can extract reliably from any activity (including OneShot, for which the answer is "no useful information").

[Twisol]

Without waitFor(child), a child activity is either called by a waiting parent, or spawned with nobody waiting, so we always know immediately what to do with any return value or exception. (That is, there's no need to store the return value or exception for some indeterminate amount of time.)

Also, if an exception is thrown by a task, we will likely want to propagate the exception up the decomposition tree, rather than only looking at the parent task. The "delegation" philosophy of decomposition -- that is, that tasks delegate their responsibilities to subordinates -- would suggest that if a child activity fails, and the parent doesn't handle that exception, the parent should also fail, as it was unable to perform one of its (incidentally, delegated) responsibilities.

This raises the question of what sibling/cousin task should do when their ancestors fault. I'd argue that, by default, the entire subtree under a faulting task should be considered to terminate in fault; it's the duty of an ancestor to catch exceptions if failure is expected and can be tolerated. This is a generalization of the existing behavior (where the entire sim faults on exception). Mechanisms for adjusting this behavior can be considered later, in the presence of concrete use-cases.

[mattdailis]

Archiving this Draft PR and splitting it into three tickets:

1. AERIE-1629: Include activity computed attributes in simulation results
2. AERIE-1630: Make activity compute attributes available to calling activity
3. AERIE-1631: Tag events with activity ids