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

Conversation

Oracen
Copy link
Contributor

@Oracen Oracen commented Nov 1, 2022

What was changed

Updated from #174 due to merge conflicts on lockfile. Existing comments over there integrated

The date parsing system in temporalio.converter:decode_search_attributes is switched from datetime.datetime.fromisoformat to dateutils.parser.isoparse. This provides support for ISO8601 compliant datetime strings with timezones attached

Why?

Python's datetime library is not timezone aware by default, with issues on StackOverflow going back as far as 2008. Prior to Python 3.11, datetime.datetime.fromisoformat was not designed to handle timezone-aware datetime strings, but rather the timezone-unaware outputs of datetime.isoformat, which was not ISO compliant! (Hardest problem in SE is naming after all.)

The new Scheduling features in Temporal handle timezone-aware schedules by default. This caused issues when communicating with the Python SDK, as it could not decode the timezone strings (typically appended with 'Z' for UTC time). dateutils.parser.isoparse is known to have some problems with greedy evaluation of date strings but this typically occurs in the presence of user input. It should not be a problem for machine-formatted strings.

While the release of 3.11 means this won't be an issue in future, much of the Python and cloud ecosystem relies on older versions of Python. This change means most current versions of Python can leverage the scheduling functionality.

Checklist

  1. Closes Search attribute Datetime need to support full ISO 8601 (and maybe more) #144

  2. How was this tested:

  • development of replication and fix (see linked issue for replication code)
  • wrote unit tests to verify old/new functionality
  • run against my local development project
  1. Any docs updates needed?
    No

Outstanding issues:

  • Integration test needed on test_workflows, suggested test_workflow_search_attributes

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.

Couple of comments. Will still approve to run on CI

pyproject.toml Outdated Show resolved Hide resolved
temporalio/converter.py Outdated Show resolved Hide resolved
temporalio/converter.py Outdated Show resolved Hide resolved
@Oracen
Copy link
Contributor Author

Oracen commented Nov 2, 2022

That build is failing because we need to install the mypy type stub, I thought it should be doing that automatically? Linting is passing locally (I'm devving in 3.10) but clearly something isn't lining up.

As to the issue of integration testing, I'm not sure if we're able to do it in a smooth way - the value of Z coming back is a server-side issue, and I don't know if its being generated by tctl, by the UI, or quite what. There's no SDK access to the new scheduler for any language for this point in time. I'm going to mock a response in an independent test so we can isolate the integration test from the rest of the search functionality checks

@Oracen
Copy link
Contributor Author

Oracen commented Nov 2, 2022

Integration test done. As for MyPy dependencies, I added them alongside the protobuf type stubs, but again behind python<3.11. Out of interest, the protobuf stubs were listed as full deps, not dev dependencies. Is this intentional?

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.

Thanks! Basically ready, just two minor things: 1) let's back out that mock change and just leave the integration test out (sorry!), and 2) let's remove that types-python-dateutil import and just # type: ignore whatever MyPy is complaining about.

Then we'll be ready to merge (assuming it won't need a master merge from another PR coming earlier, but I may be able to perform that merge myself if it happens). Sorry for the run-around.

(don't mind the flaky CI test failure)

tests/worker/test_workflow.py Outdated Show resolved Hide resolved
pyproject.toml Outdated
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).

@Oracen
Copy link
Contributor Author

Oracen commented Nov 3, 2022 via email

@cretz
Copy link
Member

cretz commented Nov 4, 2022

but not both of them

Your output only showed one "error". If I get some time today I'll check this out and experiment.

@Oracen
Copy link
Contributor Author

Oracen commented Nov 6, 2022

I'm going to chalk that up to the reduced brainpower during post-work coding. Should be G2G

@cretz cretz merged commit a75256e into temporalio:main Nov 7, 2022
@cretz
Copy link
Member

cretz commented Nov 7, 2022

Merged, thanks!

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.

Search attribute Datetime need to support full ISO 8601 (and maybe more)
2 participants