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

WIP: Workflow defintions #540

Closed
wants to merge 2 commits into from

Conversation

h7kanna
Copy link
Contributor

@h7kanna h7kanna commented May 1, 2023

What was changed

  1. Modified SDK to handle 'Payload' as the return value of workflow function instead of an empty tuple.
  2. Minor changes to SDK workflow and activity functions to accommodate generic user-defined functions.
  3. Added 'workflow' module to create abstractions to define workflow definitions in idiomatic Rust(Async)
  4. Added 'prelude' module for easy importing of types in workflow definitions.
  5. Updated tests

I use Rust a lot and I write workflows a lot and a lot. So created the abstraction functions needed to define workflows similar to other SDKs (Go). Submitting this PR to receive feedback on design and approach.

TODO

  • Factor in the feedback
  • Move the repeating and boilerplate into Macros once the API is good enough
  • Create more convenient functions similar to other SDKs.
  • Add more examples

Why?

A step towards #222

Checklist

  1. Closes [Feature Request] Workflow definitions #539

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

  3. Any docs updates needed?
    Updated crate documentation and added new module documentation

@h7kanna h7kanna requested a review from a team as a code owner May 1, 2023 03:51
@h7kanna h7kanna changed the title WIP: Workflow constructs WIP: Workflow defintions May 1, 2023
@h7kanna h7kanna force-pushed the workflow-constructs branch 2 times, most recently from a1012ca to c12c14a Compare May 1, 2023 14:34
@Sushisource
Copy link
Member

Thanks! I really appreciate your work here. Adding support for returning Payloads from workflows is something I think we could take first and separately, which would be great. More on this after.

As for the workflow & activity function abstractions, I think the direction we've had in mind is a bit different. You mentioned converting things to macros later, I think that's something we'll want to leverage, and I've also mulled over the idea of using a trait-based implementation for workflows. Big picture wise, we want to make sure we've nailed down an API that aligns with what we want a future Rust SDK to look like. Importantly, we want to make sure that once we do start moving forward on a more complete Rust SDK that it aligns there, and it's something we're going to be comfortable supporting in the long run. In fact, the main reason we haven't moved forward here is mainly a question of support rather than implementation.

So all that in mind, at the moment I think the kind of PRs I'm happy to merge at the moment are ones that target specific bits of missing functionality.

When we do feel that we have the bandwidth to move forward with a non-prototype API we'll start with a proposal here: https://github.com/temporalio/proposals


Re: Separating out returning payloads from workflow functions. It'd be great to add this. In particular, what I had in mind was being able to return anything that impl TemporalSerializable, or something similar, where:

pub trait TemporalSerializable {
    fn into_payload(self) -> anyhow::Result<Payload>
}

impl<T> TemporalSerializable for T where T: serde::Serialize {
    // blah
}

This way, the return site in the workflow need not do any conversion directly, and this can fit into a data converter type facility later on like the other SDKs have.

Thanks again! Let me know if you have any questions

@h7kanna
Copy link
Contributor Author

h7kanna commented May 1, 2023

Hi @Sushisource, Thanks.

The reason I opened the PR is to get the first commit(Payloads) accepted and merged so that I can iterate on the Workflow abstractions without constant rebasing.

I will create a separate PR for it and make use of the TemporalSerializable you suggested.

Regarding the Workflow definitions, I will keep on improving them as I need some boilerplate reduction for my project which is a lot of Rust workflows.

@h7kanna
Copy link
Contributor Author

h7kanna commented May 1, 2023

Hi @Sushisource,

On second thought, It might take a while to design for return 'impl TemporalSerializable' instead of Payload from the workflow function. At the same time, I want to keep working on abstractions for my project workflows.

I will break this into 3 parts.

  1. Return Payload from workflow functions (PR-1)
  2. Modify the Activity function similar to Workflow Function and accept Vec<Payload> here. Need this for the abstractions. Will help with rebasing as a lot of unit tests are updated (PR-2)
  3. Figure out returning the TemporalSerializable(PR-3)
  4. I will continue working on workflow definitions in my fork and close this one.

Is it ok with you?

@Sushisource
Copy link
Member

I'd like to have the PR introducing Payload go straight to the more complete abstraction, largely because it allows avoiding touching almost every single test file to add a fake returned payload. With the complete abstraction, they might still need to be touched, but it'll be to remove an into() call rather than adding more which would be great.

Supporting sending multiple payloads when scheduling activities makes sense, and is eventually needed regardless, so yeah I'm happy to have that addition too. Related, I'm still not sure I ever want to support anything other than 0 and 1 arity workflow and activity definitions in Rust. It's generally an anti-pattern to take more than one argument in your definitions because it makes maintaining input & output compatibility difficult. We need to support scheduling activities and child WFs with 2+ arities, because you might be targeting things defined in other languages, but my inclination is to avoid defining them entirely in a Rust SDK.

@h7kanna
Copy link
Contributor Author

h7kanna commented May 1, 2023

Closing this and continue in parts in #542

@h7kanna h7kanna closed this May 1, 2023
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 this pull request may close these issues.

[Feature Request] Workflow definitions
2 participants