Fix datetime deserialization by replacing timezone short code by its name in serialized values by hussein-awala · Pull Request #34492 · apache/airflow

hussein-awala · 2023-09-19T21:49:31Z

…name in serialized values

Taragolis · 2023-09-20T00:09:26Z

 o = convert_to_utc(o)

- tz = o.tzname()
+ tz = o.tzinfo.name # type: ignore


Dumb solution which should work for different types of tzinfo
Suggested change
tz=o.tzinfo.name# type: ignore
frompendulum.tz.timezoneimportFixedTimezone, Timezone
tzi=o.tzinfo
ifisinstance(tzi, FixedTimezone):
tz=tzi.offsetor"UTC"
elifisinstance(tzi, Timezone):
tz=tzi.name
else:
tz=int(o.utcoffset().total_seconds()) or"UTC"

And just for a record, I don't think that we should use local imports for datetime and pendulum in this module because we import airflow.utils.timezone which import entire datetime and some modules from pendulum (at least Timezone/Datetime)

bolkedebruin · 2023-09-20T13:27:13Z

We probably should re-use the code in the timezone serializer (timezone.py) which does the right thing. It's close to @Taragolis implementation, but I would prefer to rely on one and not two.

bolkedebruin · 2023-09-20T13:53:50Z

My suggestion:

Use serialize from timezone.py to serialize the timezone information
Increment the version
If the older version use the old_style deserialization AND - to be nice on people experiencing issues now, provide a mapping that does if needed (i.e. having a tz name is in these)

Eastern = USTimeZone(-5, "Eastern", "EST", "EDT")
Central = USTimeZone(-6, "Central", "CST", "CDT")
Mountain = USTimeZone(-7, "Mountain", "MST", "MDT")
Pacific = USTimeZone(-8, "Pacific", "PST", "PDT")

(source: https://github.com/stub42/pytz/blob/1acdc7f5ab3f4af063ddcd435f79a48a4a8ce079/src/pytz/reference.py#L137)

bolkedebruin · 2023-09-20T13:54:08Z

are you up for that @hussein-awala ?

Taragolis · 2023-09-20T14:22:38Z

Initially I thought that it would be also a good idea to use reuse serializer however there is one small but important thing.

When you have timezone (subclass of tzinfo) then we have a limited number of opportunity to extract exact timezone because this is depend on implementation, e.g. classes from pendulum.tz.timezone could store either in IANA timezone (Timezone ) or offset from utc (FixedTimezone) and it's pretty logical that we only support Pendulum implementation.

However in case if we work with ~~datetime-naive~~ datetime-aware (updated 🤦) and we don't know how to extract name (deatetime.timezone, zoneinfo, pytz and etc), we could always calculate exact offset to UTC by call utcoffset() method and this value could be use in deserialisation to Pendulum Timezone

So if we would like to create generic timezone extraction we could create this method in airflow.utils.timezone and extract depend of the input type tzinfo or datetime.datetime

bolkedebruin · 2023-09-20T14:49:20Z

Initially I thought that it would be also a good idea to use reuse serializer however there is one small but important thing.
When you have timezone (subclass of tzinfo) then we have a limited number of opportunity to extract exact timezone because this is depend on implementation, e.g. classes from pendulum.tz.timezone could store either in IANA timezone (Timezone ) or offset from utc (FixedTimezone) and it's pretty logical that we only support Pendulum implementation.
However in case if we work with datetime-naive and we don't know how to extract name (deatetime.timezone, zoneinfo, pytz and etc), we could always calculate exact offset to UTC by call utcoffset() method and this value could be use in deserialisation to Pendulum Timezone
So if we would like to create generic timezone extraction we could create this method in airflow.utils.timezone and extract depend of the input type tzinfo or datetime.datetime

I'm not sure if I follow. We already convert naive dates to UTC (pendulum) datetimes. And we explicitly do not support other libraries than Pendulum since the integration of timezones into Airflow. So I don't think your second case makes sense?

Refactoring into airflow.utils.timezone does make sense. We would end up with a wrapper that basically does the same as FixedTimezone and Timezone

Taragolis · 2023-09-20T15:26:49Z

@bolkedebruin Sorry for confuse. I meant datetime-aware, but for unknown for me reason write down naive 🤦 🤦

bolkedebruin · 2023-09-20T16:41:56Z

no worries! maybe we should see a first iteration and then figure out if we want to support non-pendulum timezone aware datetimes. I do not think we should, due to all the issues with the other implementations, but open to discussion.

hussein-awala · 2023-09-20T18:12:03Z

are you up for that @hussein-awala ?

Yes, I will try to implement what you both suggested and add some unit tests

Taragolis · 2023-09-20T19:21:07Z

maybe we should see a first iteration and then figure out if we want to support non-pendulum timezone aware datetimes. I do not think we should, due to all the issues with the other implementations, but open to discussion.

I'm just worry, if we only would serialize datetime.datetime and pendulum.DataTime with pendulum Timezone, then we potentially could breaks someone pipeline, because right now it is possible without any issues serialize datetime with UTC timezone regardless of tzinfo implementation. And different libraries use different tzinfo for timezones, for example

boto3: python-dateutils
psycopg2: own implementation
psycopg (formally v3): zoneinfo.ZoneInfo and backports.zoneinfo.ZoneInfo(Python < 3.9)

In general you could compare datetime.datetime with different tzinfo implementation

fromdateutil.tzimporttzutcfromdatetimeimportdatetime, timezone, timedeltafrompendulum.tz.timezoneimportTimezonefromzoneinfoimportZoneInfofrompendulum.tzimporttimezoneasptimezonefrompsycopg2.tzimportFixedOffsetTimezonefrompytzimportUTCd1=datetime(2021, 1, 1, tzinfo=timezone.utc)
d2=datetime(2021, 1, 1, tzinfo=Timezone("UTC"))
d3=datetime(2021, 1, 1, tzinfo=tzutc())
d4=datetime(2021, 1, 1, tzinfo=ZoneInfo("UTC"))
d5=datetime(2021, 1, 1, tzinfo=FixedOffsetTimezone(offset=0))
d6=datetime(2021, 1, 1, tzinfo=UTC)
assertd1==d2==d3==d4==d5==d6assertd1.tzname() ==d2.tzname() ==d3.tzname() ==d4.tzname() ==d6.tzname()
assertd5.tzname() =="+00"# psycopg2 uses Python 2.3 implementation ;-)assertd1.utcoffset() ==d2.utcoffset()
assertd1.utcoffset() ==d3.utcoffset()
assertd1.utcoffset() ==d4.utcoffset()
assertd1.utcoffset() ==d5.utcoffset()
assertd1.utcoffset() ==d6.utcoffset()
dl1=datetime(2021, 6, 1, tzinfo=Timezone("Europe/London"))
dl2=datetime(2021, 6, 1, tzinfo=ZoneInfo("Europe/London"))
dl3=datetime(2021, 6, 1, tzinfo=timezone(timedelta(hours=1)))
assertdl1==dl2==dl3dl1_from_name=datetime(2021, 6, 1, tzinfo=ptimezone(dl1.tzinfo.name))
dl1_from_offset=datetime(2021, 6, 1, tzinfo=ptimezone(int(dl1.utcoffset().total_seconds())))
assertdl1==dl1_from_name==dl1_from_offsetdl2_from_name=datetime(2021, 6, 1, tzinfo=ptimezone(dl2.tzinfo.key))
dl2_from_offset=datetime(2021, 6, 1, tzinfo=ptimezone(int(dl2.utcoffset().total_seconds())))
assertdl2==dl2_from_name==dl2_from_offsetdl3_from_offset=datetime(2021, 6, 1, tzinfo=ptimezone(int(dl3.utcoffset().total_seconds())))
assertdl3==dl3_from_offset

Maybe we should try to serialize datetime.datetime but result of deserialization will always use Pendulum timezone regardless original one.

In case of serialize just timezone I think it is fine to serialize only Pendulum one, and when we have min Python 3.9 we could discuss should we also serialize zoneinfo.ZoneInfo

hussein-awala · 2023-09-20T21:31:39Z

+ s["__data__"]["tz"] = "EDT"
+ d = deserialize(s)
+ assert d.timestamp() == 1657505443.0
+ assert d.tzinfo.name == "-04:00"
+ # assert that it's serializable with the new format
+ assert deserialize(serialize(d)) == d


This test ensures that the current version of the datetime Serializer fixes the bug by deserialize the US unsupported timezones, and that the deserialized values are serializable with the new format (if the user read an xcom serialized in version 1 and return it or send it to a new XCom)

hussein-awala · 2023-09-20T21:34:35Z

@Taragolis for your last comment; based on this:

We already convert naive dates to UTC (pendulum) datetimes. And we explicitly do not support other libraries than Pendulum since the integration of timezones into Airflow.

I wonder if we should support these classes or not, and if yes, should we include it in this bug fix or add it in a separate PR as a new feature.

Taragolis · 2023-09-20T22:20:50Z

I don’t think we should support all this classes:

We don’t know exact numbers of this classes
Not all of them direct Airflow dependency
Some of them might be deprecated
I think it could be a problem if we add ZoneInfo now and user serialize it in python 3.8 (backompat package) and after will try to deserialize in python 3.9+

Better what we could do it serialize offset in seconds and after it could be deserialized as Pendulum Timezone.

Taragolis · 2023-09-20T22:25:08Z

But we only could retrieve offset from datetime object.

Taragolis · 2023-09-20T22:33:45Z

And in general I think we should fix current issue only. And after we could discuss should we make further changes or not.

bolkedebruin

Seems pretty good already! Some questions / nits.

bolkedebruin · 2023-09-25T15:11:17Z

If you can adjust the nit, I'm okay with merging @hussein-awala

hussein-awala · 2023-09-25T20:09:27Z

If you can adjust the nit, I'm okay with merging @hussein-awala

I removed pytz, should I remove the deserialization for the unsupported timezones or keep them?

Change naming to properly reflect the mapping

ferruzzi · 2023-09-28T16:57:12Z

The DynamoDB to S3 system test has been failing with an "invalid timezone" exception since this got merged. I'm looking into it.


ERROR airflow.executors.debug_executor.DebugExecutor:debug_executor.py:92 Failed to execute task: Invalid timezone.
--
Traceback (most recent call last):
File "/opt/airflow/airflow/executors/debug_executor.py", line 86, in _run_task
ti.run(job_id=ti.job_id, **params)
File "/opt/airflow/airflow/utils/session.py", line 79, in wrapper
return func(*args, session=session, **kwargs)
File "/opt/airflow/airflow/models/taskinstance.py", line 2507, in run
self._run_raw_task(
File "/opt/airflow/airflow/utils/session.py", line 76, in wrapper
return func(*args, **kwargs)
File "/opt/airflow/airflow/models/taskinstance.py", line 2246, in _run_raw_task
self._execute_task_with_callbacks(context, test_mode, session=session)
File "/opt/airflow/airflow/models/taskinstance.py", line 2375, in _execute_task_with_callbacks
task_orig = self.render_templates(context=context)
File "/opt/airflow/airflow/models/taskinstance.py", line 2787, in render_templates
original_task.render_template_fields(context)
File "/opt/airflow/airflow/models/baseoperator.py", line 1248, in render_template_fields
self._do_render_template_fields(self, self.template_fields, context, jinja_env, set())
File "/opt/airflow/airflow/utils/session.py", line 79, in wrapper
return func(*args, session=session, **kwargs)
File "/opt/airflow/airflow/models/abstractoperator.py", line 699, in _do_render_template_fields
rendered_content = self.render_template(
File "/opt/airflow/airflow/template/templater.py", line 157, in render_template
return value.resolve(context)
File "/opt/airflow/airflow/utils/session.py", line 79, in wrapper
return func(*args, session=session, **kwargs)
File "/opt/airflow/airflow/models/xcom_arg.py", line 417, in resolve
result = ti.xcom_pull(
File "/opt/airflow/airflow/utils/session.py", line 76, in wrapper
return func(*args, **kwargs)
File "/opt/airflow/airflow/models/taskinstance.py", line 2967, in xcom_pull
return XCom.deserialize_value(first)
File "/opt/airflow/airflow/models/xcom.py", line 696, in deserialize_value
return BaseXCom._deserialize_value(result, False)
File "/opt/airflow/airflow/models/xcom.py", line 689, in _deserialize_value
return json.loads(result.value.decode("UTF-8"), cls=XComDecoder, object_hook=object_hook)
File "/usr/local/lib/python3.8/json/__init__.py", line 370, in loads
return cls(**kw).decode(s)
File "/usr/local/lib/python3.8/json/decoder.py", line 337, in decode
obj, end = self.raw_decode(s, idx=_w(s, 0).end())
File "/usr/local/lib/python3.8/json/decoder.py", line 353, in raw_decode
obj, end = self.scan_once(s, idx)
File "/opt/airflow/airflow/utils/json.py", line 117, in object_hook
return deserialize(dct)
File "/opt/airflow/airflow/serialization/serde.py", line 255, in deserialize
return _deserializers[classname].deserialize(classname, version, deserialize(value))
File "/opt/airflow/airflow/serialization/serializers/datetime.py", line 86, in deserialize
tz = deserialize_timezone(data[TIMEZONE][1], data[TIMEZONE][2], data[TIMEZONE][0])
File "/opt/airflow/airflow/serialization/serializers/timezone.py", line 72, in deserialize
return timezone(data)
File "/usr/local/lib/python3.8/site-packages/pendulum/tz/__init__.py", line 37, in timezone
tz = _Timezone(name, extended=extended)
File "/usr/local/lib/python3.8/site-packages/pendulum/tz/timezone.py", line 40, in __init__
tz = read(name, extend=extended)
File "/usr/local/lib/python3.8/site-packages/pendulum/tz/zoneinfo/__init__.py", line 9, in read
return Reader(extend=extend).read_for(name)
File "/usr/local/lib/python3.8/site-packages/pendulum/tz/zoneinfo/reader.py", line 50, in read_for
file_path = pytzdata.tz_path(timezone)
File "/usr/local/lib/python3.8/site-packages/pytzdata/__init__.py", line 63, in tz_path
raise ValueError('Invalid timezone')
ValueError: Invalid timezone

Taragolis · 2023-09-28T16:59:33Z

I think that happen because boto3 / botocore uses dateutil tzinfo implementation, see: #34492 (comment)

ferruzzi · 2023-09-28T17:41:05Z

Looks like maybe we just need to tweak how DynamoDBToS3Operator handles the export_time argument to fix the issue I am seeing, but heads up that other things may have been broken.

ERROR airflow.task.operators:abstractoperator.py:707 Exception rendering Jinja template for task 'backup_db_to_point_in_time', field 'export_time'. Template: XComArg(<Task(_PythonDecoratedOperator): get_export_time>)

tzname() does not return full timezones and returned short hand notations are not deterministic. This changes the serialization to be deterministic and adds some logic to deal with serialized short-hand US Timezones and CEST. --------- Co-authored-by: bolkedebruin <bolkedebruin@users.noreply.github.com> (cherry picked from commit a3c06c0)

Fix datetime deserialization by replacing timezone short code by its …
b684334
…name in serialized values

boring-cyborgBot added the area:serialization label Sep 19, 2023

hussein-awala mentioned this pull request Sep 19, 2023
pendulum.DateTime objects now being serialized as python objects with non-standard timezones #34483
Closed
2 tasks

Taragolis reviewed Sep 19, 2023
View reviewed changes

Comment threadairflow/serialization/serializers/datetime.py Outdated

Taragolis reviewed Sep 20, 2023
View reviewed changes

Use timezone serializer to serialize datetime tz
1bcb521

hussein-awala force-pushed the fix_date_deserialization branch from b4b1ee0 to 1bcb521Compare September 20, 2023 21:27

hussein-awala commented Sep 20, 2023
View reviewed changes

hussein-awala marked this pull request as ready for review September 20, 2023 21:51

hussein-awala requested review from ashb, bolkedebruin and kaxil as code owners September 20, 2023 21:51

bolkedebruin requested changes Sep 21, 2023
View reviewed changes

Comment threadairflow/serialization/serializers/datetime.py Outdated
Comment threadairflow/serialization/serializers/datetime.py Outdated

imporve import
dd7a88e

remove pytz
c6fcde7

uranusjr reviewed Sep 26, 2023
View reviewed changes

Comment threadairflow/serialization/serializers/datetime.py

Update datetime.py
17b7bd5
Change naming to properly reflect the mapping

eladkal added this to the Airflow 2.7.2 milestone Sep 28, 2023

eladkal added the type:bug-fix Changelog: Bug Fixes label Sep 28, 2023

bolkedebruin approved these changes Sep 28, 2023
View reviewed changes

bolkedebruin merged commit a3c06c0 into apache:mainSep 28, 2023

ferruzzi mentioned this pull request Sep 28, 2023
Recent changes to timezone support broke some operators #34673
Closed
2 tasks

ephraimbuddy mentioned this pull request Oct 9, 2023
Status of testing of Apache Airflow 2.7.2rc1 #34830
Closed
99 tasks

-tz=o.tzinfo.name# type: ignore
+frompendulum.tz.timezoneimportFixedTimezone, Timezone
+tzi=o.tzinfo
+ifisinstance(tzi, FixedTimezone):
+tz=tzi.offsetor"UTC"
+elifisinstance(tzi, Timezone):
+tz=tzi.name
+else:
+tz=int(o.utcoffset().total_seconds()) or"UTC"

Uh oh!

Conversation

hussein-awala commented Sep 19, 2023

Uh oh!

Uh oh!

TaragolisSep 20, 2023

Choose a reason for hiding this comment

Uh oh!

TaragolisSep 20, 2023

Choose a reason for hiding this comment

Uh oh!

bolkedebruin commented Sep 20, 2023

Uh oh!

bolkedebruin commented Sep 20, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bolkedebruin commented Sep 20, 2023

Uh oh!

Taragolis commented Sep 20, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bolkedebruin commented Sep 20, 2023

Uh oh!

Taragolis commented Sep 20, 2023

Uh oh!

bolkedebruin commented Sep 20, 2023

Uh oh!

hussein-awala commented Sep 20, 2023

Uh oh!

Taragolis commented Sep 20, 2023

Uh oh!

hussein-awalaSep 20, 2023

Choose a reason for hiding this comment

Uh oh!

hussein-awala commented Sep 20, 2023

Uh oh!

Taragolis commented Sep 20, 2023

Uh oh!

Taragolis commented Sep 20, 2023

Uh oh!

Taragolis commented Sep 20, 2023

Uh oh!

bolkedebruin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bolkedebruin commented Sep 25, 2023

Uh oh!

hussein-awala commented Sep 25, 2023

Uh oh!

Uh oh!

ferruzzi commented Sep 28, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Taragolis commented Sep 28, 2023

Uh oh!

ferruzzi commented Sep 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bolkedebruin commented Sep 20, 2023•
edited
Loading

Taragolis commented Sep 20, 2023•
edited
Loading

ferruzzi commented Sep 28, 2023•
edited
Loading