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

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

more test coverage

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

test fix

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

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

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

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

sashank added inline comments.Mon, Oct 12, 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.Mon, Oct 12, 9:26 PM

defer to @sashank

dgibson added inline comments.Tue, Oct 13, 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!Wed, Oct 14, 3:05 PM

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

Take timezone into account when computing partition names/values

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

add more testing

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

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

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

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

python_modules/dagster/dagster/scheduler/scheduler.py
212–215	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
111–113	I don't think so no, there's no guarantee that the server running them will be in UTC
145	yeah, will update to make more clear

chefkiss

python_modules/dagster/dagster/core/definitions/schedule.py
148	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.Wed, Oct 14, 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)