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

UTC datetime handling improvements #716

Merged
merged 23 commits into from
Mar 19, 2024
Merged

Conversation

motus
Copy link
Member

@motus motus commented Mar 16, 2024

Addresses some deprecation warnings about use of datetime.utcnow().

Additionally, sqlite does not store timezone info even if provided, so retrieving the data may be interpreted according to the host machine's local timezone, which may or may not be UTC.

To mitigate this, this change ensures that all timestamps are

  1. first converted to UTC before storing into the DB,
  2. converted (or augmented with zoneinfo) to UTC on retrieval

Additionally, we expand the tests to check for this behavior, first with some additional conversion matrixes when telemetry or status data is received in implicit local vs. explicit timezone data as well as executions where the implicit local timezone has be overridden with a TZ environment variable, to simulate different default timezone hosts.

Closes #718

@motus motus added the ready for review Ready for review label Mar 16, 2024
@motus motus self-assigned this Mar 16, 2024
@motus motus requested a review from a team as a code owner March 16, 2024 00:17
@motus motus added WIP Work in progress - do not merge yet and removed ready for review Ready for review labels Mar 16, 2024
@bpkroth
Copy link
Contributor

bpkroth commented Mar 18, 2024

I started hacking on this a little locally as well. Will post a PR against yours in a bit...

motus#13

@bpkroth bpkroth changed the title Get rid of deprecated datetime.utcnow() and .utcfromtimestamp() functions UTC datetime handling improvements Mar 18, 2024
@bpkroth bpkroth added ready for review Ready for review and removed WIP Work in progress - do not merge yet labels Mar 19, 2024
Copy link
Contributor

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

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

LGTM, but I wrote half of it :)
@motus, do you want to take another look?

mlos_bench/mlos_bench/tests/__init__.py Outdated Show resolved Hide resolved
mlos_bench/mlos_bench/tests/__init__.py Outdated Show resolved Hide resolved
mlos_bench/mlos_bench/environments/local/local_env.py Outdated Show resolved Hide resolved
bpkroth and others added 4 commits March 19, 2024 15:19
Co-authored-by: Sergiy Matusevych <sergiy.matusevych@gmail.com>
Co-authored-by: Sergiy Matusevych <sergiy.matusevych@gmail.com>
@bpkroth bpkroth enabled auto-merge (squash) March 19, 2024 20:40
@bpkroth bpkroth merged commit 647e3a2 into microsoft:main Mar 19, 2024
12 checks passed
@motus motus deleted the sergiym/datetime/utc branch March 19, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get rid of deprecated functions like datetime.utcnow()
2 participants