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

Review the datetime helpers in gcloud._helpers. #1732

Closed
rimey opened this issue Apr 21, 2016 · 8 comments
Closed

Review the datetime helpers in gcloud._helpers. #1732

rimey opened this issue Apr 21, 2016 · 8 comments
Assignees
Labels
api: core priority: p2 Moderately-important priority. Fix may not be included in next release.

Comments

@rimey
Copy link
Contributor

rimey commented Apr 21, 2016

gcloud._helpers has accumulated a collection of ~10 helper functions related to datetime handling. These could perhaps use a review to determine if they are all essential and maximally useful.

The particular situation in which this has come up is a proposal to update _datetime_to_rfc3339() to handle time zones as in the following module-private helper function appearing in #1691:

def _format_timestamp(timestamp):
    """Convert a datetime object to a string as required by the API.

    :type timestamp: :class:`datetime.datetime`
    :param timestamp: A datetime object.

    :rtype: string
    :returns: The formatted timestamp. For example:
        ``"2016-02-17T19:18:01.763000Z"``
    """
    if timestamp.tzinfo is not None:
        # Convert to UTC and remove the time zone info.
        timestamp = timestamp.replace(tzinfo=None) - timestamp.utcoffset()

    return timestamp.isoformat() + 'Z'
@dhermes
Copy link
Contributor

dhermes commented Apr 22, 2016

@tseaver you have a better feel for datetime than I do, WDYT about adding tz support to _datetime_to_rfc3339?

Some context for the overall number of methods, we essentially have to support all of the flavors of datetime from the various Google APIs:

  • _millis_from_datetime (millisecond precision in BigQuery, _millis is a helper used only by _millis_from_datetime)
  • _datetime_from_microseconds / _microseconds_from_datetime (the most common use case)
  • _rfc3339_to_datetime / _rfc3339_nanos_to_datetime / _datetime_to_rfc3339 (for APIs that use strings for datetimes, unfortunately some use microsecond precision and others use nanosecond precision, which Python's datetime doesn't support)
  • _pb_timestamp_to_datetime / _datetime_to_pb_timestamp (for protobuf APIs)
  • _total_seconds_backport / _total_seconds (legacy Py 2.6 support which will be sunsetted in Call for comments: Python 2.6 support will be dropped August 1, 2016 #1607)

@rimey
Copy link
Contributor Author

rimey commented Apr 22, 2016

Part of the broader context here is time zone usage in the library's Python interface. I would suggest the following guidelines for this library:

  • Returned datetime objects should generally be in UTC. They may be naive or timezone-aware, depending on the particular interface.
  • Interfaces may require input datetime objects to be naive UTC. It is preferable to also accept timezone-aware datetime objects, but in that case, naive datetime objects (assumed to be UTC) must also be accepted.

Naive datetime objects can be easier to work with, while aware datetime objects have the virtue of being self-documenting. There may also be other considerations for particular interfaces. People familiar with the issue sometimes have a preference for working in one style or the other. The Python standard library, of course, defines a framework encompassing both styles but provides no concrete tzinfo classes (not even for UTC).

@tseaver
Copy link
Contributor

tseaver commented Apr 22, 2016

To date, we have strongly preferred to avoid naive datetimes: the presumed ease-of-use goes away when dealing with an API which requires that the zone be explicit. In cases where we have accepted them from the caller, we have forcibly converted it before working with it further. Where an API returns a string timestamp, we also forcibly convert it.

We use pytz.UTC if possible, and fall back to our own forked version if not.

@rimey
Copy link
Contributor Author

rimey commented Apr 22, 2016

What APIs require "that the zone be explicit"? It's hard to imagine this being very common, given how the Python standard library doesn't provide a time zone implementation.

@dhermes
Copy link
Contributor

dhermes commented Apr 22, 2016

(See #1044 for the history of "our own forked version". I was hoping to find the reference I used when creating it, but I can't track it down.)

@rimey
Copy link
Contributor Author

rimey commented Apr 22, 2016

Introducing your own UTC tzinfo and switching between that and pytz.UTC depending on whether pytz is available makes perfect sense for the narrow purpose of returning aware datetime objects for the sake of explicitness. But, as @tseaver said, the library actually embraces time zones a bit more widely than that.

#1706 came up because installing pandas annoyingly brings in pytz. I found myself wondering if I really trust gcloud-python to work correctly in this case, which isn't covered by the automated testing.

Touching time zones at only a small number of choke points in the code is the right answer, if one must touch them at all.

@tseaver
Copy link
Contributor

tseaver commented Apr 22, 2016

@rimey The APIs document that the timestamps being pushed across the wire are in UTC:

Datastore

Storage

Pubsub

  • If passed, PubsubMessage.publishTime is required to be in RFC 3339 Zulu format at nanosecond precision (annoyingly, it is sometimes returned truncated, but it always has the Z).

BigQuery

  • Fields of type `TIMESTAMP are converted to Unix epoch values with microsecond precision. Input values which are floats are assumed to already be UTC; likewise string input values without explicit zones. String input values with explicit zones are converted to UTC before storage. The API returns TIMESTAMP values as microseconds since the UTC epoch.

Logging

DNS

  • Changes.startTime is defined as " RFC3339 text format." Because we don't have system tests running yet for it, I'm not positive that it includes the Zulu suffix.

@lukesneeringer lukesneeringer added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Apr 19, 2017
@lukesneeringer
Copy link
Contributor

Hello,
One of the challenges of maintaining a large open source project is that sometimes, you can bite off more than you can chew. As the lead maintainer of google-cloud-python, I can definitely say that I have let the issues here pile up.

As part of trying to get things under control (as well as to empower us to provide better customer service in the future), I am declaring a "bankruptcy" of sorts on many of the old issues, especially those likely to have been addressed or made obsolete by more recent updates.

My goal is to close stale issues whose relevance or solution is no longer immediately evident, and which appear to be of lower importance. I believe in good faith that this is one of those issues, but I am scanning quickly and may occasionally be wrong. If this is an issue of high importance, please comment here and we will reconsider. If this is an issue whose solution is trivial, please consider providing a pull request.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: core priority: p2 Moderately-important priority. Fix may not be included in next release.
Projects
None yet
Development

No branches or pull requests

4 participants