Page MenuHomePhabricator

Add additional validation for start date and end date timezones
Needs RevisionPublic

Authored by dgibson on Tue, Jan 5, 8:24 PM.

Details

Summary

Based on rex's good suggestion here: https://dagster.phacility.com/D5814

Be more aggressive about timezone validation - if you set one, and you set a timezone on your start time or end time, it should match up with the execution timezone. Otherwise it's highly likely that your schedule will not be starting or ending where you expect.

Test Plan

BK

Diff Detail

Repository
R1 dagster
Branch
rethinktimezonewarn (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

python_modules/dagster/dagster/utils/partitions.py
50

execution_timezone

53

I'm confused... why do start/end time have to be a pendulum instance? I thought that pendulum was just a helper library....

Couldn't you do the same checks on a normal datetime object?

python_modules/dagster/dagster/utils/partitions.py
53

Discussed this off-thread, I guess other tz libraries don't handle DST very well.

would consider downgrading the pendulum instance error check to a warning, but this seems fine

This revision is now accepted and ready to land.Wed, Jan 6, 5:24 PM

the problem is that there's no way to validate or verify the timezone name if they're using some other tzinfo - the tzinfo spec is... quite bad.

"https://docs.python.org/3/library/datetime.html#datetime.tzinfo.tzname" ("Nothing about string names is defined by the datetime module, and there’s no requirement that it mean anything in particular. For example, “GMT”, “UTC”, “-500”, “-5:00”, “EDT”, “US/Eastern”, “America/New York” are all valid replies")

OK, I ended up rethinking this a bit, rather than being very strict about exactly what comes in, transform it into a Pendulum timezone

I think this is in a slightly confusing state, I would guess due to how it started vs the latest approach. Sending to you for one more spin for some mix of more tests or renaming / rewording

python_modules/dagster/dagster/utils/partitions.py
48–60

will this fail in any condition? coerce_schedule_boundary may be a more appropriate name

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
405–415

given the title of the diff I would expect additional tests for validation of start_date and end_date, but it seems we're now just doing a coercion so maybe update title?

should there still be tests of the start_date/end_date coercion?

This revision now requires changes to proceed.Mon, Jan 11, 6:08 PM