temporalio / sdk-python

Temporal Python SDK
MIT License
441 stars 66 forks source link

[Bug] Upserting datetime search attribute fails #572

Closed accupham closed 2 days ago

accupham commented 1 month ago

What are you really trying to do?

I'm trying to update a workflow with a search attribute checkout_time of type datetime using the python SDK. The checkout_time datetime search attribute exists on that particular namespace.

Describe the bug

from datetime import datetime
from temporalio import workflow
from temporalio.common import SearchAttributeKey, SearchAttributeValue

t = datetime.fromisoformat("2024-07-05T15:43:07.875302" ) # <- yes, workflow is deterministic
k = SearchAttributeKey.for_datetime("checkout_time")
u = k.value_set(t)
workflow.upsert_search_attributes([u])

Results in this worker error:

2024-07-05T20:46:30.554682Z WARN temporal_sdk_core::worker::workflow: Error while completing workflow activation error=status: InvalidArgument, message: "BadSearchAttributes: invalid value for search attribute checkout of type Datetime: 2024-07-05T15:43:07.875302", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc", "server": "temporal", "date": "Fri, 05 Jul 2024 15:43:07 GMT"} }

Environment/Versions

cretz commented 1 month ago

:+1: Unfortunately not all ISO-8601 timestamps are accepted. The Temporal server uses the Go RFC3339Nano format which has some incompatibilities. In this case, the time is invalid, see https://go.dev/play/p/3SzyCA_UlUK. May be able to add a Z at the end of the string for it to parse, we will test. https://docs.temporal.io/visibility kinda touches on the format expected for date time for the built-in attributes (we will update the docs to more clearly state the date time format in general).

Thanks for the report. We will look into fixing the bug for this timestamp and format string.

accupham commented 1 month ago

Thank you for taking a look at it.

I also noticed the python examples in the docs for SearchAttribute upserting are using the old style without types. Might be a good idea to update those to use the new style too once changes are stable.

https://docs.temporal.io/develop/python/observability#search-attributes

cretz commented 1 month ago

Yes, we have issues opened on the documentation side to update to typed search attributes

accupham commented 1 week ago

Any updates on this?

cretz commented 1 week ago

This is considered a high priority and we are actively working on it, but no updates as of yet. It very likely may just be adding a Z to the end of the string internally, we are investigating.

dandavison commented 2 days ago

Hi @accupham, the issue here is that the SDK requires you to specify a timezone -- our server needs to store user timestamps with a timezone and the SDK can't assume that your timestamp refers to time in the location of the machine running the worker process.

dandavison commented 2 days ago

I've opened https://github.com/temporalio/sdk-python/issues/611 to have the SDK raise an exception with a more helpful error message -- thanks for the report!