Workflow support #183

cretz · 2025-01-07T22:19:54Z

What was changed:

Added full workflow support including:

Full workflow worker with deterministic fiber scheduler
All workflow functionality/features
Workflow async via "future"s
Checks to disallow known non-deterministic calls from inside workflows
Updates to client to support providing direct workflow, signal, query, and update definitions
Time-skipping workflow test environment
Sigs and tests for all
💥 BREAKING CHANGE - Activity classes now have to extend from Temporalio::Activity::Definition instead of just Temporalio::Activity

Not implemented yet:

Ractor-based sandboxing - [Feature Request] Ractor-based sandboxing #184 - Right now we require you set the workflow executor because of this
Warn on unawaited futures with failures - [Feature Request] Warn on unawaited workflow futures with failures #185
Workflow replayer - [Feature Request] Workflow replayer #187
Signal and update with start - [Feature Request] Signal-with-start and update-with-start #188

Want to help review?

Great! We welcome all reviews/feedback from everyone. If the PR gets too many comments on it, we may create a new PR for another round of comments. I know the 160-file-count can seem daunting, but a lot of it is generated or unimportant code.

What type of reviewer do you want to be?

I want to review high-level design only

Review README.md (rendered here).

I want to review the workflow features but do not care about the implementation

In addition to README.md, also review files starting with temporalio/test/worker_workflow_.

I want to review the Ruby implementation but do not want to dig into the Rust side

Review everything but what's in temporalio/ext.

I want to help review everything including the Rust side

Review everything.

yuandrew

Only reviewed the README.md, looks great! Mostly a few minor grammar comments. Progress is looking good 😊

README.md

yuandrew · 2025-01-08T21:31:05Z

README.md

+
+A time-skipping `Temporalio::Testing::WorkflowEnvironment` can be started via `start_time_skipping` which is a
+reimplementation of the Temporal server with special time skipping capabilities. This too lazily downloads the process
+to run when first called. Note, this class is not thread safe nor safe for use with independent tests. It can be reused,


with 2 "Note"s back to back, should they be on separate lines, for clarity?

Not following. I have a short paragraph for non-time-skipping and a short paragraph for time-skipping.

The original thought was 2 sentences back to back starting with "Note," felt a little strange, but the more I think about it, I think it's fine

README.md

antlai-temporal · 2025-01-09T15:01:46Z

README.md

 client = Temporalio::Client.connect('localhost:7233', 'my-namespace')



Do you want to add something about error management (maybe a comment that it throws?)

Error management of which part, workers? Not really needed in the README any more than it is in Python on .NET READMEs. Worker lifecycle including shutdown can get a bit too complex for the README, but is documented in the API docs.

It was more like if connect fails do we throw an standard temporal error, or a vanilla Ruby error, or we just return nil, or ...

What if connect fails in any other SDK? Their READMEs aren't expected to provide that information either. Like all other SDKs, client connection failure is represented like any other method error (so in Ruby's case it raises an exception, API docs are clear that this cannot return nil). We can get specific in API docs on which error if needed, though most SDKs don't for client connect.

README.md

antlai-temporal · 2025-01-09T15:19:59Z

README.md

+  workflows: [MyModule::MyWorkflow],
+  # There are various forms an activity can take, see "Activities" section for details


Is there a way to add Options, i.e., RegisterWithOptions...

Details of customizing activities and workflows are in their respective sections in the README

antlai-temporal · 2025-01-09T15:21:37Z

README.md

@@ -309,21 +372,493 @@ Notes about the above code:

 * A worker uses the same client that is used for other Temporal things.
 * This just shows providing an activity class, but there are other forms, see the "Activities" section for details.
+* The `workflow_executor` defaults to `Temporalio::Worker::WorkflowExecutor::Ractor.instance` which intentionally does


Why is it intentional that it does not work?

Because we haven't completely built it but if/when we do (still some decisions to make, see #184), we would want it to be the default. So we can't default to the working one today then just switch defaults on the user later. So during beta we require the user to explicitly set this value since the default does not work yet. This is kinda touched on in the PR description, though not much.

antlai-temporal · 2025-01-09T15:25:29Z

README.md

+Workflows are defined as classes that extend `Temporalio::Workflow::Definition`. The entry point for a workflow is
+`execute` and must be defined. Methods for handling signals, queries, and updates are marked with `workflow_signal`,


What about the constructor? Do we need one? Does it have any special requirements?

No SDK that uses class-based workflows requires a constructor (and other SDK READMEs also don't need to clarify this since it's the default situation). However, an advanced workflow_init feature does exist and is touched on below. This README, like the Python and .NET ones, do not go into detail on the advanced workflow init constructor.

antlai-temporal · 2025-01-09T15:29:01Z

README.md

+  workflow_update
+  def update_greeting_params(greeting_params_update)
+    @greeting_params_update = greeting_params_update
+  end


It is is not clear what is returned from the update, maybe be more explicit, or add comment...

This is normal Ruby code and it is clear to Ruby developers for these basic snippets. It's the value of the last statement, which in this case is the same update passed in. We could add nil as the last statement to return nil. We do not want to add full-blown YARD docs to all of the code snippets here though spelling out param and return types as it detracts from the small code.

antlai-temporal · 2025-01-09T15:32:31Z

README.md

+To start a workflow from a client, you can `start_workflow` and use the resulting handle:
+


Can you talk about error handling? What is thrown, best practices...

Assuming you're talking about error handling from the start and execute workflow calls, We do not have that detail in any of the SDK READMEs and would not like to expand the README to that level. I would expect official docs and API docs to both clarify that though.

antlai-temporal · 2025-01-09T15:35:11Z

README.md

+handle.execute_update(
+  GreetingWorkflow.update_greeting_params,
+  { salutation: 'Aloha', name: 'John' }
+)


Can you add a return value, it looks the same to signal below...

This is an update that basically has no important return. Sometimes updates do look like signals and the return is void and/or unimportant to the caller, only the action of the update.

I understand, but if people are not familiar with signal vs update they will see them similarly if update does not return something useful in the example...

I think we can delegate that to official docs and samples instead of forcing the README to do it. Some of our READMEs don't have update at all. This README is not meant to be the full docs. For this use, yes, you could basically use a signal. Maybe I will switch to signal and leave update out entirely.

antlai-temporal · 2025-01-09T15:40:00Z

README.md

+#### Workflow Fiber Scheduling and Cancellation
+


A tiny example with cancellation will be useful here to understand how it works...

I don't think this or any SDK README needs to get into that level of detail. But would expect official docs to do so.

antlai-temporal · 2025-01-09T15:43:03Z

README.md

+safe wrapper around `Fiber.schedule` for starting and `Workflow.wait_condition` for waiting.
+


A comment that is deterministic during replay may help people not familiar with how Temporal works...

Not following, can you clarify a bit what you mean? All of our Temporal utilities are deterministic during replay (unless they are in the "unsafe" area), this utility is not unique and I wouldn't expect to have to make such a statement in each section.

In that section you have Future.any_of ,and people get confused why it will always get the same one on replay.

I'm not sure they get confused about that any more than why the response is the same one for every utility on replay. Not sure how this one utility's return value is special there.

Sushisource

Hoo boy that is long.

Mostly focused on the worker/event loop parts. Looks good to me. Definitely feels a bit simpler than some of the others which is nice.

One thing I definitely do not love about Ruby: Class field initialization happening in any 'ol place instead of well-defined fields.

README.md

Sushisource · 2025-01-11T01:14:37Z

README.md

+The following protected class methods can be called just before defining instance methods to customize the
+definition/behavior of the method:


Isn't "called just before" really something like "taking instance methods as an argument"?

Might be a tad more clear, if Ruby docs don't usually say it that way then no prob.

These are method decorators, so it wouldn't be correct to say the instance method is an argument.

I don't think Ruby docs have a common way of describing this (because it's pretty uncommon), but I think "called just before" works well.

Yeah, this is not the most common "Ruby-ism" but after discussion in the proposal, we found it was the best way to both "color" a method and make its definition available as a class method (as opposed to something like workflow_signals :foo, :bar at the top of the class). I figured Ruby developers would understand the concept of "called just before".

Sushisource · 2025-01-11T01:18:19Z

README.md

+
+#### Timers and Conditions
+
+* A timer is represented by `Temporalio::Workflow.sleep`.


Wahooo explicit

Yeah, needed for things like summary and cancellation override, but per a couple of bullets below, technically we support regular sleep/timeout and just discourage it. Ruby devs kinda expect those to work and they are delegated to the scheduler.

Sushisource · 2025-01-11T01:18:55Z

README.md

+  * Each wait conditions accepts a `Cancellation`, but if none is given, it defaults to
+    `Temporalio::Workflow.cancellation`.


Maybe make a mention of what happens with 0 value timers since that discussion came up again recently and seems a periodic point of confusion

I do make this clear in the API docs:

# @param duration [Float, nil] Time to sleep in seconds. `nil` represents infinite, which does not start a timer and # just waits for cancellation. `0` is assumed to be 1 millisecond and still results in a server-side timer. This # value cannot be negative. Since Temporal timers are server-side, timer resolution may not end up as precise as # system timers.

This is like .NET and gets rid of that concern of causing non-determinism when moving between zero and non-zero. Not sure we need to clarify in the README though.

👍 That's good then

README.md

Sushisource · 2025-01-11T01:45:54Z

temporalio/lib/temporalio/internal/worker/workflow_instance.rb

+            # TODO(cretz): Use the details somehow?
+            @cancellation_proc.call(reason: 'Workflow canceled')


May as well include them in another field or something

Unfortunately these aren't really wired up to cancellation client call or server event in any way I can see so I can't really test it

temporalio/lib/temporalio/internal/worker/workflow_instance.rb

Sushisource · 2025-01-11T01:53:54Z

temporalio/lib/temporalio/internal/worker/workflow_instance.rb

+          rescue Exception => e # rubocop:disable Lint/RescueException
+            if top_level
+              on_top_level_exception(e)
+            else
+              @current_activation_error ||= e
+            end


Is it possible for multiple errors in different scheduled fibers to clobber each other in the same activation?

Yes, definitely possible, though each non-top-level scheduled fiber is expected to take care of its own errors (which is only query and update atm). But yes, we only track/return/log the first one raised (other newer SDKs do similar).

It might be worth pushing them into a list so they can all be shown, but, probably not a huge deal.

temporalio/lib/temporalio/internal/worker/workflow_instance/scheduler.rb

…signal

dwillett · 2025-01-13T17:03:46Z

README.md

+end
+
+# Wait for them all to complete
+Temporalio::Workflow.Future.all_of(fut1, fut2, fut3).wait


suggestion: typo

Suggested change

Temporalio::Workflow.Future.all_of(fut1, fut2, fut3).wait

Temporalio::Workflow::Future.all_of(fut1, fut2, fut3).wait

Thanks! Fixing now. EDIT: Fixed

As a side node, thanks for reviewing! We also welcome any general/high-level feedback you may have on our design/approach here or in #ruby-sdk at https://t.mp/slack or via any avenue you'd like.

cretz · 2025-01-14T16:02:21Z

Merging, but reviews can still be made in here if anyone would like and they will still be read/applied (they just aren't marked "approved" or not)

mjameswh · 2025-01-14T16:17:45Z

temporalio/test/test.rb

@@ -72,6 +74,10 @@ def skip_if_fibers_not_supported!
    skip('Fibers not supported in this Ruby version')
  end

+  def skip_if_not_x86!
+    skip('Test only supported on x86') unless RbConfig::CONFIG['host_cpu'] == 'x86_64'


nit: I'd suggest making this predicate more targeted, i.e. skip_if_test_server_not_supported and allow arm64 on macos.

Most devs in the team are working on Apple M* cpus, and as it is now, they wouldn't be running those tests localy even though they technically could. I know there's very few tests that depends on that condition, but still…

Makes sense, will change

mjameswh · 2025-01-14T16:28:46Z

temporalio/test/test.rb

+        logger: Logger.new($stdout),
+        dev_server_extra_args: [
+          # Allow continue as new to be immediate
+          '--dynamic-config-value', 'history.workflowIdReuseMinimalInterval="0s"'


No, really?!? TIL!

README.md

mjameswh · 2025-01-14T17:10:25Z

README.md

+  workflows: [MyModule::MyWorkflow],
+  # There are various forms an activity can take, see "Activities" section for details
+  activities: [MyModule::MyActivity],
+  # During the beta period, this must be provided explicitly, see below for details


Not sure about the "beta period" expression (it is used in a few place in this README file). It is not part of our usual terminology, and it's not clear to me how that would map to our actual version scheme. And anyway, is it really bound to a specific release, like that will be this way until 1.0, then change from that point on? Or is it just something not yet implemented? If the later, might just say "For now".

The concept of a "beta" library is well known to developers, especially Ruby ones. We need to decide what to do with this default by 1.0. This will not be a required field in 1.0, but we are not sure what will be the default yet, so we are requiring it be explicitly set.

mjameswh · 2025-01-14T17:16:13Z

README.md

-⚠️ Workflows cannot yet be implemented Ruby.
+#### Workflow Definition
+
+Workflows are defined as classes that extend `Temporalio::Workflow::Definition`. The entry point for a workflow is


are marked ... just before the method is defined

nit: Doesn't Ruby community have some common term for that pattern? e.g. anotations?

Not really, this is not that common of a pattern. Also discussed a bit in another comment thread: #183 (comment)

cretz added 3 commits November 4, 2024 15:01

Change Activity base class to Activity::Definition

71547ac

Extract out activity thread pool for general use

0d0f841

Work on workflows

a9ca51d

cretz force-pushed the workflows branch from efb9a81 to a9ca51d Compare January 8, 2025 14:21

cretz added 2 commits January 8, 2025 08:51

CI fixes

e44f2b3

Merge remote-tracking branch 'remotes/origin/main' into workflows

0f03ad4

cretz force-pushed the workflows branch 3 times, most recently from f355a1d to fd35a1b Compare January 8, 2025 19:23

cretz marked this pull request as ready for review January 8, 2025 20:23

CI fixes

4e7f60f

cretz force-pushed the workflows branch from fd35a1b to 4e7f60f Compare January 8, 2025 20:33

cretz requested a review from a team January 8, 2025 21:37

yuandrew reviewed Jan 8, 2025

View reviewed changes

cretz commented Jan 9, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

Minor README updates

65247ce

antlai-temporal reviewed Jan 9, 2025

View reviewed changes

README.md Show resolved Hide resolved

antlai-temporal reviewed Jan 9, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

antlai-temporal reviewed Jan 9, 2025

View reviewed changes

cretz mentioned this pull request Jan 10, 2025

[Feature Request] Expose replay-safe metric meter from workflows #186

Closed

Support for custom metrics in workflows and minor README fixes

0656115

cretz force-pushed the workflows branch from 05dac86 to 0656115 Compare January 10, 2025 20:00

Sushisource approved these changes Jan 11, 2025

View reviewed changes

cretz mentioned this pull request Jan 13, 2025

[Feature Request] Ensure signal buffer properly drained when dynamic signal added #190

Closed

cretz added 3 commits January 13, 2025 09:32

Minor PR suggestions

739c55e

Support draining on buffered signals on manual definition of dynamic …

fd20680

…signal

Couple more tests

c528fd5

dwillett reviewed Jan 13, 2025

View reviewed changes

cretz added 4 commits January 13, 2025 11:18

README typo

505d557

Minor fixes with imports and headers

c4423dd

Switch CI to 3.2 knowing we will do it officially soon anyways

48507e7

TracePoint leak and test fixes

19be2d2

cretz merged commit 4512224 into temporalio:main Jan 14, 2025
6 checks passed

cretz deleted the workflows branch January 14, 2025 16:02

mjameswh reviewed Jan 14, 2025

View reviewed changes

README.md Show resolved Hide resolved

mjameswh reviewed Jan 14, 2025

View reviewed changes

		client = Temporalio::Client.connect('localhost:7233', 'my-namespace')

		workflows: [MyModule::MyWorkflow],
		# There are various forms an activity can take, see "Activities" section for details

		Workflows are defined as classes that extend `Temporalio::Workflow::Definition`. The entry point for a workflow is
		`execute` and must be defined. Methods for handling signals, queries, and updates are marked with `workflow_signal`,

		To start a workflow from a client, you can `start_workflow` and use the resulting handle:

		safe wrapper around `Fiber.schedule` for starting and `Workflow.wait_condition` for waiting.

		The following protected class methods can be called just before defining instance methods to customize the
		definition/behavior of the method:


		#### Timers and Conditions

		* A timer is represented by `Temporalio::Workflow.sleep`.

		* Each wait conditions accepts a `Cancellation`, but if none is given, it defaults to
		`Temporalio::Workflow.cancellation`.

		# TODO(cretz): Use the details somehow?
		@cancellation_proc.call(reason: 'Workflow canceled')

	Temporalio::Workflow.Future.all_of(fut1, fut2, fut3).wait
	Temporalio::Workflow::Future.all_of(fut1, fut2, fut3).wait

Workflow support #183

Workflow support #183

Conversation

cretz commented Jan 7, 2025 • edited Loading

What was changed:

Want to help review?

I want to review high-level design only

I want to review the workflow features but do not care about the implementation

I want to review the Ruby implementation but do not want to dig into the Rust side

I want to help review everything including the Rust side

yuandrew 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jan 9, 2025 • 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

cretz Jan 9, 2025 • 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

Sushisource 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

cretz Jan 13, 2025 • 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

cretz Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

cretz commented Jan 14, 2025

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 commented Jan 7, 2025 •

edited

Loading

cretz Jan 9, 2025 •

edited

Loading

cretz Jan 9, 2025 •

edited

Loading

cretz Jan 9, 2025 •

edited

Loading

cretz Jan 9, 2025 •

edited

Loading

cretz Jan 9, 2025 •

edited

Loading

cretz Jan 13, 2025 •

edited

Loading

cretz Jan 13, 2025 •

edited

Loading

cretz Jan 13, 2025 •

edited

Loading