Folds and imaginary datetimes in the datetime strategy

[pganssle]

I was just testing a function using hypothesis and I realized that hypothesis never generates a datetime with the fold value set. When I went to fix this, I realized that there are two ways to handle how ambiguous and imaginary datetimes are handled with aware datetimes, and the current strategy is actually inconsistent in this regard.

The problem is whether you think of strategies.datetime as generating arbitrary datetime objects or whether they are intended to generate valid datetimes in a given zone. Currently, if you generate an aware datetime and pass a strategy that generates pytz zones, you'll always get valid datetimes, because the library actively detects pytz zones and calls normalize(localize(dt)) on the datetimes generated. If it draws any other kind of zone, hypothesis may generate an imaginary datetime, but if hypothesis picks an ambiguous datetime, it will only ever generate one side of the transition (whichever one happens chronologically first, for dateutil).

If you want it to generate arbitrary datetimes, we can drop the normalize call, and set fold randomly for every datetime generated. If you draw from st.booleans(), which shrinks towards False, fold will almost always shrink away because it usually doesn't affect the behavior of the datetime. You will need to special-case pytz here, and instead of setting fold set the is_dst flag in the localize call.

If you always want the aware datetimes generated by hypothesis to be valid (non-imaginary), then you can avoid hard-coding any awareness of pytz's unusual interface and just draw the datetimes as UTC, then convert. This will make the behavior uniform between dateutil and pytz and will set fold only for ambiguous datetimes but it will never generate imaginary datetimes.

I think these are both reasonable behaviors for datetimes. My inclination is to add a flag like allow_imaginary that switches between them and defaults to True. An alternative to this would be to stick to one or the other and have people who want the behavior not chosen create it themselves by generating a naive datetime and a timezone separately, then combining them themselves, but I think that will make it much harder for #69 to be effective.

I'm happy to implement whatever decision we come to.

[Zac-HD]

Fantastic writeup, thank you!

I'd be delighted to accept a PR adding allow_imaginary=True as a keyword-only argument.

Also very interested in setting the fold attribute (on 3.6+, or is_dst on any version), and the associated machinery. Just fiddly to stay compatible with Python 3.5 in the meantime...

[Zac-HD]

@pganssle - how does master...Zac-HD:better-datetimes look to you?

It adds the allow_imaginary argument and generates .fold (py36+), but I'm not entirely convinced that I'm getting all the subtleties of utcoffsets and timezones right... or excluding imaginary datetimes when using pytz quite the way I hope to.

[pganssle]

@Zac-HD Sorry I haven't had time to make a proper PR for this.

Few things:

1. Seems like you went with allow_imaginary=False by default. I thought we were going to go with allow_imaginary=True as the default.
2. I'm not clear on why you are doing the utcoffset math like this at all, but I don't think it's quite going to work for pytz, and it seems unnecessary for dateutil or other zones like that. Here's a little mock function that I think does what you want:

def _draw_naive(min_dt, max_dt):
    ... # Assume that this draws a naive datetime in the range [min_dt, max_dt]
        # and sets `fold` randomly in Python 3.6+

def _draw_real(min_dt, max_dt, tz):
    def to_naive_utc(dt):
        if dt.tzinfo is None:
            dt.replace(tzinfo=tz)
        return dt.astimezone(timezone.utc).replace(tzinfo=None)

    min_dt_utc, max_dt_utc = map(to_naive_utc, (min_dt, max_dt))

    dt_utc = _draw_naive(min_dt, max_dt)
    return dt_utc.replace(tzinfo=timezone.utc).astimezone(tz)

def _draw_any(min_dt, max_dt, tz):
    dt = _draw_naive(min_dt, max_dt)
    if is_pytz_timezone(tz):
        # fold and is_dst aren't necessarily the exact same thing
        # but they're both booleans and it's randomly chosen anyway
        dt = tz.localize(dt, is_dst=not dt.fold)
    else:
        dt = dt.replace(tzinfo=tz)

def do_draw(self, data):
    tz = data.draw(self.tz_strat)
    if self.allow_imaginary:
        return _draw_any(self.min_dt, self.max_dt, tz)
    else:
        return _draw_real(self.min_dt, self.max_dt, tz)

I think you'll need to refactor what I have here to get it to work correctly with the _draw_naive_datetime that you have, but hopefully you get the gist. For allow_imaginary=False, you want:

1. Convert min_dt and max_dt into UTC
2. Draw a naive datetime in that range
3. Reinterpret it as a UTC datetime
4. Convert that UTC datetime to the zone you want (it will set fold correctly in all cases and skip imaginary datetimes)

For allow_imaginary=True, you want:

1. Draw a naive datetime with a randomly-assigned fold
2. If it's pytz, pass the fold as the is_dst to localize (the two have inverted senses so I've inverted it here, but since they're randomly drawn bools you can just pass directly as well)
3. If it's not pytz, just do .replace(tzinfo=tz) on the naive datetime and you are done.

I think the biggest downside with the "ensure that it's a real time zone" thing is that it requires invoking .astimezone(), which means that when you're testing time zone libraries I bet it's going to give some gnarly errors if you break that function. I don't really see any way around using the time zone functions for this, though.

[Zac-HD]

Seems like you went with allow_imaginary=False by default. I thought we were going to go with allow_imaginary=True as the default.

It's true-by-default in the public API (function); false in the class because we reuse the machinery for times(). Which I really need to split out into it's own class, actually, not all timezones exist at all dates... I think there's going to be a refactoring PR merged before I actually get to the allow_imaginary part.

I'm not clear on why you are doing the utcoffset math like this at all

Because the bounds are currently expressed in naive time, and as I understand it interpreting as UTC then converting can change the naive part of the datetime to possibly-out-of-bounds.

On reflection it's probably a lot safer just to check for and reject imaginary times! (interpret as timezone, convert to UTC and back, reject if unequal)

Supporting tz-aware bounds is definitely on my list of nice-to-haves, but there's a fair bit of refactoring to do first!

I think the biggest downside with the "ensure that it's a real time zone" thing is that it requires invoking .astimezone(), which means that when you're testing time zone libraries I bet it's going to give some gnarly errors if you break that function. I don't really see any way around using the time zone functions for this, though.

Eh, this sounds pretty normal to me really - everything breaks when I point Hypothesis at it. So long as the bugs get fixed I'm happy!

[pganssle]

Because the bounds are currently expressed in naive time, and as I understand it interpreting as UTC then converting can change the naive part of the datetime to possibly-out-of-bounds.

On reflection it's probably a lot safer just to check for and reject imaginary times! (interpret as timezone, convert to UTC and back, reject if unequal)

Hm.. So I did consider this, though obviously didn't put enough thought into actually dealing with the edge cases.

I think you may have the same problem with the "convert to UTC and back" problem if it's a valid local time but it can't be represented in UTC because of the boundary conditions. You'll still fail to generate the UTC times for something like datetime(1, 1, 1, tzinfo=timezone(timedelta(hours=1)).

I think the options are to either:

1. Assume that there are no gaps in the regions that cannot be represented in UTC, and include those in the resulting outputs.
2. Restrict the domain of valid datetimes to the overlap between valid local times and valid UTC times.

I'm not sure if either is better than the other. The non-restricted version is more likely to turn up bugs, but they are likely to have no real effect on "business logic", since time zone offsets are very close to meaningless before ~1900 and decreasingly accurate the further into the future you go, so correctly handling time zones at 0 CE or 9999 CE are very much YAGNI situations.

I'm inclined to say restrict the domain of valid datetimes to the overlap between the two and just do the draw in UTC, because I think that's the domain that people who would opt in to allow_imaginary=False would want anyway.

One other point to consider about the "draw a naive and reject if it can't round trip" option is that people may accidentally set the boundaries in such a way that imaginary datetimes are a large fraction of the valid search space, which I assume would be very slow.

[Zac-HD]

OK, I think I've been trying to fix too many issues with datetime support at once. Here they are:

• Moving the relevant functions from strategies._internal.core to live with the SearchStrategy subclasses in strategies._internal.datetime
• Refactoring the dates() strategy for structured shrinking
• Give the times() strategy cleaner tz support by splitting it from datetimes()

After the refactoring, some feature improvements:

• Generate a fold attribute on Python >= 3.6
• Make pytz use localize(is_dst=...) without normalize(), for consistency
• Add an allow_imaginary=True arg, initially with a filter-based implementation

which should be much easier to both write and review as two PRs!

---

And finally some options which would be nice-to-have but are not on the roadmap:

• Valid datetimes by construction (not filtering), with nice boundary condition support
• Timezone-aware bounds. Clearly useful, obvious a big change to the semantics, different issue and one I'm not working on until someone actually asks for the feature.