Updated Activity/Workflow Function bounds to allow a more generic use…

[h7kanna]

What was changed

1. Updated workflow and activity functions to take 0 or 1 arguments and accommodate generic functions as extensions (loosen up bounds)

2. Unfortunately, it touches activity tests, but they are only updating the function signature, all tests pass including load tests.

Why?

To enable creating a type-safe API using more generic user functions.
For example, a user function that does not return anyhow::Error

With this, we can register

1. 0 arg function similar to how it is done currently

worker.register_activity("echo", |_: ActContext| async {
    Result::<ActExitValue<()>, _>::Err(anyhow!("Oh no I failed!"))
});

2. 1 arg function as shown below, similar to how workflow function is registered currently.

worker.register_activity(
    "echo_activity",
    ActivityFunction::from(|_ctx: ActContext, echo_me: String| async move { Ok(echo_me) }),
);

Checklist

1. Closes

2. How was this tested:

1. Updated tests
2. Ran unit tests
3. Ran integration tests
4. Ran heavy tests(load)
   All tests passed.

3. Any docs updates needed?
   Updated function documentation

[h7kanna]

Hi @Sushisource, This is the last PR(relating to SDK changes) I will submit until further update from you on the proposal.
This will enable me to work independently of the SDK in my fork without any large rebasing in the future.
Changes are in line with what you have suggested in the previous comments till now, with only 0/1 arguments API in the SDK.

Thanks for taking the time, I know this is not planned work for you.

Ignore the function signature but my code currently looks a lot boilerplate free like I have written in this blog

[Sushisource]

Sorry it took me a bit to get around to this

[Sushisource]

So ideally both this and the activity version are implemented generically (as actual From impls) for anything that matches these bounds -- allowing us to avoid XXXFunction::from calls at definition sites, since the registration functions already accept impl Into<XXXFunction>. I think this is a really nice piece of ergonomics to maintain.

[h7kanna]

Hi, sorry, did not understand this comment completely.

[Sushisource]

The idea would be to do something like this:

impl< ... generics ... > From<F> where: bounds for ActivityFunction {
   ...
}

For both activity and wf functions

So there's no need to call ActivityFunction::from at the same place you're defining or registering the function - everything just slots in.

[h7kanna]

Haa,

I tried to do that, but the From implementation cannot be implemented for functions with more generic arguments, it fails with 'unconstrained bounds'

So the only From implementation is the one existing. Activity/Workflow Function that takes only Context.

For functions that take an argument, We need the ActivityFunction::from/WorkflowFunction::new wrapper like in the core tests.

But in the production SDK, we will actually not use these functions(I wish). There should be higher-level 'execute_activity' or 'execute_childworkflow' functions that exist in other SDKs which hide the 'Payload' processing(Converter etc), but those that check the types of the arguments like this

Essentially, the PR is to allow any function that can be converted into the below functions that the scheduling(currently) works with.

dyn Fn(WfContext) -> BoxFuture<'static, Result<WfExitValue<Payload>, anyhow::Error>>
    + Send
    + Sync
    + 'static

or

    dyn Fn(ActContext) -> BoxFuture<'static, Result<ActExitValue<Payload>, anyhow::Error>>
        + Send
        + Sync,

[Sushisource]

So this https://github.com/temporalio/sdk-core/blob/master/sdk/src/lib.rs#L840 implements an arg-accepting automatic implementation for activities - and, yeah, clearly we can't make that work for both 0 and 1 input-arity functions without some extra boilerplate or macros, etc. That said, there's nothing preventing implementing IntoActivityFunc for 0 and 1 input-arity functions and having this all work properly. Same could be done with workflows.

I think that'd be the best place to leave things for now, no need to worry about higher-arity functions at the moment.

You can see a preview of what I'm thinking of for later here: #550 which will lean on procmacros for making this all seamless.

[h7kanna]

I will look up the proposal and adapt this accordingly.

[h7kanna]

Closed this as per Comments