[NOMERGE] Rust SDK future proposal draft #550

Sushisource · 2023-05-12T18:09:01Z

Just serves as a place we can discuss Rust SDK vision before turning it into an actual proposal.

For the community: This (unfortunately, sorry) is not meant to imply we'll be implementing this any time soon - rather it will serve to illustrate where we intend to go once we have decided to take on productionization of the Rust SDK.

cretz

Initial thoughts, I may be way off on some but they are easily dismissable. They at least provide alternatives for the proposal to justify not using any them.

cretz · 2023-05-12T19:10:06Z

workflow-api/src/lib.rs

+    impl Workflow for MyWorkflow {
+        type Input = String;
+        type Output = u64;
+        const NAME: &'static str = "MyWorkflowType";


I wonder if this would be better as a static getter, e.g. fn get_name() -> &str (or maybe String since we should only ask once anyways). If you do leave as const, might as well use the #[workflow] attribute.

Yeah, it probably should be, if you need to dynamically construct workflow definitions for whatever reason

cretz · 2023-05-12T19:12:19Z

workflow-api/src/lib.rs

+        type Output = u64;
+        const NAME: &'static str = "MyWorkflowType";
+
+        fn new(input: Self::Input, _ctx: SafeWfContext) -> Self {


Suggested change

fn new(input: Self::Input, _ctx: SafeWfContext) -> Self {

fn new(input: Self::Input, _ctx: PureWorkflowContext) -> Self {

Two suggestions:

No more abbreviations (boo "Wf")

Something besides safe because I fear it's too generic of an adjective. I don't think "Pure" is that good either tbh, I'd have to think on it. But it's basically "CommandsDisallowedWorkflowContext".

cretz · 2023-05-12T19:15:28Z

workflow-api/src/lib.rs

+            // TODO: The need to box here is slightly unfortunate, but it's either that or require
+            //  users to depend on `async_trait` (which just hides the same thing). IMO this is the
+            //  best option until more language features stabilize and this can go away.


Can you just require run be an async fn?

Nah, that's the thing that can't work in a trait yet.

May make it worth doing it attribute style. PyO3 are heavy users of this approach over trait-based (e.g. https://pyo3.rs/v0.18.3/class and https://pyo3.rs/v0.18.3/function).

That's just async trait at that point, but if it ends up being useful for other reasons I'm definitely OK with a procmacro

Maybe something like:

#[workflow] struct MyWorkflow {} impl MyWorkflow { #[workflow_run] async fn run(&mut self, some_arg: String) -> TemporalResult<String> { // ... } #[workflow_signal] async fn my_signal(&mut self, some_arg) { // ... } }

Where it all gets interesting is what the caller side looks like in a type safe way which is why I know that traits requiring defined in/out types are helpful.

(and a #[workflow_init] or #[workflow_new] or whatever named attribute on a func that accepts workflow args to create the struct (required if struct cannot be "default" created)...we built this attribute for .NET constructors to allow for readonly field initialization)

The #[workflow] attribute would just go over the impl block I think. There's no need for it on the struct definition, and that allows you to just require a new and a run method without them needing separate attributes.

Nice. May be worth exploring at proposal PR time if there aren't too many drawbacks. If caught between two impls that both may kinda work, just list pros/cons of each and then start one of our classic design Zoom calls to pick a winner :-)

Mega necropost

Preparing for pres at offsite... I'm now pretty convinced #[workflow] attribute macro is the best way to go for sure. Fixes async problem, makes signals/queries automatically work, enforces new & run... It's the way to go. People who want to could still do it all by hand if they hate it for whatever reason

And it's so pretty!

cretz · 2023-05-12T19:18:17Z

workflow-api/src/lib.rs

+
+        fn run(
+            &mut self,
+            ctx: WfContext,


When you do get around to proposal time, I think it'd be worth a discussion of the tradeoff of making this statically accessible via async-local or similar. What I've found in the last two SDKs is that I can just grab the current coroutine scheduler.

Calls like https://docs.rs/tokio/latest/tokio/task/fn.spawn.html#panics are ok assuming the runtime or panicking, I wonder if we can be too. But also I could understand from a function-grouping perspective that having it on a context may be clearer.

One benefit of not having a manually propagated context is that users aren't tempted to store it to call from a query handler or something.

Hrmm, now I wonder if we can have the trait have fn context(&mut self) -> WorkflowContext and fn safe_context(&self) -> SafeWorkflowContext on it. But that doesn't help the new call though.

We could. I would be OK with it I think. This feels somewhat more obvious to me, and I like that you don't have these error paths for "called in some context where it doesn't make sense".

cretz · 2023-05-12T19:20:11Z

workflow-api/src/lib.rs

+    fn run(
+        &mut self,
+        ctx: WfContext,
+    ) -> BoxFuture<Result<WfExitValue<Self::Output>, anyhow::Error>>;


Is there any way we can avoid forcing anyhow on users here? Can we just have TemporalFailure enum here and some into() or whatever-similar-to-anyhow helper that's clear it converts to ApplicationFailure?

It can certainly be our own thing yes, this is actually addressed briefly in a comment near the top

cretz · 2023-05-12T19:21:01Z

workflow-api/src/lib.rs

+    impl MyWorkflow {
+        // Attrib commented out since it's nonexistent for now, but that's what it'd look like.
+        // #[signal]
+        pub fn my_signal(&mut self, arg: String) {


Can I have the context here?

cretz · 2023-05-12T19:21:11Z

workflow-api/src/lib.rs

+            self.bar.insert(arg, 1);
+        }
+        // #[query]
+        pub fn my_query(&self, arg: String) -> Option<u64> {


Can I have the "safe" context here?

cretz · 2023-05-12T19:27:24Z

workflow-api/src/lib.rs

+                ctx.timer(Duration::from_secs(1)).await;
+                self.foo = 1;
+                // See activity definitions file
+                ctx.my_act_fn("Hi!").await.unwrap();


Concern here is how to pass in options. One reason that Java's approach of reusing actual activity signature was so rough is that you were forced to make options a separate thing.

cretz · 2023-05-12T19:29:34Z

workflow-api/src/lib.rs

+                // The into() is unfortunately unavoidable without making C-A-N and confirm cancel
+                // be errors instead. Personally, I don't love that and I think it's not idiomatic
+                // Rust, whereas needing to `into()` something is. Other way would be macros, but
+                // it's slightly too much magic I think.


Concerning the into(), I wonder if maybe instead of Result<WfExitValue<Self::Output>, anyhow::Error> you have Result<Self::Output, TemporalNonSuccess> of some form. That way Ok only represents literal success, not cancellation or continue as new.

I think it's worth optimizing for the normal/happy path of Ok being the literal result and all else being the Err.

The problem is CAN is a form of success. The semantics of it are my real issue. TemporalNonSuccess as a name is a perfect example. CAN isn't "NonSuccess".

Now, to be fair, currently I have WillRespondAsync as an error in activities. My rationale is that's less common than CAN. I get optimizing for the happy path, but into is easy and the compiler error will make it super obvious if the user hasn't done it.

I could live with it, for sure, but it just doesn't feel correct to me or particularly idiomatic for Rust

Since continue as new has to be invoked top level to use this type anyways, I wonder if you can make it a macro that returns (so they don't have to ? or Err it), e.g. continue_as_new!(). That can kinda hide it. Otherwise, I agree that continue as new and cancel aren't technically errors, it's just a bit rough on the simple path. I can see both arguments here.

cretz · 2023-05-12T19:31:16Z

workflow-api/src/lib.rs

+        bar: HashMap<String, u64>,
+    }
+
+    impl Workflow for MyWorkflow {


Suggested change

impl Workflow for MyWorkflow {

#[workflow]

impl Workflow {

I wonder if there's value in not requiring trait impl and instead marking the run method with #[workflow_run] to extract input/output types. Only reason I was thinking this is because I kinda like that I can put my signal/query/update functions in the same impl block as my run stuff. But otherwise, I see how it hurts new() (unless you put the attribute there too).

People are pretty used to multiple impl blocks in Rust. That said, I can get behind a procmacro like this too if it's providing value like auto-naming. It just needs to not do too much magic, because messing up IDEs etc is where things get really bad.

Sushisource added 3 commits May 12, 2023 11:10

Initial trait-based workflow API skeleton

ca75b1d

WfExitValue

f5533a8

Activity definition

a36ab43

Sushisource force-pushed the wf-api branch from 79e3e31 to a36ab43 Compare May 12, 2023 18:10

Sushisource mentioned this pull request May 12, 2023

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

Closed

cretz reviewed May 12, 2023

View reviewed changes

Sushisource mentioned this pull request Aug 22, 2023

Rust SDK Prototype Release #222

Open

dandavison force-pushed the wf-api branch 2 times, most recently from 20b7bac to a36ab43 Compare December 5, 2023 18:21

Sushisource mentioned this pull request Apr 24, 2024

sdk: allow types to abstract over encoding #718

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NOMERGE] Rust SDK future proposal draft #550

[NOMERGE] Rust SDK future proposal draft #550

Sushisource commented May 12, 2023

cretz left a comment

cretz May 12, 2023

Sushisource May 12, 2023

cretz May 12, 2023

cretz May 12, 2023

Sushisource May 12, 2023

cretz May 12, 2023

Sushisource May 12, 2023

cretz May 12, 2023 •

edited

Loading

cretz May 12, 2023 •

edited

Loading

Sushisource May 12, 2023

cretz May 12, 2023 •

edited

Loading

Sushisource Aug 11, 2023

cretz Aug 14, 2023

cretz May 12, 2023 •

edited

Loading

Sushisource May 12, 2023

cretz May 12, 2023

Sushisource May 12, 2023 •

edited

Loading

cretz May 12, 2023

Sushisource May 12, 2023

cretz May 12, 2023

Sushisource May 12, 2023

cretz May 12, 2023

cretz May 12, 2023

Sushisource May 12, 2023

cretz May 12, 2023 •

edited

Loading

cretz May 12, 2023

Sushisource May 12, 2023

	fn new(input: Self::Input, _ctx: SafeWfContext) -> Self {
	fn new(input: Self::Input, _ctx: PureWorkflowContext) -> Self {

[NOMERGE] Rust SDK future proposal draft #550

Are you sure you want to change the base?

[NOMERGE] Rust SDK future proposal draft #550

Conversation

Sushisource commented May 12, 2023

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 12, 2023 • edited Loading

Choose a reason for hiding this comment

cretz May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz May 12, 2023 •

edited

Loading

cretz May 12, 2023 •

edited

Loading

cretz May 12, 2023 •

edited

Loading

cretz May 12, 2023 •

edited

Loading

Sushisource May 12, 2023 •

edited

Loading

cretz May 12, 2023 •

edited

Loading