Page MenuHomePhabricator

Replace timezone-unsafe use of now() and utcnow()
Needs RevisionPublic

Authored by max on Oct 9 2020, 3:58 PM.

Details

Reviewers
dgibson
dish
Summary

Also adds custom linter rules to catch use of these functions.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Branch
utcnow
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 9 2020, 4:14 PM
Harbormaster failed remote builds in B19367: Diff 23534!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 9 2020, 6:31 PM
Harbormaster failed remote builds in B19372: Diff 23542!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 9 2020, 7:47 PM
Harbormaster failed remote builds in B19378: Diff 23549!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 9 2020, 10:18 PM
Harbormaster failed remote builds in B19390: Diff 23563!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 10 2020, 1:45 PM
Harbormaster failed remote builds in B19392: Diff 23565!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 27 2020, 10:15 PM
Harbormaster failed remote builds in B20277: Diff 24603!
Harbormaster failed remote builds in B20274: Diff 24599!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 28 2020, 6:49 PM
Harbormaster failed remote builds in B20338: Diff 24672!
Harbormaster failed remote builds in B20339: Diff 24673!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 28 2020, 9:01 PM
Harbormaster failed remote builds in B20349: Diff 24684!
max requested review of this revision.Oct 28 2020, 9:34 PM
dgibson requested changes to this revision.Oct 29 2020, 3:12 PM

love a good timezone diff. The big thing is that I think we should standardize on pendulum datetimes? (and probably make get_utc_timezone private rather than adding more callsites)

python_modules/dagster-graphql/dagster_graphql_tests/graphql/setup.py
865

can this just be something like time.time(hour=9, minute=30), why does it even care what time it is now

python_modules/dagster/dagster/core/storage/event_log/schema.py
12

do we need a migration for this? or are we fine with it only kicking in for new users?

python_modules/dagster/dagster/core/storage/event_log/sql_event_log.py
31

So this adds get_utc_timezone calls in a bunch of new places - should we settle on using pendulum timezones instead since that is what get_current_datetime_in_utc uses, so that there's a single timezone representation in the codebase?

python_modules/dagster/dagster/utils/__init__.py
35

pendulum.from_timestamp(0)?

452–453

dt = pendulum.instance(dt).in_tz("UTC")?

527

pendulum.from_timestamp(timestamp)?

python_modules/dagster/dagster/utils/partitions.py
56–61

This is the desired end state and where i'm planning that we get to in 0.10.0 (see https://github.com/dagster-io/dagster/issues/3128 which tracks adding a requirement that you specify a timezone), but changing it right now could break existing users' schedules, if they're assuming that the datetime they specify is in the system timezone

I think it's also not true as currently written? on line 112 we still set the timezone to the system timezone if one isn't currently set (via pendulum.now().timezone.name)

python_modules/libraries/dagster-airflow/dagster_airflow/cli.py
45

i don't know enough about airflow to understand the implications of this change

python_modules/libraries/dagster-aws/dagster_aws/cloudwatch/loggers.py
125

pendulum.instance(datetime) instead of datetime.replace?

This revision now requires changes to proceed.Oct 29 2020, 3:12 PM