Worker Versioning #262

Sushisource · 2025-05-08T23:13:04Z

What was changed

Added the annotations and options for worker-deployment-based versioning

Why?

New feature in all SDKs

Checklist

Closes [Feature Request] Support New Worker Versioning API #237
How was this tested:
New & existing tests
Any docs updates needed?

Sushisource · 2025-05-08T23:13:32Z

also waiting on temporalio/api#579 lol

cretz · 2025-05-09T14:00:57Z

temporalio/lib/temporalio/worker/deployment_options.rb

The require story is a bit confusing in Ruby, but best practice is often to require what's needed in each file (or something you know brings it in), though unfortunately it's not really enforceable with any tooling I know of.

I think we should require 'temporalio/common_enums' in this file (and arguably require 'temporalio/worker_deployment_version' even though we only use it in YARD, but users will use it). Same generally for all files in the lib.

cretz · 2025-05-09T14:03:27Z

temporalio/lib/temporalio/worker_versioning.rb

+# frozen_string_literal: true
+
+module Temporalio
+  WorkerDeploymentVersion = Data.define(


Can we change this filename to worker_deployment_version.rb? While we have a rare file or two that encompass multiple top-level things not relating to the filename, most files should be named after the single module or single class they define/represent.

If we think there may be multiple classes representing versioning, we should consider a versioning module

cretz · 2025-05-09T14:04:37Z

temporalio/lib/temporalio/worker_versioning.rb

+
+    # @!visibility private
+    def _to_bridge_options
+      Internal::Bridge::Worker::WorkerDeploymentVersion.new(


Add require for file containing this

I had no idea that this stuff would just silently magically work if it was imported somewhere else. Ruby feels so footgun-prone to me. Can't say I was a huge fan honestly.

Yes it is footgun-prone, but it's often much easier to read (and for many, write). Just different ecosystem tradeoffs.

cretz · 2025-05-09T14:28:12Z

temporalio/lib/temporalio/worker_versioning.rb

+  class WorkerDeploymentVersion
+    # Parse a version from a canonical string, which must be in the format
+    # `<deployment_name>.<build_id>`. Deployment name must not have a `.` in it.
+    def self.from_canonical_string(canonical)


Add YARD tags for param and return here

cretz · 2025-05-09T14:28:21Z

temporalio/lib/temporalio/worker_versioning.rb

+    end
+
+    # Returns the canonical string representation of the version.
+    def to_canonical_string


Add YARD tags for return here

cretz · 2025-05-09T14:38:48Z

temporalio/lib/temporalio/internal/worker/workflow_worker.rb


+            # Enforce versioning behavior is set when versioning is on
+            if should_enforce_versioning_behavior &&
+               defn.versioning_behavior == VersioningBehavior::UNSPECIFIED && defn.dynamic_options_method.nil?


Suggested change

defn.versioning_behavior == VersioningBehavior::UNSPECIFIED && defn.dynamic_options_method.nil?

defn.versioning_behavior == VersioningBehavior::UNSPECIFIED && !defn.dynamic_options_method

Pedantic, but Ruby is safer than Python in these cases because falsy in Ruby is only nil and false (not 0 or empty string or any of that). But this is fine too.

cretz · 2025-05-09T14:40:32Z

temporalio/lib/temporalio/worker.rb

    # @param build_id [String] Unique identifier for the current runtime. This is best set as a unique value
    #   representing all code and should change only when code does. This can be something like a git commit hash. If
    #   unset, default is hash of known Ruby code.
+    #   Exclusive with `deployment_options`.


Mentioned internally, but I'd be ok just removing build_id and use_worker_versioning since setting them directly is deprecated as I understand it (and we're not GA)

Cool, forgot we had discussed that lol

cretz · 2025-05-09T14:42:59Z

temporalio/lib/temporalio/workflow/definition.rb

-          updates:
+          updates:,
+          versioning_behavior: @versioning_behavior || VersioningBehavior::UNSPECIFIED,
+          dynamic_options_method: @dynamic_options_method


Let's fail in this method if this is non-nil but dynamic is true

cretz · 2025-05-09T14:45:58Z

temporalio/test/test.rb

+          data_converter: Temporalio::Converters::DataConverter.default,
+          interceptors: [],
+          logger: Logger.new($stdout, level: Logger::WARN),
+          default_workflow_query_reject_condition: nil,
+          runtime: Temporalio::Runtime.default


Suggested change

data_converter: Temporalio::Converters::DataConverter.default,

interceptors: [],

logger: Logger.new($stdout, level: Logger::WARN),

default_workflow_query_reject_condition: nil,

runtime: Temporalio::Runtime.default

logger: Logger.new($stdout)

4 of these are already the default, and if we want to adjust log level, we should do it more generally instead of just for this env var based env (though I think default level is fine)

cretz · 2025-05-09T14:46:32Z

temporalio/test/worker_workflow_versioning_test.rb

+require 'test'
+require 'timeout'
+
+class WorkerWorkflowTest < Test


Suggested change

class WorkerWorkflowTest < Test

class WorkerWorkflowVersioningTest < Test

cretz · 2025-05-12T13:26:58Z

temporalio/lib/temporalio/worker_deployment_version.rb

+    #
+    # @param canonical [String] The canonical string representation of the version.
+    # @return [WorkerDeploymentVersion] The parsed version.
+    def self.from_canonical_string(canonical)


Tempted to suggest changing this and the other to parse/to_canonical_s to be a bit more Ruby like, but I think this is fine

cretz · 2025-05-12T13:27:42Z

temporalio/lib/temporalio/worker_deployment_version.rb

+    # Returns the canonical string representation of the version.
+    def to_canonical_string


Suggested change

# Returns the canonical string representation of the version.

def to_canonical_string

# Returns the canonical string representation of the version.

#

# @return [String]

def to_canonical_string

cretz · 2025-05-12T13:31:24Z

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

+          @current_deployment_version = WorkerDeploymentVersion._from_bridge(
+            activation.deployment_version_for_current_task
+          )


Negligible performance benefit, but can also do this lazily (e.g. nil this and @current_deployment_version ||= in a def current_deployment_version method instead of attr_reader) instead of creating a new, mostly-never-used object for every activation (especially one that probably doesn't change much either)

🤷‍♂️ hardly seems worth the extra code

It's not much extra code I don't think, but I think it's a reasonable Ruby pattern to lazy create something that's rarely accessed

cretz · 2025-05-12T13:35:22Z

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

+            ((@workflow_failure_exception_types || []) +
+             (@definition_options.failure_exception_types || [])).any? do |cls|
+              err.is_a?(cls)
+            end


Suggested change

((@workflow_failure_exception_types || []) +

(@definition_options.failure_exception_types || [])).any? do |cls|

err.is_a?(cls)

end

@workflow_failure_exception_types&.any? { |cls| err.is_a?(cls) } ||

@definition_options.failure_exception_types&.any? { |cls| err.is_a?(cls) }

err.is_a?(cls)

end

Reads a bit easier to me. Sure rubocop may change to blocks or whatever, but concatenating and creating empty arrays when not needed seems unnecessary.

cretz · 2025-05-12T13:36:51Z

temporalio/lib/temporalio/internal/worker/workflow_worker.rb

      # Worker for handling workflow activations. Most activation work is delegated to the workflow executor.
      class WorkflowWorker
-        def self.workflow_definitions(workflows)
+        def self.workflow_definitions(workflows, should_enforce_versioning_behavior)


Suggested change

def self.workflow_definitions(workflows, should_enforce_versioning_behavior)

def self.workflow_definitions(workflows, should_enforce_versioning_behavior:)

Internal code so doesn't matter much, but I recommend kwargs instead of positional for these kinds of trailing booleans

cretz · 2025-05-12T13:39:46Z

temporalio/lib/temporalio/worker.rb

      activity_executors: ActivityExecutor.defaults,
      workflow_executor: WorkflowExecutor::ThreadPool.default,
      interceptors: [],
-      build_id: Worker.default_build_id,


Hrmm, now I am wondering how we get the behavior of default build ID back. People expect a default build ID from our SDKs. This makes me wonder if we should revisit deprecating build-ID-only across SDKs since that's what is default for all SDKs today.

I am bringing up the deprecate-build-id-only thing internally again with this concern.

Internally discussed and recognized that the build-id-sans-deployment-name approach has to be supported since it's the default in every SDK. So I think we should make deployment_options be Worker.default_deployment_options and self.default_deployment_options be a method with @default_deployment_options ||= DeploymentOptions.new(version: WorkerDeploymentVersion.new(deployment_name: '', build_id: default_build_id)).

cretz · 2025-05-12T13:46:09Z

temporalio/lib/temporalio/worker.rb

          task_queue:,
          tuner: tuner._to_bridge_options,
-          build_id:,
+          build_id: '',


Why have the bridge accept values that are never present any more?

cretz

Looks like there's a YARD complaint (can bundle exec rake yard to see errors locally)

cretz

Only a minor suggestion, then LGTM. I have opened an issue on a test flake I saw in your CI: #268.

cretz · 2025-05-14T18:45:31Z

temporalio/lib/temporalio/worker.rb

      workflow_failure_exception_types: [],
      workflow_payload_codec_thread_pool: nil,
      unsafe_workflow_io_enabled: false,
+      deployment_options: nil,


Can we set this as the default right here, e.g.:

Suggested change

deployment_options: nil,

deployment_options: Worker.default_deployment_options,

and add a method near the top like:

def self.default_deployment_options @default_deployment_options ||= DeploymentOptions.new( version: WorkerDeploymentVersion.new(deployment_name: '', build_id: Worker.default_build_id) ) end

It's a bit pedantic, but a really nice feature of Ruby that Python and others don't even have. And it allows people to programmatically access the default, make their own based on it, etc. It's a good way to set default parameter values.

Sure, can do

Sushisource requested a review from a team as a code owner May 8, 2025 23:13

cretz reviewed May 9, 2025

View reviewed changes

Sushisource force-pushed the worker-versioning branch from d283e05 to d396cb5 Compare May 9, 2025 23:27

cretz reviewed May 12, 2025

View reviewed changes

Sushisource added 16 commits May 14, 2025 10:31

README updates

ba5edd3

Update Core

3022ab3

Test and initial impl

879151a

First test working

058d65d

Add more tests

184168c

Dynamic config getter working

9c8aa43

Remove somehow added .idea files

62e8711

Add default setting tests

5b06a18

Move test to own file

cb37b69

Review comments

73619f0

Regenerate Protos

87177d5

RBS files and fixes

067852c

More review comments

380a46a

Auto-set build ID

4d5ac43

Update Core & protos

24841ac

Fix type errors

4f62b9a

Sushisource force-pushed the worker-versioning branch from cc2e1f8 to 7b268e3 Compare May 14, 2025 17:36

Add versioning override

b3604f9

Sushisource force-pushed the worker-versioning branch from 7b268e3 to b3604f9 Compare May 14, 2025 17:54

cretz reviewed May 14, 2025

View reviewed changes

Last review

2ad4178

cretz approved these changes May 14, 2025

View reviewed changes

Sushisource enabled auto-merge (squash) May 14, 2025 20:56

Sushisource merged commit df0a34f into main May 14, 2025
12 checks passed

Sushisource deleted the worker-versioning branch May 14, 2025 21:05

	defn.versioning_behavior == VersioningBehavior::UNSPECIFIED && defn.dynamic_options_method.nil?
	defn.versioning_behavior == VersioningBehavior::UNSPECIFIED && !defn.dynamic_options_method

	class WorkerWorkflowTest < Test
	class WorkerWorkflowVersioningTest < Test

		# Returns the canonical string representation of the version.
		def to_canonical_string

	def self.workflow_definitions(workflows, should_enforce_versioning_behavior)
	def self.workflow_definitions(workflows, should_enforce_versioning_behavior:)

	deployment_options: nil,
	deployment_options: Worker.default_deployment_options,

Worker Versioning #262

Worker Versioning #262

Uh oh!

Conversation

Sushisource commented May 8, 2025

What was changed

Why?

Checklist

Uh oh!

Sushisource commented May 8, 2025

Uh oh!

cretz May 9, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz May 9, 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!

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!

Choose a reason for hiding this comment

Uh oh!

cretz May 13, 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!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

cretz left a comment • 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!

cretz May 14, 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!

Reviewers

Assignees

Labels

cretz May 9, 2025 •

edited

Loading

cretz May 9, 2025 •

edited

Loading

cretz May 13, 2025 •

edited

Loading

cretz left a comment •

edited

Loading

cretz May 14, 2025 •

edited

Loading