Updated workflow function to return Payload instead of empty tuple

[h7kanna]

What was changed

1. Modified SDK to handle 'Payload' as the return value of workflow function instead of an empty tuple.
2. Updated tests

Why?

A step towards #222

Checklist

1. This part of work in [Feature Request] Workflow definitions #539

2. How was this tested:
   Updated tests and added new tests

3. Any docs updates needed?
   No

[Sushisource]

Thanks! Per my comment in #540 I think I'd prefer it if we went straight to the more general abstraction here 😄

[h7kanna]

Sure, I will work towards updating this.

[h7kanna]

Hi @Sushisource,

I am actually scratching my head to understand why you are asking for the Payload abstraction. 😁

I designed the workflow module to make sure that we can return any type as long as it implements Serialize. So in my workflow code, I am just returning structs wrapped in WfExitValue without manual payload conversions.

Indeed I just postponed implementing the Payload converter as well in the workflow module.

The thing is in all the tests, Workflow functions are created manually using WorkflowFunction.new, wherein I am just using the worker.register function with a wrapper to convert any supported function to BoxedFuture that the WorkflowFunction needs. 😬

I just need to update the WorkflowFunction.new to match the wrapper 😌
Also, as a bonus, I will go ahead and modify the wrapper to use the generic payload converter trait instead of the AsJsonPayloadExt and FromJsonPayloadExt I am using now.

[Sushisource]

I'm a bit confused - very few of the tests (3 by my count) directly use WorkflowFunction::new, most are just using register_wf directly.

What you have looks good to me now in terms of not touching many files - the main reason to introduce an abstraction is the JsonPayloadExt traits are not meant to be the end-goal for what workflow/activity functions can return. They should be able to return anything Serializeable, and eventually that can get hooked up to a payload converters concept.

So for now we could do that, just make the bounds here be Serialize instead.

Eventually WorkflowFuture itself needs to be generic over the output type, rather than always being WorkflowResult<Payload>, but I'm fine with that side of things just giving you Payloads for now.

[Sushisource]

This test needs to stay as a panic, it's important to the goal of the test. You'll have to just add a type hint for the closure's return type.

[h7kanna]

Type inference won't work and the code becomes unreachable after panic!(). So panicking with unwrap

[Sushisource]

Ah right, I missed the unwrap - regardless this can keep the more obvious panic by providing an explicit return type for the closure.

[h7kanna]

Fixed

[h7kanna]

I tried a lot to replace 'Payload' in the Workflow Future with a trait before starting the discussion 😄.
It ends with the requirement of WorkflowFunction becoming generic and then bounds won't add up correctly and the From implementation fails with 'unconstrained' bounds etc. I will try again though.

Regarding the Payload being generic. I changed the trait a bit for now here, next PR. I think we need to pass the Payload converter into both WfContext and ActContext and use it in a wrapper function like this.

I will work on how we can make this dynamic without help from the generics. Currently looks like this

    let activity = ActivityOptions {
        activity_id: Some(activity_id.to_string()),
        ..Default::default()
    };
    let result = ctx.activity(activity).await.unwrap_ok_payload();
    let result = String::from_payload(None, &result).expect("serializes fine");
    Ok(WfExitValue::Normal(result))
}

[h7kanna]

Though, the end result with wrapper functions the workflow looks like this, without any manual payload processing

pub async fn workflow_shell(ctx: WfContext, _input: (String,)) -> Result<ExecOutput, Error> {
    let activity_timeout = Duration::from_secs(5);
    let output = execute_activity_1_args_with_errors(
        &ctx,
        ActivityOptions {
            activity_id: Some("shell_activity".to_string()),
            activity_type: "activity_shell::execute".to_string(),
            ..Default::default()
        },
        activity_shell::exec_command2,
        activity_shell::ExecInput {
            command: "sort".to_string(),
            ..Default::default()
        },
    )
    .await;
    match output {
        Ok(output) => Ok(output),
        Err(e) => Err(e),
    }
}

[Sushisource]

Cool, for the purposes of this PR, I think just changing the bounds here be Serialize instead would be sufficient, and keeps things pretty much the same but without explicitly coupling things to JSON.

As for the further generic-ification, I like what you have in your final example there, but I also want to avoid any explicit arity-based wrapper functions. More likely than not, I don't think we'll be merging anything that makes changes that substantial until we're prepared to really start investing in productionizing and supporting the Rust SDK.

That said, your interest has been great, and in the spirit of us being transparent I'm going to make some time to at least publish a proposal outlining the direction I want to go with the Rust SDK, even though we still may not actually invest in the implementation immediately.

[h7kanna]

Hi,

The multiple arity functions are just wrappers in my code. They will not be in the SDK. But I would need the possibility of passing Vec<Payload> in the ActivityOptions. Are you okay with that?

For example, the SDK will only have 0 or 1 argument function wrappers as you mentioned in the previous comment here. But my code is like Itertools for Iterators :)

[h7kanna]

Thanks for deciding to spend time on the proposal 😄 . I can use that for direction instead.

[Sushisource]

Sweet, thanks!