Adjust usage of `utcnow()` and `utcfromtimestamp()` for Python 3 by amotl · Pull Request #440 · crate/crate-python

amotl · 2022-07-21T23:13:50Z

Motivation

This patch further sets the stage for #437.

About

On the transition from Python 2 to Python 3, this subtle adjustment sometimes slips through. It is needed for properly working with the now() variants of native Python date and datetime objects.

Instead of datetime.utcnow(), use datetime.now(timezone.utc)
Instead of utcfromtimestamp(), use fromtimestamp(..., tz=timezone.utc)

See also: https://blog.ganssle.io/articles/2019/11/utcnow.html

Please note that this patch still reflects the fact that only timezone-unaware (naive) datetime objects are handled yet. In order to compensate that, a number of .replace(tzinfo=None) calls have been added along the lines.

On the transition from Python 2 to Python 3, this subtle adjustment needed when working with date and datetime objects sometimes slips through. - Instead of `datetime.utcnow()`, use `datetime.now(timezone.utc)` - Instead of `utcfromtimestamp()`, use `fromtimestamp(..., tz=timezone.utc)` See also: https://blog.ganssle.io/articles/2019/11/utcnow.html Please note that this patch still reflects the fact that only timezone-unaware (naive) datetime objects are handled yet. In order to compensate that, a number of `.replace(tzinfo=None)` calls have been added along the lines.

matriv

Thx @amotl left a couple of comments.

matriv · 2022-07-22T09:06:16Z

 )
 from .sqlalchemy.tests import test_suite as sqlalchemy_test_suite
 from .sqlalchemy.types import ObjectArray
+from ..testing.util import datetime_now_utc_naive, date_now_utc_naive


why this is relative?

PyCharm created that import line for me. The import statements above are also relative.

matriv · 2022-07-22T09:06:32Z

+from datetime import datetime, timezone
+
+
+def datetime_now_utc_naive():


why did you choose naive in the naming?

The documentation where this patch is based upon also uses the same nomenclature "aware" vs. "naive" for refering to "timezone-aware" vs. "timezone-unaware" native Python datetime objects ¹².

Date and time objects may be categorized as “aware” or “naive” depending on whether or not they include timezone information.

Footnotes

https://docs.python.org/3/library/datetime.html ↩

https://blog.ganssle.io/articles/2019/11/utcnow.html ↩

mfussenegger · 2022-07-22T11:57:29Z

-    >>> from datetime import datetime
-    >>> now = datetime.utcnow()
+    >>> from datetime import datetime, timezone
+    >>> now = datetime.now(timezone.utc).replace(tzinfo=None)


I don't understand the change here. Isn't now(timezone.utc).replace(tzinfo=None) the same as utcnow()?

It is really sad. In situations when the system time zone setting, defined through the TZ environment variable, is expressed in UTC, everything is fine. However, it is not the case if the system time zone is anything other than UTC.

With c1f183e, I've tried to improve the situation to outline the problem, on behalf of some test cases. 5894c2e quotes and references all important information about this topic, from both the Python documentation and articles by other capacities.

This is not the case.

utcnow:

Return the current UTC date and time, with tzinfo None.

Yes, it is a naive datetime without timezone info attached, but it is using utc.

>>> print(datetime.utcnow(), '\n', datetime.now(tz=timezone.utc).replace(tzinfo=None), '\n', datetime.now()) 2022-07-22 18:27:27.055192 2022-07-22 18:27:27.055200 2022-07-22 20:27:27.055213

↪ date +"%Z %z" CEST +0200

Also as the articles point out, the problem is the use of the naive datetime (lack of tzinfo) in subsequent operations. If we remove the tzinfo anyways via .replace(tzinfo=None) we don't gain anything by using now(tz=timezone.utc) but only make things more complicated.

And I'm not sure if attaching a timezone is even the correct thing to do, because the timestamp stored in CrateDB also doesn't have one attached. Assuming that timestamp values returned by the server are UTC may not be safe.

Thank you. You convinced me that this goes too far. I've submitted #441 instead, which solely focuses on mitigating a fluke I ~~regularly~~ occasionally observed when running the test suite on CI after midnight.

mfussenegger · 2022-07-22T11:58:31Z

            try:
-                return datetime.utcfromtimestamp(value / 1e3).date()
+                return datetime.fromtimestamp(value / 1e3, tz=timezone.utc) \
+                    .replace(tzinfo=None).date()


If we throw away the tzinfo why not keep using utcfromtimestamp?

I think the idea is to be a preparation step to actually allow the user to pass a tzinfo here, later on?

mfussenegger · 2022-07-22T11:59:08Z

+
+
+def datetime_now_utc_naive():
+    return datetime.now(timezone.utc).replace(tzinfo=None)


Isn't this just utcnow?

I far from expert in python but I also read this: https://blog.ganssle.io/articles/2019/11/utcnow.html

Those test cases try to outline the situation why 5a94e36 is needed. They specifically exercise some scenarios where the system time zone is expressed in UTC, or not, and shows the corresponding deviances. Other test cases attempt to outline that the new helper functions `datetime_now_utc_naive` and `date_now_utc_naive` do the right thing in all situations, regardless of any corresponding system locale or time zone setting.

amotl marked this pull request as ready for review July 21, 2022 23:35

amotl requested review from matriv and mfussenegger July 21, 2022 23:35

matriv reviewed Jul 22, 2022

View reviewed changes

amotl requested a review from matriv July 22, 2022 09:56

mfussenegger requested changes Jul 22, 2022

View reviewed changes

amotl added 3 commits July 22, 2022 20:12

Add changelog item for adjustment of utcnow() and utcfromtimestamp()

70af9c0

CI: Don't fail fast to get a broader picture

10a2c7b

amotl force-pushed the amo/remove-datetime-utcnow branch from 5894c2e to 10a2c7b Compare July 22, 2022 18:13

Fix? Try to clear all environment variables.

302984c

amotl mentioned this pull request Jul 22, 2022

Tests: Use UTC-based utcnow().date() instead of localtime-based date.today() #441

Merged

amotl closed this Jul 22, 2022

amotl deleted the amo/remove-datetime-utcnow branch July 22, 2022 23:22

		from datetime import datetime, timezone


		def datetime_now_utc_naive():



		def datetime_now_utc_naive():
		return datetime.now(timezone.utc).replace(tzinfo=None)

Conversation

amotl commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

About

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amotl Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amotl Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfussenegger Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amotl commented Jul 21, 2022 •

edited

Loading

amotl Jul 22, 2022 •

edited

Loading

amotl Jul 22, 2022 •

edited

Loading

mfussenegger Jul 22, 2022 •

edited

Loading