Details

Reviewers

sashank
alangenfeld

Commits

R1:fe88d204fa88: Add execution_timezone field to schedules

Summary

Allows individual schedules to run at a fixed timezone (overridding whatever timezone has been set on the instance).

Test Plan

Diff Detail

Repository

R1 dagster

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

dgibson created this revision.Oct 8 2020, 5:40 PM

dgibson retitled this revision from Add execution_timestamp field to schedules to Add execution_timezone field to schedules.Oct 8 2020, 5:43 PM

more test coverage

Harbormaster failed remote builds in B19323: Diff 23483!Oct 8 2020, 5:59 PM

test fix

Harbormaster failed remote builds in B19324: Diff 23484!Oct 8 2020, 6:16 PM

Harbormaster completed remote builds in B19326: Diff 23486.Oct 8 2020, 6:27 PM

dgibson requested review of this revision.Oct 8 2020, 6:27 PM

dgibson added reviewers: alangenfeld, sashank.Oct 8 2020, 9:05 PM

sashank added inline comments.Oct 12 2020, 8:54 PM

python_modules/dagster/dagster/core/definitions/decorators/schedule.py
513–517	Won't this be confusing in the playground and backfill UIs now? I feel like if they want their schedule to run every hour exactly, they should use UTC execution_timezone. But if they configure an hourly schedule in a DST timezone, the partitions should be local to their timezone and the behavior at DST boundaries should be documented.

sashank added a parent revision: D4671: Make scheduler timezone-aware.Oct 12 2020, 9:26 PM

defer to @sashank

dgibson added inline comments.Oct 13 2020, 6:02 PM

python_modules/dagster/dagster/core/definitions/decorators/schedule.py
513–517	Made the partition names (and values) timezone-aware in https://dagster.phacility.com/D4761

Ditch the instance-level default timezone (going to tackle getting the partition names and values exactly right separately)

Harbormaster failed remote builds in B19490: Diff 23679!Oct 14 2020, 3:05 PM

Harbormaster completed remote builds in B19495: Diff 23684.Oct 14 2020, 3:40 PM

Take timezone into account when computing partition names/values

Harbormaster failed remote builds in B19499: Diff 23688!Oct 14 2020, 4:10 PM

add more testing

Harbormaster failed remote builds in B19515: Diff 23708!Oct 14 2020, 8:29 PM

sashank added inline comments.Oct 14 2020, 9:15 PM

python_modules/dagster/dagster/core/definitions/decorators/schedule.py
37	Maybe mark this argument on all the decorators with the new experimental feature flags
python_modules/dagster/dagster/scheduler/scheduler.py
213–216	Where do these show up again?
python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_timezones.py
112–114	Shouldn't all these tests now freeze the time in UTC?
146	The result would be the same even if this were 1 minute later right?

dgibson added inline comments.Oct 14 2020, 9:17 PM

python_modules/dagster/dagster/scheduler/scheduler.py
213–216	just as output in the scheduler process For 0.10.0 IMO we should require execution_timezone on all schedules, so this should become an invariant violation
python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_timezones.py
112–114	I don't think so no, there's no guarantee that the server running them will be in UTC
146	yeah, will update to make more clear

chefkiss

python_modules/dagster/dagster/core/definitions/schedule.py
149	Is there some sort of built in type in `datetime` that we can use here to represent the timezone instead of a string?

This revision is now accepted and ready to land.Oct 14 2020, 9:21 PM

fix some issues with calculating the partition times during DST transitions (can't trust relativedelta or timedelta, can trust pendulum.datetime.subtract and pendulum.range)