BUG: Fix leap second table download triggered on import - #10125

Only used in that module.

If this works, I think it may be possible to do the same in time.formats, so that even importing the time module wouldn't download the LeapSecond table immediately...

Re: #9479 (comment) and the comments above -- Would be nice if @weaverba137 can take this PR for a spin and see where it crashes next.

I'd rather we fix a real use case than go hunting for all Time() calls.

I will try to run a test when I can. I'm pretty slammed today.

These might be in scope too:

astropy/coordinates/builtin_frames/lsr.py:J2000 = Time('J2000')
astropy/coordinates/builtin_frames/utils.py:EQUINOX_J2000 = Time('J2000', scale='tt')
astropy/coordinates/builtin_frames/utils.py:EQUINOX_B1950 = Time('B1950', scale='tt')
astropy/coordinates/builtin_frames/utils.py:DEFAULT_OBSTIME = Time('J2000', scale='tt'

Of course, those are a lot trickier, since an actual time instance is wanted. I'm starting to think the leap second loading itself should be moved away from __new__ to where it might actually get used (I don't quite recall why I didn't start with that...)

The one in LSR has a vague "for speed" comment but it is not used in the module. Maybe @adrn can explain why it was added in #6219 and whether it is still relevant.

Re: #10125 (comment)

Ah, @mhvk , if you can nip it in the bud, then maybe this PR won't be needed. I'll put this on hold.

The one in LSR has a vague "for speed" comment but it is not used in the module. Maybe @adrn can explain why it was added in #6219 and whether it is still relevant.

I don't remember. Try removing and see if tests still pass? 🤷‍♂

@pllim - not sure that I'll succeed - I don't remember much about my trials, but do remember it was an enormous hassle to avoid circular dependencies...

Ah, @mhvk , if you can nip it in the bud, then maybe this PR won't be needed. I'll put this on hold.

What about going gradually, and get this in now and instead of holding up the PR, just revert if/when the other approach works out and gets in?

I'm fine with replacing the jds here with actual values (no need for the lazy loading), but think it is unlikely it actually solves the problem, so the changelog entry may need to be adjusted.

replacing the jds here with actual values

That works for me. Will the values ever change due to leap seconds? Maybe we can even include it in constants and be done with it. I see a few flavors of Time(epoch) calls across the package.

It is too bad we cannot use PEP 562 yet. The ones in builtin_frames/utils.py are tricky.

Also, I don't think this is worth it as it would be hard to maintain going forward: https://www.themetabytes.com/2018/06/19/lazy-loading-modules-in-python/

This is looking pretty hacky... 😬 But at least tests passed with this patch for me, on Windows no less.

Great, now at least it is clear where the problems lie. Will try to find some time to see if I can instead move the download further down.

Probably the coffee still isn't working, but I'm confused about this approach and I don't see how this is preventing making new Time objects at import time, which if I understand can trigger a download. I made a file lazy.py:

def func(arg):
    print(f'func called: {arg}!')
    return arg

class TimeAttribute:
    def __init__(self, arg):
        print(f'In TimeAttribute: {arg}')

class Blah:
    attr = TimeAttribute(arg=func('first'))

    def meth(arg=func('second')):
        pass

Then

>>> import lazy
func called: first!
In TimeAttribute: first
func called: second!

@taldcroft , I didn't know Python actually runs the nested code on import. 🤔 I'll have to double check.

UPDATE: You are right! 😱 I gotta think about this some more.

I think the trick is to make them class properties. If I changed @taldcroft 's lazy.py to the following:

class Blah:
    @property
    def attr(self):
        return TimeAttribute(arg=func('first'))

    def meth(arg=func('second')):
        pass

I only get:

>>> import lazy
func called: second!

But this doesn't solve the module level caching.

I do hope that @mhvk can find a lower-level solution. It does not seem like there should be any requirement to access the latest leap seconds just to create a Time object. It's only for converting.

A lower-level solution also helps other projects that may be creating Time objects on import. It seems that importing any part of astropy should never trigger or rely on network access.

Well, I am stuck anyway. I went down the rabbit hole of frame attributes. Changing them to class property broke everything. There is some magic going on with class level attributes and there is no easy way to lazy load them. 😢

I am closing this PR. I think this is a dead end. Sorry for the noise!

@pllim - thanks for the valiant attempt! I agree that there's a need for a lower-level solution, and it should be do-able (though it is not like I didn't try initially; am hoping that a fresh look will help...)