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.

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

Feature/datetime conversion fix #179

Merged
merged 11 commits into from Nov 7, 2022
51 changes: 39 additions & 12 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pyproject.toml
Expand Up @@ -30,7 +30,9 @@ opentelemetry-api = { version = "^1.11.1", optional = true }
opentelemetry-sdk = { version = "^1.11.1", optional = true }
protobuf = "~4.21"
python = "^3.7"
python-dateutil = { version = "^2.8.2", python = "<3.11" }
types-protobuf = "~3.20"
types-python-dateutil = { version = "^2.8.19.2", python = "<3.11" }
Copy link
Member

Choose a reason for hiding this comment

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

I'd be ok with just # type: ignore on offending lines instead of adding this dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only issue with this is Mypy is complaining on init that its missing stubs, and even --ignore-missing-import misses a single error that leads to an exception:

(temporalio-py3.10) ubuntu@ip-172-31-27-188:~/projects/sdk-python$ mypy --namespace-packages .
temporalio/converter.py:40: error: Library stubs not installed for "dateutil" (or incompatible with Python 3.10)
temporalio/converter.py:40: note: Hint: "python3 -m pip install types-python-dateutil"
temporalio/converter.py:40: note: (or run "mypy --install-types" to install all missing stub packages)
temporalio/converter.py:40: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

We could solve this by changing the mypy command with the following:

mypy --install-types --non-interactive .

or add them to pyproject.toml instead. What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

What happens if you add # type: ignore to the end of line 40 of converter.py (i.e. the end of the import line)? If that still doesn't help, can you try adding this in pyproject.toml right after the [tool.mypy] section:

[[tool.mypy.overrides]]
module = "dateutil"
ignore_missing_imports = true

python/mypy#10582 suggests it may help (which is annoying, we already have that set globally).

typing-extensions = "^4.2.0"

[tool.poetry.dev-dependencies]
Expand Down
22 changes: 21 additions & 1 deletion temporalio/converter.py
Expand Up @@ -7,12 +7,14 @@
import dataclasses
import inspect
import json
import sys
from abc import ABC, abstractmethod
from dataclasses import dataclass
from datetime import datetime
from enum import IntEnum
from typing import (
Any,
Callable,
Dict,
List,
Mapping,
Expand All @@ -33,6 +35,10 @@
import temporalio.api.common.v1
import temporalio.common

if sys.version_info < (3, 11):
# Python's datetime.fromisoformat doesn't support certain formats pre-3.11
from dateutil import parser


class PayloadConverter(ABC):
"""Base payload converter to/from multiple payloads/values."""
Expand Down Expand Up @@ -677,6 +683,19 @@ def encode_search_attribute_values(
return default().payload_converter.to_payloads([safe_vals])[0]


def _get_iso_datetime_parser() -> Callable[[str], datetime]:
"""Isolates system version check and returns relevant datetime passer

Returns:
A callable to parse date strings into datetimes.
"""
if sys.version_info >= (3, 11):
return datetime.fromisoformat # noqa
else:
# Isolate import for py > 3.11, as dependency only installed for < 3.11
return parser.isoparse


def decode_search_attributes(
api: temporalio.api.common.v1.SearchAttributes,
) -> temporalio.common.SearchAttributes:
Expand All @@ -697,7 +716,8 @@ 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]
parser = _get_iso_datetime_parser()
val = [parser(v) for v in val]
ret[k] = val
return ret

Expand Down
39 changes: 38 additions & 1 deletion tests/test_converter.py
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 @@ -164,6 +164,43 @@ def test_encode_search_attribute_values():
temporalio.converter.encode_search_attribute_values(["foo", 123])


def test_decode_search_attributes():
"""Tests decode from protobuf for python types"""

def payload(key, dtype, data, encoding=None):
if encoding is None:
encoding = {"encoding": b"json/plain"}
check = temporalio.api.common.v1.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)

# Check timezone aware, hour offset
dttz_check = temporalio.converter.decode_search_attributes(
payload("dt", "Datetime", '"2020-01-01T00:00:00+00:00"')
)
assert dttz_check["dt"][0] == datetime(2020, 1, 1, 0, 0, 0, tzinfo=timezone.utc)


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

Expand Down
57 changes: 39 additions & 18 deletions tests/worker/test_workflow.py
Expand Up @@ -22,6 +22,7 @@
Tuple,
cast,
)
from unittest import mock

import pytest
from google.protobuf.timestamp_pb2 import Timestamp
Expand Down Expand Up @@ -1344,6 +1345,20 @@ def do_search_attribute_update(self) -> None:
)


def mock_modify_value_datetime_z(inputs: bytes) -> WorkflowActivation:
"""Adds a check in case the server sends a 'Z' as part of its UTC format"""
job_act = WorkflowActivation.FromString(inputs)
for job in job_act.jobs:
if not job.HasField("start_workflow"):
continue
for item in job.start_workflow.search_attributes.indexed_fields.values():
if item.metadata.get("type") != b"Datetime":
# Only looking for dates
continue
item.data = item.data.replace(b"+00:00", b"Z")
return job_act


async def test_workflow_search_attributes(client: Client, env_type: str):
if env_type != "local":
pytest.skip("Only testing search attributes on local which disables cache")
Expand Down Expand Up @@ -1372,24 +1387,30 @@ async def search_attributes_present() -> bool:
assert await search_attributes_present()

async with new_worker(client, SearchAttributeWorkflow) as worker:
handle = await client.start_workflow(
SearchAttributeWorkflow.run,
id=f"workflow-{uuid.uuid4()}",
task_queue=worker.task_queue,
search_attributes={
f"{sa_prefix}text": ["text1", "text2", "text0"],
f"{sa_prefix}keyword": ["keyword1"],
f"{sa_prefix}int": [123],
f"{sa_prefix}double": [456.78],
f"{sa_prefix}bool": [True],
f"{sa_prefix}datetime": [
# With UTC
datetime(2001, 2, 3, 4, 5, 6, tzinfo=timezone.utc),
# With other offset
datetime(2002, 3, 4, 5, 6, 7, tzinfo=timezone(timedelta(hours=8))),
],
},
)
with mock.patch(
Oracen marked this conversation as resolved.
Show resolved Hide resolved
"temporalio.bridge.proto.workflow_activation.WorkflowActivation.FromString",
side_effect=mock_modify_value_datetime_z,
):
handle = await client.start_workflow(
SearchAttributeWorkflow.run,
id=f"workflow-{uuid.uuid4()}",
task_queue=worker.task_queue,
search_attributes={
f"{sa_prefix}text": ["text1", "text2", "text0"],
f"{sa_prefix}keyword": ["keyword1"],
f"{sa_prefix}int": [123],
f"{sa_prefix}double": [456.78],
f"{sa_prefix}bool": [True],
f"{sa_prefix}datetime": [
# With UTC
datetime(2001, 2, 3, 4, 5, 6, tzinfo=timezone.utc),
# With other offset
datetime(
2002, 3, 4, 5, 6, 7, tzinfo=timezone(timedelta(hours=8))
),
],
},
)
# Make sure it started with the right attributes
expected = {
f"{sa_prefix}text": {"type": "str", "values": ["text1", "text2", "text0"]},
Expand Down