Skip to content

fix: update Workflows wrapper to work more natively with Python objects#138

Merged
ryanking13 merged 3 commits into
mainfrom
gyeongjae/workflows-type-conversion
Jul 3, 2026
Merged

fix: update Workflows wrapper to work more natively with Python objects#138
ryanking13 merged 3 commits into
mainfrom
gyeongjae/workflows-type-conversion

Conversation

@ryanking13

Copy link
Copy Markdown
Contributor

Another bindings update. This update the existing Workflow wrapper classes to take and return native Python objects, so that users does not have to use to_js explicitly.


Context for workflows team: We are updating Python workers SDK so that users does not have to user pyodide FFIs at all, and use the native Python objects directly. After this update, we can remove all the to_js usage in workflows docs (https://developers.cloudflare.com/workflows/python/bindings/)

@ask-bonk

ask-bonk Bot commented Jun 22, 2026

Copy link
Copy Markdown

Now I have a comprehensive understanding of the changes. Let me assess any actionable issues:

  1. _WorkflowInstanceWrapper inherits _convert_result which uses self.__class__: When _BindingWrapper._convert_result encounters a remaining JsProxy that should be wrapped, it wraps with self.__class__(converted), which would be _WorkflowInstanceWrapper. This could be problematic if, say, status() returns a complex object that still has JsProxy parts — they'd get wrapped as _WorkflowInstanceWrapper instead of _BindingWrapper. However, this is the existing pattern used by _BindingWrapper subclasses, and the tests verify the behavior works correctly. The workflow status data should be fully converted by python_from_rpc anyway.

  2. _WorkflowBindingWrapper doesn't have __getattr__ snake-to-camel fallback: If someone calls e.g. env.MY_WORKFLOW.create_batch(...), it hits the explicit method. But _BindingWrapper.__getattr__ would forward create_batch as-is to the JS binding, which expects createBatch. This is fine because the explicit method handles it. If someone calls env.MY_WORKFLOW.createBatch(...) directly (the JS name), it would work through __getattr__. No issue here.

  3. The _WorkflowInstanceWrapper comment says only send_event needs snake-to-camel mapping: But what about _WorkflowBindingWrapper.create_batchcreateBatch? The comment at line 1271-1272 is slightly misleading because it only applies to _WorkflowInstanceWrapper, not _WorkflowBindingWrapper. The comment is scoped to the right class, so it's accurate.

  4. Test coverage: The tests are thorough — they cover create, get, status, batch, send_event, step types (do, sleep, sleep_until, wait_for_event), error handling, retry config, context, event metadata, and backward compat with legacy to_js pattern.

Let me check one more thing — whether _poll_until_settled has a potential issue. It initializes status before the loop, but if the initial status is already non-queued/non-running, it returns immediately. The elapsed starts at 0.0, and the first check inside the while loop checks status["status"] not in ("queued", "running"). If the initial status already satisfies this, it returns before any sleep. Good.

Actually, wait — I notice a subtle issue in _poll_until_settled. Let me look again:

async def _poll_until_settled(instance, timeout=20.0):
    elapsed = 0.0
    interval = 0.1
    status = await instance.status()  # <-- First poll, before the loop
    while elapsed < timeout:
        if status["status"] not in ("queued", "running"):
            return status
        await asyncio.sleep(interval)
        elapsed += interval
        status = await instance.status()
    return status  # <-- Returns last status even if not settled

This is fine — the function polls, sleeps, polls again. If timeout is exceeded, it returns the last status (leaving it to the caller to decide what to do). The caller at line 83-84 handles this correctly by asserting settled["status"] not in TERMINAL.

Overall, this is a clean, well-tested change. The core logic changes are:

  1. Workflow wrappers now extend _BindingWrapper for automatic argument/result conversion
  2. wait_for_event now properly awaits and converts the result
  3. wrapped_run converts return values back to JS via python_to_rpc
  4. _convert_args was extracted as a static method for reuse

I don't see any actionable logic bugs, security issues, backward compat violations, or missing edge cases. The tests cover both the new native Python object path and the legacy to_js path.

LGTM

github run

@dom96 dom96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but would like @Caio-Nogueira to take a look as well.

@Caio-Nogueira

Copy link
Copy Markdown

Yeah LGTM, as long as it doesn't break backwards compatibility with current callers that explicitly use to_js and to_py

@ryanking13 ryanking13 merged commit 63ea6a0 into main Jul 3, 2026
13 checks passed
@ryanking13 ryanking13 deleted the gyeongjae/workflows-type-conversion branch July 3, 2026 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants