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

Step Functions interpreter #7

Closed
sam-goodwin opened this issue Feb 28, 2022 · 15 comments · Fixed by #36
Closed

Step Functions interpreter #7

sam-goodwin opened this issue Feb 28, 2022 · 15 comments · Fixed by #36

Comments

@sam-goodwin
Copy link
Collaborator

Similar to AppsyncFunction, we should build interpreters for AWS Step Functions:

  1. new ExpressStepFunction
  2. new StandardStepFunction

The ExpressStepFunction represents a synchronous workflow that returns a value and has a limited subset of features available (see: https://docs.aws.amazon.com/step-functions/latest/dg/welcome.html#welcome-workflows)

const getPerson = new ExpressStepFunction((arg: string): Person | undefined => {
  const person = personTable.get(arg);
  return {
    id: person.id.S,
    name: person.name.S
   };
})

The StandardStepFunction represents an asynchronous workflow that does not return a value - to retrieve the value, a caller must call getExecutionStatus.

Appsync Integration

It should be possible to call a Step Function from within an Appsync Resolver:

new AppsyncFunction(() => {
  // ExpressStepFunction returns synchronously
  const person = getPerson("arg");
})
@sam-goodwin
Copy link
Collaborator Author

The Step Function integrations have a different interface than Appsync. Step Function integrations more closely map to the raw AWS APIs, where-as Appsync has specialized APIs.

A consequence of this is that we can't share the same wrapper integration classes across Appsync and Step Functions.

What naming/namespace convention makes sense here?

Table:

new Appsync.TableIntegration(table)
new Appsync.Table(table)
new AppsyncTableIntegration(table)
new AppsyncTable(table)
new AppsyncDynamoDBTable(table)
new Table(table, "Appsync")

Function:

new Function(f)
new Appsync.Function(f)
new AppsyncFunction(f)
new Appsync.FunctionIntegration(f)
new AppsyncFunctionIntegration(f)

If we went with new AppsyncFunction(f), then it would conflict with the current AppsyncFunction being used to create Resolvers. Maybe AppsyncFunction should be renamed to AppsyncResolver?

@sam-goodwin
Copy link
Collaborator Author

Would it be a bad idea to provision a Lambda Function to supplement Step Functions with some of JSONata's functionality? It's not "functionless", but Step Function's intrinsic capabilities are unfortunately lacking.

Without such functionality, we will have to heavily constrain the syntax allowed within a StepFunction construct.

@sam-goodwin
Copy link
Collaborator Author

@thantos
Copy link
Collaborator

thantos commented Mar 12, 2022

Continuing discussion on how to support try catch.

One option is to use parallel to represent the try scopes.

try {
   X()
   Y()
   Z()
} catch (err) {
   A()
}
B()
flowchart LR;
   subgraph parallel-if
       X["X()"]-->Y["Y()"];
       Y-->Z["Z()"];
   end
   parallel-if --catch --> A["A()"];
   A --> B["B()"];
   parallel-if --> B;

Caveat is that the output of parallel will be an array instead of the single output of Z().

flowchart LR;
  Pass["Pass(output[0])"]
   subgraph parallel-if
       X["X()"]-->Y["Y()"];
       Y-->Z["Z()"];
   end
   parallel-if --catch --> A["A()"];
   A --> B["B()"];
   parallel-if --> Pass;
   Pass --> B

@sam-goodwin
Copy link
Collaborator Author

Why would we implement that as a parallel? I would have thought it would be a chain of Tasks, each with their Catch property pointing to the same Catch state.

flowchart LR;
       X["X()"]-->Y["Y()"];
       Y-->Z["Z()"];
       Z-->B
       X-->Catch
       Y-->Catch
       Z-->Catch
       Catch-->A
      A["A()"]-->B
   B["B()"]-->End

@thantos
Copy link
Collaborator

thantos commented Mar 13, 2022

That's another option. I was worried about what it would take to map every node in every sub-expression to a possible catch.

With parallel, any failure is caught and the sub-expressions do not need to be aware of what might catch them (like an expression in a try)

@thantos
Copy link
Collaborator

thantos commented Mar 13, 2022

try {
    try {
          A()
          B()
          C()
    } catch(err) {
          D()
    }
    if(E()) {
         F()
    } else {
         G()
    }
 } catch(err) {
    H()  
 }
I()
flowchart LR;
    A --> B
    B --> C
    C --> cond-E
    A -- err --> catch-2
    B -- err --> catch-2
    C -- err --> catch-2
    catch-2 --> D
    D --> cond-E
    cond-E -- Error --> catch-1
    cond-E -- TRUE --> F
    cond-E -- FALSE --> G
    F -- err --> catch-1
    G -- err --> catch-1
    D -- err --> catch-1
    catch-1 --> H
    G --> I
    H --> I
    F --> I

vs

flowchart LR;
    subgraph p1
    subgraph p2
    A --> B
    B --> C
    end
    C --> cond-E
    p2 -- err --> catch-2
    catch-2 --> D
    D --> cond-E
    cond-E -- TRUE --> F
    cond-E -- FALSE --> G
    end
    p1 --> catch-1
    catch-1 --> H
    G --> I
    H --> I
    F --> I

@sam-goodwin
Copy link
Collaborator Author

With parallel, any failure is caught and the sub-expressions do not need to be aware of what might catch them (like an expression in a try)

It's most important that the behavior reflects what the developer writes. Running things in parallel when they expect them in series would be bad.

@thantos
Copy link
Collaborator

thantos commented Mar 13, 2022

Ohh, here parallel is used as a container which can have a single catch, You have a single branch of a parallel which is a single graph run in sequence. Parallel branches are run in parallel, but a single branch is run in sequence.

Not suggesting we would change the behavior at all.

@thantos thantos mentioned this issue Mar 14, 2022
19 tasks
@sam-goodwin
Copy link
Collaborator Author

In step functions, what should the behavior of for-loops and list.map be?

  1. map the NodeJS behavior exactly to Step Functions - i.e. run with a MaxConcurrency of 1 - and then provide a special $SFN.map utility that allows for configuring MaxConcurrency?
  2. do everything in parallel by default (divergence from NodeJS behavior) and have developers use $SFN.map to configure MaxConcurrency to 1.

I think we should maintain the NodeJS behavior (option 1). We want to minimize surprises.

@sam-goodwin
Copy link
Collaborator Author

sam-goodwin commented Mar 19, 2022

@thantos @brendo-m thoughts?

@thantos
Copy link
Collaborator

thantos commented Mar 19, 2022

I agree, let's opt for the typescript definition of functions over the service's implementation. We want map to mean the same thing used in step functions, app sync, or lambda.

@sam-goodwin
Copy link
Collaborator Author

Need to account for this unwrapped numbers https://twitter.com/allenheltondev/status/1504851648111382529?s=21

@sam-goodwin
Copy link
Collaborator Author

Proposal for for try-catch:

  1. if the throw state is within a try-catch, use a Pass state to go to the CatchClause.
  2. if the throw state is not within a try-catch, use a Fail state and terminate the State Machine

@thantos
Copy link
Collaborator

thantos commented Oct 11, 2022 via email

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 a pull request may close this issue.

2 participants