Simplification of the task's output

After few months using MESG Core, I realize that create task’s outputs is a bit overcomplicated.
For now we have the following:

One task has one payload of inputs and multiple possible outputs with their own payload.

This is good to have multiple outputs in a function outputs are mostly (if not all the time) something like success or error.

We could simplify this and just have one output from the task with its own payload. Of course the service could still raise an error.

PRO:

  • Simplification of the mesg.yml
tasks:
  xxx:
    inputs: {...}
    outputs:
      foo: { type: String }
  • Simplification of the libraries
// mesg lib catches empty result and do a big try catch on the function
// also now the task don't have the responsibility to call the the submit result the lib can control only one submit result
mesg.listenTask({
  foo: (inputs: object): object => {}
})

CON:

  • A huge breaking change as all existing services will be broken
  • Need update on the API
  • Need update on the libraries

We could have some way to minimize the impact with some conventions. If you are using success/error then we can automatically assign the success as output and catch the error but that will be ugly.

I totally support this simplification even if it breaks every services!

I would like to add also a PRO argument:

The states of execution will also be simplified and more precise.
Currently, there is a error state only for error occurring inside the Core.
If the error output is managed by the Core, then the Core could set the execution’s state to error as well. It will be a very nice feature to allow a better retry system on execution.

As @Anthony said, the libs should catches error throwed or returned by the task function.
But gRPC api must integrate the error.
The current message containing the result of an execution is:

message SubmitResultRequest {
  string executionID = 1;
  string outputKey = 2;
  string outputData = 3;
}

I suggest to transform it to something like:

message SubmitResultRequest {
  string executionID = 1;
  oneof output {
    string outputData = 3;
    string error = 4;
  }
}
1 Like

I was thinking about this for a while and I agree on this simplification. And since we now support object types in the definition format, it’s still possible for devs create different type of outputs by directly defining them in the output data itself.

One other thing to think about is, we should be able to filter by output types inside the applications. If we remove this feature now, devs will need to do this filtering directly on the data by checking which output presents in the data object.

Also, we don’t have many input data types in events and task inputs too. So having them for only task outputs doesn’t makes too much sense.

I’m not sure to understand. Can you explain more please.

Yes i agree. But this filtering should be done the same way on both task’s output and events!

I’m not sure to understand. Can you explain more please.

Nothing important. I say that we have outputX, outputY etc. in the task results and we don’t have the similar thing for events to accept different inputs like inputY, inputY or for task inputs. So, there is no a good reason to keep this pattern just for task outputs, users can implement their own since we support the object data type now.

According to the new dev roadmap, this proposal is one of the first to be implemented and is blocking for many other dev on the Engine and CLI.

It will be divided in many small PRs that should not break the Engine or introduce bugs. Most of the PRs are dependant to each other, but some are not, so be careful and don’t merge PRs that don’t have to be dependant!
If some modification break other part of the codebase that are not define by the PR, fix it the easiest way to make it work (default value, hardcode stuff, etc…) because it will be updated by a other PR.

@krhubert is responsible for the implementation of this proposal, expect for the update of the JS lib.

EDIT 20th May:
All of the following PR should be merged to the branch feature/single-output (instead of dev) so we can still release the dev branch in case there is bug fix!

update importer package PR #963

  • It should read a single output mesg.yml file and transform it to a multiple output service definition (current service definition).
    This way, only the importer package have to be updated and the rest of the codebase doesn’t.
  • the new mesg.yml should look like:
tasks:
  name:
    name: "Token's name"
    description: "Get the name of a ERC20"
    inputs:
      contractAddress:
        name: "Contract address"
        description: "The address of the contract"
        type: String
    outputs:
      name:
        name: "Token's name"
        description: "The name of the ERC20"
        type: String
  • the outputs in the service definition should be like:
    outputs:
      success:
        name: "Success"
        description: "Output when the task executes successfully"
        data: #the outputs data from the new mesg.yml outputs
          name:
            name: "Token's name"
            description: "The name of the ERC20"
            type: String
      error:
        name: "Error"
        description: "Output when an error occurs"
        data:
          message:
            name: "Message"
            description: "The error message"
            type: String

change internal structure

Dependant on the previous PR.

  • Update the Task.Outputs to type []*Parameter in service package PR #964
  • Remove the Output struct in service package PR #973
  • Update validation logic (most of it should be in service/task.go) PR #973

update execution package

Dependant on the previous PR.

  • remove outputKey PR #975
  • rename outputData to outputs
  • accept error from service when submitting an error

update API package

  • Remove outputKey PR #975
  • Add management of error from the service. (in SubmitResult and processExecution I guess).
  • remove outputKey from filter PR #971
  • remove any hack to use service and execution db

update gRPC APIs

Dependant on the previous PR.

  • Update the service struct in protobuf package
  • Update the SubmitResultRequest to:
message SubmitResultRequest {
  string executionID = 1;
  string outputData = 3;
  string error = 4;
}

maybe with an oneOf on outputData and error.

  • Remove outputFilter from ListenResultRequest
  • Remove outputKey from ResultData

update Go libraries

Dependant on the PR Update APIs.

  • Remove the OutputKey.
  • Add the error management

update JS libraries PR #90

Dependant on the PR Update APIs.

  • Don’t pass outputs callback to the parameter of the task function.
    The task function should return an Object that will be JSONed by the lib before send to the Engine.
    If the task function throw an error, the lib should catch it and send it to the Engine.
    Maybe, the lib could also be compatible with the task function returning a Promise? @core what do you think?

update services

Dependant on the PR Update JS lib for JS service and Go lib for Go service.

  • Update both the mesg.yml and the code of services

update CLI

Dependant on the PR Update APIs.

  • Update the CLI commands to get rid of all output key related stuff.

PR #972

A new thing came up during the update of the Execution and API package:

With PR 975, Executions go to state Failed when the service return an error in the SubmitResult request. This is the good behavior.
The problem is Executions also go to state Failed when errors are returned from: json.Unmarshal and exec.Complete.

  • This have to be fixed by simply returning the error from those function instead of saving them into Executions.
  • If an error occurs, the execution should not be published on the pubsub system

The returned error will be send back to the service as response of the SubmitResult api.

Executions should go to failed state only when the service send an error when calling SubmitResult.

The function SubmitResult, processExecution and saveExecution need to be modified to fix this bad behavior:

To test the features, you can use the service erc20 with branch single-outputs:

./dev-cli service dev https://github.com/mesg-foundation/service-ethereum-erc20/tarball/single-outputs

Then to execute a task without error:

./dev-cli service execute ethereum-erc20 --task name --data contractAddress='0x420167d87d35c3a249b32ef6225872fbd9ab85d2'

Task with error:

./dev-cli service execute ethereum-erc20 --task name --data contractAddress='0x420167d87d35c3a249b32ef6225872fbd9ab85d'

Merged with PR https://github.com/mesg-foundation/engine/pull/974