Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature/datetime conversions #174

Closed
wants to merge 4 commits into from
Closed

Feature/datetime conversions #174

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
748f0ff
Set up unit tests around decoder
Oracen Oct 30, 2022
680ac92
Switch parser to one spec'd for ISO8601
Oracen Oct 30, 2022
954da1d
Add dependency
Oracen Nov 1, 2022
08d47e4
Realphabetise
Oracen Nov 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion temporalio/converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import google.protobuf.json_format
import google.protobuf.message
import google.protobuf.symbol_database
from dateutil import parser
Copy link
Member

Choose a reason for hiding this comment

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

This is a new dependency just for this feature. Is there any way we can avoid it? Is there any way with the standard library to do this, and/or is the parser easy to vendor/copy into here?

(also, a new dependency would need to be added to project.toml which you can do via poetry add dateutil and then re-alphabetizing)

Copy link
Member

@cretz cretz Oct 31, 2022 •
edited

Choose a reason for hiding this comment

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

Looks like it may be unavoidable to add this dependency unfortunately. Can't easily vendor stdlib impl as it's in C (ref python/cpython#92177). But can we put a if if sys.version_info >= (3, 11): here to use the stdlib version? (also don't forget to add that dependency and to run poe format).

Also, this will need to land after (and merge main after) #172 is merged which will add Python 3.11 support.

Copy link
Contributor Author

@Oracen Oracen Oct 31, 2022 •
edited

Choose a reason for hiding this comment

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

Not an issue on my side, I must have been working on a tainted env because I thought dateutil was in standard. Will rebuild and double-check.

For those wondering about why this is an issue:
https://stackoverflow.com/questions/127803/how-do-i-parse-an-iso-8601-formatted-date
https://stackoverflow.com/questions/969285/how-do-i-translate-an-iso-8601-datetime-string-into-a-python-datetime-object

On doing it within the standard library, we could hack around with the following:

# Example lookup for Z
map_datetime = {
    "Z": "+00:00"
}

# ...
# v = "2020-01-01T00:00:00Z"

if v[-1].isalpha() and len(v) == 20:
    v = v[:-1] + map_datetime[v[-1].upper()]

# Code as normal

There's a slight catch here that "J" = local time, so we'd have to decide if we treat that string as timezone unaware or pull the timezone from the local machine.

The only reason I avoided this was to rely on a battle-tested date parser that could potentially catch a wider range of formats coming in from other languages. If you're confident we'll only see letters coming in (outside of the +/- hours format) then I'm happy to switch over to an explicit remap of the last digits for the sake of avoiding the external dependency

Copy link
Member

Choose a reason for hiding this comment

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

I think dateutil is probably an acceptable solution since it doesn't seem to carry dependencies on its own or move much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that, will add in the version check and wait for the 3.11 updates you guys have planned

from typing_extensions import Literal

import temporalio.api.common.v1
Expand Down Expand Up @@ -697,7 +698,7 @@ def decode_search_attributes(
val = [val]
# Convert each item to datetime if necessary
if v.metadata.get("type") == b"Datetime":
val = [datetime.fromisoformat(v) for v in val]
val = [parser.isoparse(v) for v in val]
ret[k] = val
return ret

Expand Down
34 changes: 33 additions & 1 deletion tests/test_converter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import sys
from collections import deque
from dataclasses import dataclass
from datetime import datetime
from datetime import datetime, timezone
from enum import Enum, IntEnum
from typing import (
Any,
Expand Down Expand Up @@ -32,6 +32,7 @@
import temporalio.common
import temporalio.converter
from temporalio.api.common.v1 import Payload as AnotherNameForPayload
from temporalio.api.common.v1.message_pb2 import Payload


class NonSerializableClass:
Expand Down Expand Up @@ -164,6 +165,37 @@ def test_encode_search_attribute_values():
temporalio.converter.encode_search_attribute_values(["foo", 123])


def test_decode_search_attributes():
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to also add an integration test that sets a datetime attribute in a workflow? Or maybe alter test_workflow_search_attributes so that it will break without your fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leave it with me

"""Tests decode from protobuf for python types"""

def payload(key, dtype, data, encoding=None):
if encoding is None:
encoding = {"encoding": b"json/plain"}
check = Payload(
data=bytes(data, encoding="utf-8"),
metadata={"type": bytes(dtype, encoding="utf-8"), **encoding},
)
return temporalio.api.common.v1.SearchAttributes(indexed_fields={key: check})

# Check basic keyword parsing works
kw_check = temporalio.converter.decode_search_attributes(
payload("kw", "Keyword", '"test-id"')
)
assert kw_check["kw"][0] == "test-id"

# Ensure original DT functionality works
dt_check = temporalio.converter.decode_search_attributes(
payload("dt", "Datetime", '"2020-01-01T00:00:00"')
)
assert dt_check["dt"][0] == datetime(2020, 1, 1, 0, 0, 0)

# Check timezone aware works as server is using ISO 8601
dttz_check = temporalio.converter.decode_search_attributes(
payload("dt", "Datetime", '"2020-01-01T00:00:00Z"')
)
assert dttz_check["dt"][0] == datetime(2020, 1, 1, 0, 0, 0, tzinfo=timezone.utc)


NewIntType = NewType("NewIntType", int)
MyDataClassAlias = MyDataClass

Expand Down