Serialize datetime

[mpharrigan]

spun off from #5152

It's super important when running experiments to keep track of when things happen.

cc @maffoo

[maffoo]

This should not be necessary, because the timestamp() method takes into account the timezone (or the locale timezone for naive datatimes) to convert to a unix epoch timestamp (which itself has no timezone).

In [12]: now, now_utc = datetime.datetime.now(), datetime.datetime.now(tz=datetime.timezone.utc)

In [13]: now
Out[13]: datetime.datetime(2022, 4, 20, 11, 52, 22, 976460)

In [14]: now_utc
Out[14]: datetime.datetime(2022, 4, 20, 18, 52, 22, 976471, tzinfo=datetime.timezone.utc)

In [15]: now.timestamp()
Out[15]: 1650480742.97646

In [16]: now_utc.timestamp()
Out[16]: 1650480742.976471

[mpharrigan]

but if you assert now == now_utc it will fail

[mpharrigan]

so if you had

ts = now().in_my_local_timezone()
assert ts == read_json(to_json(ts))

the assertion would fail, even though you're still referring to the correct point in time.

[maffoo]

While round trip equality is nice, I think datetime is generally broken with how it handles naive vs tz-aware instances and so we shouldn't expect roundtrip equality (even two tz-aware datetime instances with the same timestamp in different timezones do not compare equal). So I suggest we support serializing any datetime by getting its timestamp, and then we just have to pick whether to deserialize as naive or tz-aware datetimes. I think tz-aware with utc is ok, though I would probably prefer naive for simplicity.

[wcourtney]

I thought that the idea of specifying the tz in deserialization was to force comparison by timestamp(). In a quick test, it seemed like I could reasonable compare two tz-aware datetimes. Riffing on mpharrigan@'s example above:

>>> ts = datetime.datetime.now(tz=timezone('US/Pacific'))
>>> ts == datetime.datetime.fromtimestamp(ts.timestamp(), tz=datetime.timezone.utc)
True

Also...

>>> ts = datetime.datetime.now()
>>> ts.timestamp() == datetime.datetime.fromtimestamp(ts.timestamp(), tz=datetime.timezone.utc).timestamp()
True

That all seems reasonable to me, so I'd +1 to allowing any input datetime here.

[mpharrigan]

ah. Thanks for investigating @wcourtney . It looks like equality does work for "aware" datetimes.

[mpharrigan]

( I removed the utc check during serialization)

[maffoo]

I'm conflicted about whether it's better to use naive datetime or explicit utc timezone here. I generally find that using naive datetimes is simpler, as long as you use timestamp() for any kind of comparison, but I could be convinced otherwise.

In [17]: ts = 1650480742

In [18]: dt = datetime.datetime.fromtimestamp(ts)

In [19]: dt_utc = datetime.datetime.fromtimestamp(ts, tz=datetime.timezone.utc)

In [20]: dt
Out[20]: datetime.datetime(2022, 4, 20, 11, 52, 22)

In [21]: dt_utc
Out[21]: datetime.datetime(2022, 4, 20, 18, 52, 22, tzinfo=datetime.timezone.utc)

In [22]: dt.timestamp()
Out[22]: 1650480742.0

In [23]: dt_utc.timestamp()
Out[23]: 1650480742.0

One big problem with the datetime library IMO is that it has a function datetime.datetime.utcnow() which ideally would be the same as datetime.datetime.now(tz=datetime.timezone.utc) but which in fact returns a "naive" datetime with no tz but whose calendar fields (year, month, day, hour, min, second) agree with the current utc time. I find this incredibly confusing.

[mpharrigan]

yes, the docs for utcnow has a red box telling you not to use it.

naive datetimes is simpler, as long as you use timestamp() for any kind of comparison

I figured it might be worth forcing folks to use the explicit thing so they can't accidentally not use timestamp(). I mean: another alternative would be to use timestamp floats instead of datetime.datetime everywhere.

the other problem is that it's really hard to use datetime.__eq__ in a meaningful way with naive datetimes. I guess that falls under "comparison".

[wcourtney]

More explicit than timestamp floats would be timestamp protos, maybe made easier by proto-plus utilities? I personally prefer that, but my hunch is that it's better for the library to support datetime natively.

For usability of datetime, +1 to forcing users to use .timestamp() by setting the explicit tz.

[maffoo]

Naive datetimes compare fine to other naive datetimes, and I think I'd lean toward using them when deserializing for simplicity. But this is definitely an area where footguns abound. We have switched over to using a Timestamp internally that is just a wrapper around unix epoch timestamps to avoid all these issues with datetime.

[maffoo]

More explicit than timestamp floats would be timestamp protos, maybe made easier by proto-plus utilities? I personally prefer that, but my hunch is that it's better for the library to support datetime natively.

For usability of datetime, +1 to forcing users to use .timestamp() by setting the explicit tz.

You can call .timestamp() on any datetime instance, whether it has a tz or not, so I don't think that setting it forces users to do the right thing.

I think timestamp protos are going to feel out of place in most python code and we don't currently pull in protobuf as a dependency of cirq-core, so probably shouldn't rely on them unless we want to support time serialization only in cirq-google.

[mpharrigan]

on the basis that:

1. the proto fields use an analog of utc "aware" datetimes
2. things seem generally less broken if you live entirely in "aware" datetime land

I'd like to go ahead with forcing 'aware' during serialization and always deserializing as "aware" utc.

[maffoo]

Proto uses a timestamp (which can be gotten from naive or aware datetime), so I don't think that carries much weight here. But I'm fine with deserializing as aware and and accepting only aware datetimes (could decide to accept naive datetimes in the future if we want). Thanks for working through all the comments here!

[wcourtney]

Thanks for working through this! I'm a fan of being more explicit, especially WRT time since it's often hard enough to reason about even w/o Python's fun tz handling.

[maffoo]

Minor comment about variable names in tests, otherwise LGTM.

[maffoo]

nit: can we use the suffix _dt instead for datetime instances in these tests? _ts sounds like "timestamp", and I think we should be careful about the distinction between a datetime and a timestamp.

[mpharrigan]

done

[CirqBot]

Automerge cancelled: A required status check is not present.

Missing statuses: ['Coverage check', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']