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

backport TZ fixes from downstream #209

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ChanceNCounter
Copy link
Contributor

This attempts to fix #203 using portions of HelloChatterbox#32. All tests pass on my computer, but they were already doing that. Paging @forslund.

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Aug 16, 2021
@ChanceNCounter ChanceNCounter marked this pull request as ready for review August 16, 2021 16:55
Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Chance, are naive datetimes being improperly handled, or is there disagreement that we should default to UTC?

Where do we think the majority of naive datetime objects would be coming from? If we assume it to be calls to datetime.datetime.now() then perhaps we should be defaulting to the system timezone and then converting that to the default?

At the end of the day, I still think any manipulation we do of timezone info on naive datetime objects means we are guessing and likely hiding errors for downstream projects. The fact that there is disagreement of what should be the default indicates to me that there is no good universal default.

lingua_franca/time.py Outdated Show resolved Hide resolved
lingua_franca/time.py Outdated Show resolved Hide resolved
lingua_franca/time.py Outdated Show resolved Hide resolved
JarbasAl
JarbasAl previously approved these changes Aug 17, 2021
@ChanceNCounter
Copy link
Contributor Author

Review changes: Irrelevant funcs removed. Default TZ set to UTC.

Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing those.

I don't think we've yet got to the bottom of the timezone rabbithole. Have you got a project where you actually use this? Some code we can look at to reason out what would happen in different scenarios?

The question in my mind is whether we assume that the majority of tz naive datetime objects are being generated

  1. In reference to the system clock (such as by datetime.datetime.now()) or
  2. by something that inherently references the users local timezone like like an API or some manually constructed datetime object.

For case 1 - if I have something that calls datetime.datetime.now() and then try to convert that datetime to local or UTC - replacing it with either UTC or some other default TZ - the result is going to be incorrect depending on what the system timezone is set to. So if case 1 is the most likely then my assumption is that we should be replacing it with the systems timezone info

If case 2 is more likely then replacing it with the configured __default_tz makes the most sense to me.

@@ -17,7 +17,7 @@
from dateutil.tz import gettz, tzlocal


__default_tz = None
__default_tz = gettz("UTC")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make the default_timezone() function obsolete? at least the or tzlocal() seems to be.

@@ -46,7 +46,7 @@ def now_utc():
Returns:
(datetime): The current time in Universal Time, aka GMT
"""
return to_utc(datetime.utcnow())
return datetime.now(gettz("UTC"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer 👍

Comment on lines +77 to +81
# naive datetimes assumed to be in default timezone already!
# in the case of datetime.now this corresponds to tzlocal()
# otherwise timezone is undefined and can not be guessed, we assume
# the user means "my timezone" and that LN was configured to use it
# beforehand, if unconfigured default == tzlocal()
Copy link
Contributor

@krisgesling krisgesling Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to update these comments

@ChanceNCounter
Copy link
Contributor Author

Having lost the (internal) single source of truth, I'd just as happily clobber naive DTs to the system TZ. The problem arises in the broader ecosystem. The original reason I suggested using LF.time as the source of truth was to ensure that configs and API params would have something to query that only changes on config. If we're comfortable trusting the system clock, and allowing everything to fail in the cases where that fails, alright. That's what all the downstream cores seem to be doing.

@krisgesling krisgesling self-assigned this Oct 29, 2021
NeonJarbas pushed a commit to NeonJarbas/ovos-lingua-franca that referenced this pull request Jan 18, 2022
add to_system utility

fix extract_datetime_fr timezone bug  MycroftAI#218

fix farsi tests (ignoring timezone due to not being rebased before merge)

remove unimplemented farsi code (is_fractional and normalize)

split english tests into their own files like the other languages
JarbasAl pushed a commit to OpenVoiceOS/ovos-lingua-franca that referenced this pull request Jan 18, 2022
* port timezone fixes for time utils MycroftAI#209

add to_system utility

fix extract_datetime_fr timezone bug  MycroftAI#218

fix farsi tests (ignoring timezone due to not being rebased before merge)

remove unimplemented farsi code (is_fractional and normalize)

split english tests into their own files like the other languages + remove solved TODO comment

authored-by: jarbasai <[email protected]>
NeonJarbas pushed a commit to NeonJarbas/ovos-lingua-franca that referenced this pull request Jan 18, 2022
add to_system utility

fix extract_datetime_fr timezone bug  MycroftAI#218

fix farsi tests (ignoring timezone due to not being rebased before merge)

remove unimplemented farsi code (is_fractional and normalize)

split english tests into their own files like the other languages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
No open projects
Status: Under active review
Development

Successfully merging this pull request may close these issues.

The test_format_ and test_parse_fa tests has errors
4 participants