Additional API surface area for Dapr Workflow authoring SDK by cgillum · Pull Request #1012 · dapr/dotnet-sdk

cgillum · 2023-01-18T00:21:37Z

Description

This PR adds a significant amount of API surface area to the workflow authoring SDK. It's a follow-up to #981, which had some feedback comments still unresolved. The other motivation for this work was that building the demo highlighted some significant gaps in the development experience.

Summary of changes:

Workflows and activities can now be defined as classes, deriving from Workflow or WorkflowActivity base classes respectively.
Updated example app to look more like a real order processing web app rather than "hello, world"
Added several more APIs to the WorkflowContext object, allowing workflows to be far more expressive (matching what the Durable Task SDK supports).
Renamed WorkflowClient to WorkflowEngineClient to make it more distinct from the Dapr client and added several more APIs to hide the internal dependencies.
Renamed WorkflowMetadata to WorkflowState (warning, Git made some incorrect assumptions about which files were renamed)
More detailed documentation

NOTE: The careful reader will notice that a lot of the code in this SDK is a very thin wrapper around the Durable Task SDK. I do wonder if trying to hide the Durable Task SDK from the Dapr SDK will at some point become unsustainable and confusing for developers. I worry that we won't be able to fully hide the fact that the Dapr Workflow engine is implemented using the Durable Task Framework. An alternative approach to what I'm doing now would be to tell Dapr developers to simply use the Durable Task SDK directly, and we could ship some Dapr-specific extension methods that work with it. This would be far less confusing and less burdensome to Dapr maintainers since the amount of code they need to maintain is dramatically reduced. However, I'm not sure how the Dapr maintainers/TOC would feel about this from a branding/control perspective. I'm open to opinions, though we don't necessarily need to block this PR on that discussion.

Issue reference

This PR is related to the following GitHub issues:

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

codecov · 2023-01-18T00:25:53Z

Codecov Report

Merging #1012 (0d2324c) into master (5a7e59f) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
+ Coverage   69.90%   69.92%   +0.01%     
==========================================
  Files         157      157              
  Lines        5227     5227              
  Branches      562      562              
==========================================
+ Hits         3654     3655       +1     
+ Misses       1440     1439       -1     
  Partials      133      133

Flag	Coverage Δ
net5	`69.88% <ø> (+4.76%)`	⬆️
net6	`69.84% <ø> (+0.01%)`	⬆️
netcoreapp3.1	`69.88% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Dapr.Client/DaprClientGrpc.cs	`86.88% <0.00%> (+0.14%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

tmacam · 2023-01-19T19:44:04Z

+    /// </para>
+    /// <para>
+    ///   Workflows may be replayed multiple times to rebuild their local state after being reloaded into memory.
+    ///   workflow code must therefore be <em>deterministic</em> to ensure no unexpected side effects from execution


I think I remember a comment about workflows must be thought as idempotent -- while the activities themselves don't need to be. Not sure if this idempotency is a concept we want to highlight here

Actually, workflows don't need to be idempotent because they don't have any external side effects, which essentially guarantees effectively-once execution guarantees. The only restriction therefore is that workflows are deterministic.

Activities don't need to be deterministic, but it is a good practice to make them idempotent since they have at-least-once execution guarantees. I believe we discuss this in the comments for the WorkflowActivity class.

tmacam · 2023-01-19T19:46:07Z

+    ///   Workflow code is tightly coupled with its execution history so special care must be taken when making changes
+    ///   to workflow code. For example, adding or removing activity tasks to a workflow's code may cause a
+    ///   mismatch between code and history for in-flight workflows. To avoid potential issues related to workflow
+    ///   versioning, consider applying the following code update strategies:


tmacam · 2023-01-19T21:47:15Z

+                if (this.workflowState == null)
+                {
+                    return WorkflowRuntimeStatus.Unknown;
+                }
+
+                switch (this.workflowState.RuntimeStatus)
+                {
+                    case OrchestrationRuntimeStatus.Running:
+                        return WorkflowRuntimeStatus.Running;
+                    case OrchestrationRuntimeStatus.Completed:
+                        return WorkflowRuntimeStatus.Completed;
+                    case OrchestrationRuntimeStatus.Failed:
+                        return WorkflowRuntimeStatus.Failed;
+                    case OrchestrationRuntimeStatus.Terminated:
+                        return WorkflowRuntimeStatus.Terminated;
+                    case OrchestrationRuntimeStatus.Pending:
+                        return WorkflowRuntimeStatus.Pending;
+                    default:
+                        return WorkflowRuntimeStatus.Unknown;
+                }


So the only thing this is doing is:

null => Unknown

Suspended => Unknown

everything else is returned as is. Is that it?

I am trying to understand if this couldn't be replaced with return this.workflowState?.RuntimeStatus ?? WorkflowRuntimeStatus.Unknown and the Suspended state is the only thing that would break this, so I am trying to understand Suspended was omitted from that switch on purpose or by accident.

Ok, just noticed that this is doing a type mapping from OrchestrationRuntimeStatus to WorkflowRuntimeStatus. Anyway, re-checking if Suspended => should be equivalent to Unknown.

Yes, so I unfortunately the 1.0.0-rc.1 version of the Durable Task SDK doesn't have the Suspended enum yet, so I can't make that change until the 1.0.0 version gets published. So for now, WorkflowRuntimeStatus.Suspended is unused until probably the next release.

cgillum

Thanks for the review, @tmacam! See my responses. I'll also push an update based on your feedback.

cgillum · 2023-01-19T22:45:11Z

+    /// </para>
+    /// <para>
+    ///   Workflows may be replayed multiple times to rebuild their local state after being reloaded into memory.
+    ///   workflow code must therefore be <em>deterministic</em> to ensure no unexpected side effects from execution


Actually, workflows don't need to be idempotent because they don't have any external side effects, which essentially guarantees effectively-once execution guarantees. The only restriction therefore is that workflows are deterministic.

Activities don't need to be deterministic, but it is a good practice to make them idempotent since they have at-least-once execution guarantees. I believe we discuss this in the comments for the WorkflowActivity class.

cgillum · 2023-01-19T22:49:53Z

+                if (this.workflowState == null)
+                {
+                    return WorkflowRuntimeStatus.Unknown;
+                }
+
+                switch (this.workflowState.RuntimeStatus)
+                {
+                    case OrchestrationRuntimeStatus.Running:
+                        return WorkflowRuntimeStatus.Running;
+                    case OrchestrationRuntimeStatus.Completed:
+                        return WorkflowRuntimeStatus.Completed;
+                    case OrchestrationRuntimeStatus.Failed:
+                        return WorkflowRuntimeStatus.Failed;
+                    case OrchestrationRuntimeStatus.Terminated:
+                        return WorkflowRuntimeStatus.Terminated;
+                    case OrchestrationRuntimeStatus.Pending:
+                        return WorkflowRuntimeStatus.Pending;
+                    default:
+                        return WorkflowRuntimeStatus.Unknown;
+                }


Yes, so I unfortunately the 1.0.0-rc.1 version of the Durable Task SDK doesn't have the Suspended enum yet, so I can't make that change until the 1.0.0 version gets published. So for now, WorkflowRuntimeStatus.Suspended is unused until probably the next release.

halspang

Overall this looks good pretty good to me. I just have a few questions but I don't see anything glaring in here.

halspang · 2023-01-19T23:09:05Z

+        /// <param name="context">The workflow's context.</param>
+        /// <param name="input">The workflow's input.</param>
+        /// <returns>Returns the workflow output as the result of a <see cref="Task"/>.</returns>
+        Task<object?> RunAsync(WorkflowContext context, object? input);


For my own edification, the reason we're doing it this way is so that when the customer extends the abstract class, they can still specify the type variables, right?

The main reason we're doing this is so that we can have common code that can run the user's code without needing to worry about the specific type parameters. I don't recall exactly, but I think there wasn't a way for me to write this common code when generics were involved. It was a problem I originally encountered in the design of the Durable Task SDK that we depend on.

halspang · 2023-01-19T23:09:39Z

+    /// Users should not implement workflows using this interface, directly.
+    /// Instead, <see cref="Workflow{TInput, TOutput}"/> should be used to implement workflows.
+    /// </remarks>
+    public interface IWorkflow


If we don't want users to use this, why not make it internal or just provide the abstract class?

I can't make it internal because there are public APIs that depend on it (like the RegisterWorkflow<T>() API). I believe there was some reason I needed this because of constraints related to how generics work in .NET, though I can't remember exactly what it was...

halspang · 2023-01-19T23:09:53Z

+    /// Users should not implement workflow activities using this interface, directly.
+    /// Instead, <see cref="WorkflowActivity{TInput, TOutput}"/> should be used to implement workflow activities.
+    /// </remarks>
+    public interface IWorkflowActivity


Same questions from Workflow here.

halspang · 2023-01-19T23:30:16Z

+        /// </summary>
+        /// <remarks>
+        /// <para>
+        /// This method is primarily designed for eternal workflows, which are workflows that


Are eternal workflows common? The idea of that scares me 😅 I'm fine keeping this, just curious.

Yes, it's actually very common. 😅 It's a way for you to create some kind of long running agent that doesn't have an explicit lifetime, like a monitor that maybe runs in a loop and only takes action when some event occurs.

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

cgillum

Thanks @halspang for your comments! I've added my responses.

cgillum · 2023-01-20T00:37:47Z

+        /// </summary>
+        /// <remarks>
+        /// <para>
+        /// This method is primarily designed for eternal workflows, which are workflows that


Yes, it's actually very common. 😅 It's a way for you to create some kind of long running agent that doesn't have an explicit lifetime, like a monitor that maybe runs in a loop and only takes action when some event occurs.

cgillum · 2023-01-20T00:40:04Z

+    /// Users should not implement workflows using this interface, directly.
+    /// Instead, <see cref="Workflow{TInput, TOutput}"/> should be used to implement workflows.
+    /// </remarks>
+    public interface IWorkflow


I can't make it internal because there are public APIs that depend on it (like the RegisterWorkflow<T>() API). I believe there was some reason I needed this because of constraints related to how generics work in .NET, though I can't remember exactly what it was...

cgillum · 2023-01-20T00:42:08Z

+        /// <param name="context">The workflow's context.</param>
+        /// <param name="input">The workflow's input.</param>
+        /// <returns>Returns the workflow output as the result of a <see cref="Task"/>.</returns>
+        Task<object?> RunAsync(WorkflowContext context, object? input);


The main reason we're doing this is so that we can have common code that can run the user's code without needing to worry about the specific type parameters. I don't recall exactly, but I think there wasn't a way for me to write this common code when generics were involved. It was a problem I originally encountered in the design of the Durable Task SDK that we depend on.

tmacam

LGTM

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

halspang

One small comment and can I also ask you to update the .csproj for workflows to no longer target net5? We're removing it in another PR so if you don't want to I can get it there as well.

halspang · 2023-01-23T22:22:20Z


+            serviceCollection.AddOptions<WorkflowRuntimeOptions>().Configure(configure);
+
+            static bool TryGetGrpcAddress(out string address)


We have a shared package that may already do what you need here.

https://github.com/dapr/dotnet-sdk/blob/master/src/Shared/DaprDefaults.cs

Thanks for pointing this out, I hadn't seen it. I ran into a couple problems trying to use it as-is though. For now, I'm going to put a TODO comment in the code suggesting that we adopt the common code if/when the problems can be safely addressed.

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

…t-sdk into alpha-updates

Additional API surface area

c173ecc

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

cgillum requested review from a team as code owners January 18, 2023 00:21

tmacam reviewed Jan 19, 2023

View reviewed changes

Comment thread examples/Workflow/WorkflowWebApp/Program.cs

tmacam reviewed Jan 19, 2023

View reviewed changes

Comment thread src/Dapr.Workflow/WorkflowState.cs Outdated

cgillum commented Jan 19, 2023

View reviewed changes

halspang reviewed Jan 19, 2023

View reviewed changes

PR feedback

5df1c07

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

cgillum commented Jan 20, 2023

View reviewed changes

tmacam previously approved these changes Jan 20, 2023

View reviewed changes

Environment variable configuration

d0f62a9

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

cgillum dismissed tmacam’s stale review via d0f62a9 January 23, 2023 18:20

Merge branch 'master' into alpha-updates

448dd2e

halspang reviewed Jan 23, 2023

View reviewed changes

cgillum added 2 commits January 23, 2023 16:35

Fix .NET TFMs and complete the README.md contents

048522d

Signed-off-by: Chris Gillum <cgillum@microsoft.com>

Merge branch 'alpha-updates' of https://github.com/cgillum/dapr-dotne…

0d2324c

…t-sdk into alpha-updates

cgillum requested review from halspang and tmacam and removed request for halspang and tmacam January 24, 2023 22:35

halspang approved these changes Jan 24, 2023

View reviewed changes

halspang merged commit 389de69 into dapr:master Jan 24, 2023

halspang added this to the v1.10 milestone Feb 15, 2023

cgillum mentioned this pull request Apr 8, 2023

REQUEST: New membership for cgillum dapr/community#308

Closed

4 tasks

This was referenced Mar 19, 2026

Bump Dapr.Client from 1.4.0 to 1.15.4 azureossd/Container-Apps#128

Closed

Bump Dapr.Actors from 1.0.0 to 1.11.0 azureossd/Container-Apps#129

Merged


		serviceCollection.AddOptions<WorkflowRuntimeOptions>().Configure(configure);

		static bool TryGetGrpcAddress(out string address)

Conversation

cgillum commented Jan 18, 2023

Description

Issue reference

Checklist

Uh oh!

codecov Bot commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cgillum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgillum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmacam left a comment

Choose a reason for hiding this comment

Uh oh!

halspang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov Bot commented Jan 18, 2023 •

edited

Loading