Page MenuHomeElementl

Replace timezone-unsafe use of now() and utcnow()
AbandonedPublic

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

Details

Summary

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

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Branch
utcnow
Lint
Lint Errors
SeverityLocationCodeMessage
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:210E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:212E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:221E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:222E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:240E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:241E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:255E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:256E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:369E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:372E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:381E0602Undefined Variable
Errorintegration_tests/test_suites/celery-k8s-integration-test-suite/test_integration.py:383E0602Undefined Variable
Errorpython_modules/dagster/dagster/utils/partitions.py:98E0602Undefined Variable
Errorpython_modules/dagster/dagster/utils/partitions.py:101E0602Undefined Variable
Errorpython_modules/libraries/dagster-aws/dagster_aws_tests/cloudwatch_tests/test_loggers.py:87E0602Undefined Variable
Errorpython_modules/libraries/dagster-ge/dagster_ge/factory.py:104E0602Undefined Variable
Unit
No 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!

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
864

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
34

pendulum.from_timestamp(0)?

452–453

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

526

pendulum.from_timestamp(timestamp)?

python_modules/dagster/dagster/utils/partitions.py
54–59

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
44

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
max added inline comments.
python_modules/dagster-graphql/dagster_graphql_tests/graphql/setup.py
864

i don't have any idea what this test setup is meant to be doing so i don't want to mess with the semantics

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

i don't understand the logic behind get_utc_timezone at all -- i would define a single constant like dagster.seven.UTC given my druthers

python_modules/dagster/dagster/utils/__init__.py
34

yep