Dynamic workflows and activities #1946

yuandrew · 2025-05-02T05:28:44Z

What was changed

Added support for dynamic workflows and activities.

Wrote a sample-go sample temporalio/samples-go#404, will merge once this feature goes in

Why?

Feature parity with other SDKs

Checklist

Closes Support dynamic activity and workflow implementations #543
Queries, signals, updates are tracked in a different issue: Support dynamic query and update handler #997
How was this tested:
Added new unit tests and integration tests
Any docs updates needed?

internal/internal_worker.go

internal/workflow.go

cretz · 2025-05-06T12:41:12Z

internal/encode_args.go

@@ -80,6 +80,11 @@ func decodeArgsToRawValues(dc converter.DataConverter, fnType reflect.Type, data
 	}

 	// Unmarshal
+
+	// Dynamic workflows take in a single EncodedValues, encode all data into single EncodedValues


I thought it would be varargs of RawValue, but encoded values makes sense too

It is unfortunate we have both concepts, but EncodedValues is more appropriate for dynamic workflows

Hrmm. I think they're similar from a user POV for params (I personally prefer codec to apply outside workflow boundary), but we should allow any return value IMO.

In some SDKs we do not require a certain signature for a dynamic workflow. So if they want a dynamic workflow that accepts a string parameter, ok, it'll just fail with others. We offer the raw value option to all workflows including dynamic.

Yeah the dynamic workflow should be allowed to return anything I agree, I was referring to the input type

cretz · 2025-05-06T12:41:45Z

internal/encode_args.go

@@ -80,6 +80,11 @@ func decodeArgsToRawValues(dc converter.DataConverter, fnType reflect.Type, data
 	}

 	// Unmarshal
+
+	// Dynamic workflows take in a single EncodedValues, encode all data into single EncodedValues
+	if fnType.NumIn() == 2 && len(pointers) == 1 && fnType.In(1) == reflect.TypeOf((*converter.EncodedValues)(nil)).Elem() {


Hrmm, wonder if it's worth a more specific check whether it's a dynamic workflow/activity instead of assuming so based on signature? Maybe not worth it.

Yeah I would consider a more specific check or just not sue this function for dynamic workflows/activities

internal/internal_worker.go

cretz · 2025-05-06T12:50:19Z

internal/internal_worker.go

@@ -929,7 +1011,10 @@ func (we *workflowExecutor) Execute(ctx Context, input *commonpb.Payloads) (*com
 	// Execute and serialize result
 	result, err := envInterceptor.inboundInterceptor.ExecuteWorkflow(ctx, &ExecuteWorkflowInput{Args: args})
 	var serializedResult *commonpb.Payloads
-	if err == nil && result != nil {
+	// Dynamic workflows always return EncodedValues, skip encoding because result should already be encoded


Do they always return encoded values? There is no validation of this return type at registration time, and they don't really need to. For many, it may make more sense to return an any or a RawValue. Also, even if we do support encoded value as a return type, one might expect a single encoded value instead of plural (or at least have the option).

Yeah it is a good question, In Java we require dynamic workflows to return an Object so that is like any in Go.

Instead of any, I think we should have no requirement on return type at all. Returning a RawValue should be just fine for example. Even returning a string or no return value besides error I think should be fine. Although we did in the past, we don't have to require strict signatures nowadays for dynamic things if we don't want.

I'm in favor of matching the other non-Java SDKs and not requiring a return value. I don't think adding a return type requirement adds any guard rails for the users, so I wanna say matching behavior of the majority of the SDKs would be better.

internal/internal_worker.go

internal/workflow.go

cretz · 2025-05-06T12:57:07Z

internal/workflow.go

+	// DynamicWorkflowOptions are options for a dynamic workflow.
+	//
+	// Exposed as: [go.temporal.io/sdk/workflow.DynamicWorkflowOptions]
+	DynamicWorkflowOptions struct {


I wonder if we need a different name here. The concept of "workflow options" is traditionally used by workflow callers to set options about the workflow they are calling (e.g. StartWorkflowOptions and ChildWorkflowOptions).

yeah I see what you're saying, maybe config?

Unsure, maybe DynamicWorkflowWorkerOptions or something like that. @Quinn-With-Two-Ns - any ideas?

RegisterDynamicWorkflowOptions ? similar to RegisterWorkflowOptions

internal/internal_worker.go

Quinn-With-Two-Ns · 2025-05-06T15:53:54Z

I think this is a great case to try adding a sample and link to it in the PR, what do you think @yuandrew ?

yuandrew · 2025-05-06T19:20:09Z

Good idea, added a sample and linked in PR description

internal/workflow_testsuite.go

cretz

Everything LGTM but still hung up on the naming of the type returned from LoadDynamicOptions

cretz · 2025-05-14T13:21:02Z

internal/workflow.go

+	// Exposed as: [go.temporal.io/sdk/workflow.DynamicRegisterOptions]
+	DynamicRegisterWorkflowOptions struct {
+		// Allows dynamic options to be loaded for a workflow.
+		LoadDynamicOptions func(details LoadDynamicOptionsDetails) (RegisterDynamicWorkflowOptions, error)


The more I think about this, the concept of DynamicRegisterWorkflowOptions (even if exposed as DynamicRegisterOptions) and RegisterDynamicWorkflowOptions is just too confusingly named.

I think we should use the word "runtime" in here maybe, e.g.:

Suggested change

LoadDynamicOptions func(details LoadDynamicOptionsDetails) (RegisterDynamicWorkflowOptions, error)

LoadDynamicRuntimeOptions func(details LoadDynamicRuntimeOptionsDetails) (DynamicRuntimeWorkflowOptions, error)

Fine with me, whatever we do here we should also change the dynamic activity options name

This is only specifically concerning runtime loading of workflow options by workflow type and has no activity anallog at this time

internal/internal_worker.go

Quinn-With-Two-Ns · 2025-05-14T14:47:10Z

internal/internal_worker.go

@@ -978,8 +1082,11 @@ func (ae *activityExecutor) ExecuteWithActualArgs(ctx context.Context, args []in
 	result, resultErr := interceptor.ExecuteActivity(ctx, &ExecuteActivityInput{Args: args})
 	var serializedResult *commonpb.Payloads
 	if result != nil {
+		// Dynamic activities always return EncodedValues, skip encoding because result should already be encoded


Didn't we say we wanted dynamic workflows and activities to be able to return any value?

yuandrew added 6 commits May 1, 2025 10:52

WIP

e62fa22

Added test for registering multiple dynamic activities

0550bd9

varags of EncodedValue doesn't work

9f2b2c3

test almost working, extra {} for some reason around

32fec83

need .name to get rid of brackets

5f8de5e

activities work now too

fd4910e

yuandrew commented May 6, 2025

View reviewed changes

internal/internal_worker.go Outdated Show resolved Hide resolved

clean up

ad35c64

yuandrew force-pushed the dynamic-workflows branch from 0dba4f7 to ad35c64 Compare May 6, 2025 05:05

yuandrew commented May 6, 2025

View reviewed changes

internal/workflow.go Outdated Show resolved Hide resolved

yuandrew changed the title ~~[draft] Dynamic workflows~~ Dynamic workflows and activities May 6, 2025

yuandrew marked this pull request as ready for review May 6, 2025 05:11

yuandrew requested a review from a team as a code owner May 6, 2025 05:11

cretz reviewed May 6, 2025

View reviewed changes

Quinn-With-Two-Ns reviewed May 6, 2025

View reviewed changes

internal/internal_worker.go Outdated Show resolved Hide resolved

yuandrew mentioned this pull request May 6, 2025

Add dynamic workflow sample temporalio/samples-go#404

Draft

Fix lock location, add dynamic activity options,

112feba

Quinn-With-Two-Ns reviewed May 9, 2025

View reviewed changes

internal/workflow_testsuite.go Show resolved Hide resolved

workflow_testsuite dynamic workflow test, fix other test

8470520

cretz reviewed May 14, 2025

View reviewed changes

Quinn-With-Two-Ns reviewed May 14, 2025

View reviewed changes

internal/internal_worker.go Show resolved Hide resolved

Quinn-With-Two-Ns reviewed May 14, 2025

View reviewed changes

yuandrew added 4 commits May 14, 2025 10:32

rename, cleanup

73d24c8

Merge remote-tracking branch 'upstream/master' into dynamic-workflows

1cf5548

set unused field to _

5e36c14

forgot a rename spot

7434edc

cretz approved these changes May 15, 2025

View reviewed changes

Quinn-With-Two-Ns approved these changes May 16, 2025

View reviewed changes

Merge branch 'master' into dynamic-workflows

38a1800

yuandrew enabled auto-merge (squash) May 16, 2025 00:31

yuandrew merged commit f55d382 into temporalio:master May 16, 2025
14 checks passed

yuandrew deleted the dynamic-workflows branch May 16, 2025 01:07

	LoadDynamicOptions func(details LoadDynamicOptionsDetails) (RegisterDynamicWorkflowOptions, error)
	LoadDynamicRuntimeOptions func(details LoadDynamicRuntimeOptionsDetails) (DynamicRuntimeWorkflowOptions, error)

Dynamic workflows and activities #1946

Dynamic workflows and activities #1946

Uh oh!

Conversation

yuandrew commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Quinn-With-Two-Ns commented May 6, 2025

Uh oh!

yuandrew commented May 6, 2025

Uh oh!

Uh oh!

cretz 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yuandrew commented May 2, 2025 •

edited

Loading

cretz May 6, 2025 •

edited

Loading

cretz May 6, 2025 •

edited

Loading