Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

st.datetimes(allow_imaginary=False) can raise internal error #2662

Closed
mvaled opened this issue Nov 11, 2020 · 6 comments · Fixed by #2666
Closed

st.datetimes(allow_imaginary=False) can raise internal error #2662

mvaled opened this issue Nov 11, 2020 · 6 comments · Fixed by #2666
Labels
bug something is clearly wrong here legibility make errors helpful and Hypothesis grokable

Comments

@mvaled
Copy link
Contributor

mvaled commented Nov 11, 2020

strategies.datetime may raise such an error when passed allow_imaginary=False and tzinfo is None.

I have the following:

dates = datetimes(
    min_value=datetime.datetime(2000, 1,1),
    max_value=datetime.datetime(9000, 12,31),
    allow_imaginary=False,
)

The error looks like (pytest style):

value = datetime.datetime(2000, 1, 1, 0, 0)

    def datetime_does_not_exist(value):
        # This function tests whether the given datetime can be round-tripped to and
        # from UTC.  It is an exact inverse of (and very similar to) the dateutil method
        # https://dateutil.readthedocs.io/en/stable/tz.html#dateutil.tz.datetime_exists
        try:
            # Does the naive portion of the datetime change when round-tripped to
            # UTC?  If so, or if this overflows, we say that it does not exist.
            roundtrip = value.astimezone(dt.timezone.utc).astimezone(value.tzinfo)
        except OverflowError:
            # Overflows at datetime.min or datetime.max boundary condition.
            # Rejecting these is acceptable, because timezones are close to
            # meaningless before ~1900 and subject to a lot of change by
            # 9999, so it should be a very small fraction of possible values.
            return True
>       assert value.tzinfo is roundtrip.tzinfo, "so only the naive portions are compared"
E       AssertionError: so only the naive portions are compared
E       assert None is datetime.timezone(datetime.timedelta(days=-1, seconds=68400), 'CST')
E        +  where None = datetime.datetime(2000, 1, 1, 0, 0).tzinfo
E        +  and   datetime.timezone(datetime.timedelta(days=-1, seconds=68400), 'CST') = datetime.datetime(2000, 1, 1, 0, 0, tzinfo=datetime.timezone(datetime.timedelta(days=-1, seconds=68400), 'CST')).tzinfo

.venv/lib/python3.8/site-packages/hypothesis/strategies/_internal/datetime.py:75: AssertionError

Notice that value.tzinfo is None, so astimezone uses the local timezone.

@mvaled
Copy link
Contributor Author

mvaled commented Nov 11, 2020

To work around this issue I'm using:

dates = st.datetimes(
    min_value=datetime.datetime(2000, 1, 1),
    max_value=datetime.datetime(9000, 12, 31),
    timezones=st.just(datetime.timezone.utc),
    allow_imaginary=False,
).map(lambda d: d.replace(tzinfo=None, fold=0))

@Zac-HD Zac-HD added bug something is clearly wrong here legibility make errors helpful and Hypothesis grokable labels Nov 12, 2020
@Zac-HD
Copy link
Member

Zac-HD commented Nov 12, 2020

Thanks for reporting this! I think our fix is probably to disallow allow_imaginary=False for naive (tzinfo=None) datetimes, since imaginary times are only defined with respect to a particular timezone.


Unfortunately your workaround still permits the generation of imaginary times - while UTC has no imaginary times, replacing UTC with your local timezone may create one. Instead, use:

dates = st.datetimes(
    min_value=datetime.datetime(2000, 1, 1),
    max_value=datetime.datetime(9000, 12, 31),
    timezones=st.just(datetime.timezone.utc),
).map(lambda d: d.astimezone(tz=None))

@mvaled
Copy link
Contributor Author

mvaled commented Nov 12, 2020

@Zac-HD

Thanks.

But, if I understood the docs correctly, d.astimezone(tz=None) does use the local timezone and returns a non-naive datetime.

Since, what I need are naive datetimes (but still regarded as datetimes in UTC), I think d.replace(tzinfo=None) is the way to go. Am I missing something?

@Zac-HD
Copy link
Member

Zac-HD commented Nov 12, 2020

There's no such thing as an imaginary datetime with tzinfo=None, so you can simply use

dates = st.datetimes(
    min_value=datetime.datetime(2000, 1, 1), max_value=datetime.datetime(9000, 12, 31)
)

More generally, if your datetime objects represent a time in UTC I would strongly recommend using tzinfo=UTC - as you've seen above, tzinfo=None has some pretty unpredicatble semantics around comparisons and conversion to local time. The docs for datetime.utcnow() carry the same warning.

@mvaled
Copy link
Contributor Author

mvaled commented Nov 12, 2020

@Zac-HD

Thank a lot. I'm still wrapping my head around the imaginary datetime stuff. Unfortunately, I cannot use tzinfo=UTC because I'm interfacing with the Odoo framework which requires all datetime objects be naive although regarded as in UTC.

Best regards,
Manuel.

@Zac-HD
Copy link
Member

Zac-HD commented Nov 12, 2020

Well... I'd try to use tzinfo=UTC in all your code, and then replace with None just before calling into their framework. And open a ticket asking for support of non-naive datetimes, because naive-only is really just asking for bizzare transient bugs depending on the server timezone, daylight savings, and so on.

@Zac-HD Zac-HD changed the title AssertionError: so only the naive portions are compared st.datetimes(allow_imaginary=False) can raise internal error Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is clearly wrong here legibility make errors helpful and Hypothesis grokable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants