Page MenuHomePhabricator

Make scheduler timezone-aware
ClosedPublic

Authored by dgibson on Mon, Oct 5, 7:45 PM.

Details

Summary
  • Use pendulum datetimes, which have much better timezone and DST handling.
  • Associate a timezone string with the scheduler, do the cron math using that timezone (and then convert back to UTC for the ticks)
Test Plan

Existing BK tests cover a scheduler in UTC. Next diff will add coverage for non-UTC timezones and daylight savings time

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

Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Oct 5, 8:03 PM
Harbormaster failed remote builds in B19143: Diff 23263!
Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Oct 5, 8:50 PM
Harbormaster failed remote builds in B19146: Diff 23270!

unpin dagster-airflow version (airflow is no longer pinned to that version)

Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Oct 5, 9:14 PM
Harbormaster failed remote builds in B19149: Diff 23273!
Harbormaster failed remote builds in B19148: Diff 23272!

more messing with airflow deps

Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Oct 5, 9:52 PM
Harbormaster failed remote builds in B19158: Diff 23282!

downgrade pendulum version for now to match pinned airflow version

And we're back with much more testing

remove unrelated changes, py2 fixes

+ max since he has expressed some interest in pendulum

python_modules/dagster/dagster/grpc/types.py
207–223

not that this is something we are worried about yet - but this is going to cause a version incompatibility problem right?

python_modules/dagster/dagster/grpc/types.py
207–223

it is

python_modules/dagster/dagster/grpc/types.py
207–223

I *think* all you would need to do make it compat is add =None on the new args, serdes already does a check to drop unknown args by keyword

seems worth it since they are already opt_

sashank added inline comments.
python_modules/dagster/dagster/core/definitions/decorators/schedule.py
253

Oh wow this was just completely wrong before, wasn't it

python_modules/dagster/dagster/scheduler/scheduler.py
157

Nit: We might want to have an error message here, so people know why this line is failing.

Something along the lines of:

In order to use the scheduler run command, you need to have DagsterCommandLineScheduler configured, but you have DagsterCronScheduler configured
188–189

👍 🎉

188–189

It might be helpful to add that example you showed me inline for future readers to know exactly what you're talking about

440

Should we add another tag that's in the local execution time? To help people debug?

This revision is now accepted and ready to land.Mon, Oct 12, 9:25 PM
python_modules/dagster/dagster/core/definitions/decorators/schedule.py
253

I don't think it was wrong when everything was in UTC - it may not even be wrong now, but its no longer needed to get the time exactly right as long as you're on the right day and will produce the right partition name

python_modules/dagster/dagster/core/definitions/decorators/schedule.py
253

once you're no longer in UTC it's no longer true that, say, going back 3 hours at 3AM is no will always reach midnight, which you can count on in UTC

This revision was automatically updated to reflect the committed changes.