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

problem with utc.localize #413

Closed
ReimarBauer opened this issue Jul 24, 2020 · 4 comments
Closed

problem with utc.localize #413

ReimarBauer opened this issue Jul 24, 2020 · 4 comments

Comments

@ReimarBauer
Copy link

ReimarBauer commented Jul 24, 2020

I used the conda package, so I'm not sure if there is a mistake or it is in the origin. Just asking ;)

just seen that in 1.24 I can't

from skyfield.api import utc


>>> help(utc.localize)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'datetime.timezone' object has no attribute 'localize'

in 1.23 this utc.localize can be used.

Help on method localize in module pytz:

localize(dt, is_dst=False) method of pytz.UTC instance
    Convert naive time to local time
(END)

At now I can only say I miss it, if it is gone how have I to replace it?

@brandon-rhodes
Copy link
Member

My guess is that you are using an advanced enough version of Python that it provides its own utc object, in which case Python uses that instead of the one from pytz. Real Python time zones do not have localize(), which is a trick that pytz used to support Daylight Savings Time on old Python versions.

Even when using the utc from pytz, there is never a need to call localize(), which is only necessary for time zones that switch to Daylight Savings Time. So the pytz method should never have been doing anything, but should always have simply returned the same datetime to you.

It does seem unfortunate that a method should appear or disappear based on Python version and based on whether pytz is installed or not! I think we should fix that: I think that Skyfield should stop using the pytz version of the time zone, so that it never has the non-standard (and not useful) localize() method. I think it should only ever use the official one, or its own. So I think I should replace:

if hasattr(dt_module, 'timezone'):
    utc = dt_module.timezone.utc
else:
    try:
        from pytz import utc
    except ImportError:
        # Lacking a full suite of timezones from pytz, we at least need a
        # time zone object for UTC.

        class UTC(dt_module.tzinfo):
            'UTC'
            ...

— with:

if hasattr(dt_module, 'timezone'):
    utc = dt_module.timezone.utc
else:
    # Lacking a full suite of timezones from pytz, we at least need a
    # time zone object for UTC.

    class UTC(dt_module.tzinfo):
        'UTC'
        ...

Does that make sense? I think it would have prevented you from writing code that called utc.localize(), and prevented your having to adjust your code now?

@ReimarBauer
Copy link
Author

Joern proposed a change on our site based on your idea
https://bitbucket.org/wxmetvis/mss/pull-requests/850/fixed-time-handling-for-skyfield/diff

This will go into our next minor release.

We should get off using the non standard localize method, may be a deprecation warning should help users to change their lines too.

@brandon-rhodes
Copy link
Member

… may be a deprecation warning should help users to change their lines too.

That’s an interesting idea! But a deprecation warning for folks upgrading to recent versions of Python 3 would require Skyfield to subclass the native Python utc object to provide the missing method — in which case there seems little point in Skyfield not just going with its own standalone copy of the class, if it has to create its own anyway?

Another approach would be to switch the logic: instead of having the default and fallback in the order datetime.timezone.utcpytz.utc → custom UTC class, the order could be pytz.utcdatetime.timezone.utc → custom UTC. That would have prevented the error that you saw, because even on recent Pythons it would use the pytz version if pytz was installed.

With so many options, let’s gather my thoughts:

  1. I like that Skyfield no longer imports pytz, because it means that Skyfield no longer offers a different object under the same name utc just because of the presence of absence of a third-party library. Previously, the same person moving the same script to the same Python version on a different machine might have seen different behavior (specifically: the localize() method going away!) because of a random third-party library pytz that their own script might not mention or use, but that happened to be present on the first machine but absent on the second.
  2. I also like not importing pytz because it’s a waste of memory and time to load all world timezones when Skyfield only needs utc. If users need to import pytz, that choice is theirs, but Skyfield should not cause it to be conditionally imported when it is not a declared dependency of Skyfield.
  3. Given those two beliefs, there are only two questions before us. One is whether Skyfield’s own class UTC should be given a localize() method. That would mean that everyone on old Python versions, without a built-in utc object, would see exactly the same behavior as before, whether pytz was loaded or not — a much more uniform situation.
  4. The second question is whether Skyfield should maybe just always use its own UTC class, so that Skyfield and its utc object remains exactly the same across all Python versions and machines.

So I wanted the answer here to be, “the documentation never illustrated utc.localize(), and it’s a needless complication, and folks can import utc from pytz if they want the method available, and hopefully mss is the only project that was broken and they have fixed it. So Skyfield can be simple and not offer the method any more!”

But I think that instead I should abandon my wild dreams of simplification, and pivot Skyfield to being as completely uniform for its users as possible: abandon the conditional imports and just always use its own built-in UTC class, to which I can easily add a localize() method for compatibility with the old pytz.utc that I used to import conditionally.

There is only one possible problem:

What if folks using utc had ever done if my_timezone is pytz.utc: to make decisions in their code, expecting the utc they imported from Skyfield to be the same one they saw in pytz? My guess is that I should ignore that possibility and choose complete uniformity instead. But I’ll maybe wait a day before deciding for sure!

@brandon-rhodes
Copy link
Member

In the commit referenced above I attempt to make the situation more uniform by always returning either an official UTC object or else Skyfield’s own replacement for it. Skyfield no longer imports an entire 3rd-party module simply to use a single few-line object from it! This also means that Skyfield utc will now always be a standard timezone object, never a quirky one with non-standard modules (that then disappear on systems with pytz not supported).

Hopefully this will make all code that relies on Skyfield entirely stable forever, with respect to how utc behaves. Thanks for letting me know about the discrepancy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants