fix(workflows): defer @task serialization off decoration (Approach A, #1259)#50
Merged
Conversation
… #1259) @task called cloudpickle.dumps(fn) at DECORATION time, so importing any module with @task functions serialized every one of them by value — and for local/ closure functions in a heavy-import environment that deep traversal overflowed the recursion limit (catchable RecursionError on Linux, hard C-stack segfault on macOS/musl), whether or not the task was ever dispatched. Approach A (interim fix, tracked in marqov-dev/marqov-platform#1259): move the cloudpickle into a lazy, cached helper that runs only when a task actually builds a graph node. Importing @task modules now serializes nothing. Also drop the write-only _task_func_ref attribute (zero readers in the SDK or the platform; computing it eagerly would defeat the laziness). Residual (not fixed by A): a LOCAL task function still pickles by value at graph-build, so the 13 test_decorators.py cases (which decorate <locals>) stay xfail(strict=False) with an updated reason. Full resolution — referencing importable functions by name — is specced separately for team review. tests/test_task_serialization.py: deterministic contract (spies on cloudpickle.dumps, not the flaky overflow) — decoration never serializes, direct calls never serialize, serialization deferred to graph-build, func_ref cached once, _task_func_ref gone. Graph-build cases pickle a module-level function (by reference) so they're safe under the [all] suite that surfaces the bug.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Interim fix for the
@taskeager-serialization bug (marqov-dev/marqov-platform#1259).Problem
@taskcalledcloudpickle.dumps(fn)at decoration time — so importing any module with@taskfunctions serialized every one of them, by value, at import. For local/closure functions in a heavy-import environment (the[all]extra), cloudpickle's by-value deep traversal overflows the recursion limit: a catchableRecursionErroron Linux, a hard C-stack segfault on macOS/musl. It fired whether or not the task was ever dispatched.Fix (Approach A)
Move the cloudpickle into a lazy, cached helper that runs only when a task actually builds a graph node inside a
@workflow. Importing@taskmodules now serializes nothing. Also drop the write-only_task_func_refattribute (verified zero readers in this repo and in the platform; computing it eagerly would defeat the laziness).This is the interim fix. The durable realignment — referencing importable functions by name instead of shipping pickled bytes (which is what Temporal, Prefect, and Celery all do) — is specced separately for team review.
What this does and doesn't fix
@taskfunctions never pickles.test_decorators.pycases decorate<locals>and stayxfail(strict=False)with an updated reason. Module-level task functions are unaffected (they pickle cheaply, by reference).Tests
tests/test_task_serialization.py(new) locks in the contract deterministically — it spies oncloudpickle.dumpsrather than trying to reproduce the environment-dependent overflow: decoration never serializes, direct calls never serialize, serialization is deferred to graph-build,func_refis cached (pickled once), and_task_func_refis gone. The graph-build cases pickle a module-level function (by reference), so they're safe to run under the[all]suite that surfaces the underlying bug.Verified locally: the 5 contract tests pass; 73 workflow-subsystem tests pass; the 13
test_decorators.pycases xpass in isolation. (The full[all]suite segfaults on macOS — that's the bug — so its verification is CI's job on Linux.)Refs marqov-dev/marqov-platform#1259.
Note
Medium Risk
Changes when and how task functions are serialized for workflow dispatch; behavior for module-level tasks should match, but local/closure tasks still pickle by value at graph-build with the same overflow risk as before.
Overview
@taskno longer runscloudpickleat decoration/import time — the fix for marqov-platform#1259’s import-time recursion/segfault under heavy imports.Serialization moves into a lazy, per-task cached
_func_ref()that runs only when a task registers a graph node inside@workflow. Direct calls outside a workflow still execute the function and never pickle. The unused_task_func_refwrapper attribute is removed so nothing re-introduces eager pickling.Tests: New
tests/test_task_serialization.pyspies oncloudpickle.dumpsto lock in that contract.tests/test_decorators.pystaysxfailfor local tasks at graph-build (residual by-value risk); the marker reason is updated accordingly.Reviewed by Cursor Bugbot for commit 126bf2b. Bugbot is set up for automated code reviews on this repo. Configure here.