Skip to content

Data converter non-string keys #833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 18, 2025

Conversation

dandavison
Copy link
Contributor

@dandavison dandavison commented Apr 18, 2025

Before this PR, workflows and activities could not return dicts with non-string keys. E.g. something as simple as this fails:

    @workflow.run
    async def run(self, name: str) -> dict[int, str]:
        return {1: "a"}
TypeError: Failed converting key 1 on dict[int, str]

This is surprising to users because a pervasive feature of the Python SDK is that type annotations are used to make the data converter do the right thing: the type annotation should be "honored" there.

The behavior happens because (1) we default to json/plain encoding, (2) in JSON keys must be strings, (3) the Python JSON encoder accepts str, int, float, bool, None dict keys, silently encoding them as valid JSON string representations, (4) although the type annotation is typically present (either from a type annotation in workflow/activity code) or less commonly because a user supplied the return_type argument) to "fix" the key type, our data converter doesn't do that. But JSON encoding is an implementation detail of the Python SDK.

The SDK's pydantic_data_converter has no problem with dict types like that above. This PR makes our default data converter support them also.

@dandavison dandavison changed the title Data converter non string keys Data converter non-string keys Apr 18, 2025
@dandavison dandavison force-pushed the data-converter-non-string-keys branch 2 times, most recently from 2f7ce62 to c734225 Compare April 18, 2025 17:05
@dandavison dandavison marked this pull request as ready for review April 18, 2025 17:05
@dandavison dandavison requested a review from a team as a code owner April 18, 2025 17:05
@dandavison dandavison force-pushed the data-converter-non-string-keys branch from c734225 to 6f261c0 Compare April 18, 2025 17:11
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

Nothing blocking, just a question (forgot to hit approve on this comment, did just after)

Comment on lines +378 to +381
ok(Dict[int, str], {1: "1"})
ok(Dict[float, str], {1.0: "1"})
ok(Dict[bool, str], {True: "1"})
ok(Dict[None, str], {None: "1"})
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm that before this change, all of these failed? I just want to make sure one of these didn't accidentally work before we made these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I actually confirmed that same thing myself before marking the PR ready for review. They do all fail.

@dandavison dandavison enabled auto-merge (squash) April 18, 2025 18:10
@dandavison dandavison force-pushed the data-converter-non-string-keys branch from 6f261c0 to d423176 Compare April 18, 2025 22:33
@dandavison dandavison merged commit 4933dc5 into temporalio:main Apr 18, 2025
13 checks passed
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.

2 participants