BUG: Skip good_enough when auto_max_age is None by pllim · Pull Request #12713 · astropy/astropy

pllim · 2022-01-10T23:00:47Z

Description

This pull request is to lessen unnecessary warnings when iers.conf.auto_max_age is None.

TODO

See if others think this is the correct solution.
Add test(s).
Add change log.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

github-actions · 2022-01-10T23:01:31Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

mhvk

Thanks for looking into this! A suggestion for a slightly different approach. Obviously, this would also need a test...

Co-authored-by: Marten van Kerkwijk <[email protected]>

pllim · 2022-01-11T14:25:50Z

These tests failed for me locally but I cannot tell if my local data files are outdated or if it is real. I guess I'll wait for CI to tell me.

pytest astropy/utils/iers/tests/test_leap_second.py --remote-data

self = <astropy.utils.iers.tests.test_leap_second.TestAutoOpenExplicitLists object at ...>
tmpdir = local('/tmp/.../pytest-16/test_fake_expired_file0')

    def test_fake_expired_file(self, tmpdir):
        fake_file1 = make_fake_file('28 June 2010', tmpdir)
        fake_file2 = make_fake_file('27 June 2012', tmpdir)
        # Between these and the built-in one, the built-in file is best.
        ls = iers.LeapSeconds.auto_open([fake_file1, fake_file2,
                                         iers.IERS_LEAP_SECOND_FILE])
        assert ls.meta['data_url'] == iers.IERS_LEAP_SECOND_FILE

        # But if we remove the built-in one, the least expired one will be
        # used and we get a warning that it is stale.
        with pytest.warns(iers.IERSStaleWarning):
            ls2 = iers.LeapSeconds.auto_open([fake_file1, fake_file2])
        assert ls2.meta['data_url'] == fake_file2
        assert ls2.expires == Time('2012-06-27', scale='tai')

        # Use the fake files to make sure auto_max_age is safe.
        # Should have no warning here.
        with iers.conf.set_temp('auto_max_age', None):
            ls3 = iers.LeapSeconds.auto_open([fake_file1,
                                              iers.IERS_LEAP_SECOND_FILE])
>       assert ls3.meta['data_url'] == fake_file1
E       AssertionError: assert '/home/.../Do...ap_Second.dat' == '/tmp/pytest-...p_seconds.dat'
E         - /tmp/.../pytest-16/test_fake_expired_file0/fake_leap_seconds.dat
E         + /home/.../astropy/utils/iers/data/Leap_Second.dat

astropy/utils/iers/tests/test_leap_second.py:193: AssertionError
_________________________________________ TestDefaultAutoOpen.test_erfa_found __________________________________________
self = <astropy.utils.iers.tests.test_leap_second.TestDefaultAutoOpen object at ...>

    def test_erfa_found(self):
        # Set huge maximum age such that whatever ERFA has is OK.
        # Since it is checked first, it should thus be found.
        with iers.conf.set_temp('auto_max_age', 100000):
            ls = iers.LeapSeconds.open()
>       assert ls.meta['data_url'] == 'erfa'
E       AssertionError: assert 'https://hpie...ap_Second.dat' == 'erfa'
E         - erfa
E         + https://hpiers.obspm.fr/iers/bul/bulc/Leap_Second.dat

astropy/utils/iers/tests/test_leap_second.py:243: AssertionError
________________________________________ TestDefaultAutoOpen.test_builtin_found ________________________________________
self = <astropy.utils.iers.tests.test_leap_second.TestDefaultAutoOpen object at ...>

    def test_builtin_found(self):
        # Set huge maximum age such that built-in file is always OK.
        # If we remove 'erfa', it should thus be found.
        self.remove_auto_open_files('erfa')
        with iers.conf.set_temp('auto_max_age', 100000):
            ls = iers.LeapSeconds.open()
>       assert ls.meta['data_url'] == iers.IERS_LEAP_SECOND_FILE
E       AssertionError: assert 'https://hpie...ap_Second.dat' == '/home/.../Do...ap_Second.dat'
E         - /home/.../astropy/utils/iers/data/Leap_Second.dat
E         + https://hpiers.obspm.fr/iers/bul/bulc/Leap_Second.dat

astropy/utils/iers/tests/test_leap_second.py:251: AssertionError
______________________________________ TestDefaultAutoOpen.test_fake_future_file _______________________________________
self = <astropy.utils.iers.tests.test_leap_second.TestDefaultAutoOpen object at ...>
tmpdir = local('/tmp/.../pytest-16/test_fake_future_file0')

    def test_fake_future_file(self, tmpdir):
        fake_file = make_fake_file('28 June 2345', tmpdir)
        # Try as system file for auto_open, setting auto_max_age such
        # that any ERFA or system files are guaranteed to be expired.
        with iers.conf.set_temp('auto_max_age', -100000), \
                iers.conf.set_temp('system_leap_second_file', fake_file):
            ls = iers.LeapSeconds.open()
>       assert ls.expires == Time('2345-06-28', scale='tai')
E       AssertionError: assert <Time object: scale='tai' format='iso' value=2022-06-28 00:00:00.000>
== <Time object: scale='tai' format='iso' value=2345-06-28 00:00:00.000>
E        +  where <Time object: scale='tai' format='iso' value=2022-06-28 00:00:00.000>
= <LeapSeconds length=42>\n
 year month tai_utc \n
int32 int32 float64 \n
----- ----- --------\n
 1960     1 1.417818\n
 1961     ...    1     32.0\n
 2006     1     33.0\n
 2009     1     34.0\n
 2012     7     35.0\n
 2015     7     36.0\n
 2017     1     37.0.expires
E        +  and   <Time object: scale='tai' format='iso' value=2345-06-28 00:00:00.000>
= Time('2345-06-28', scale='tai')

astropy/utils/iers/tests/test_leap_second.py:272: AssertionError

pllim · 2022-01-11T14:44:07Z

@mhvk , I don't grok why two tests fail with the change you suggested. Any advise on how to proceed? Thanks!

mhvk · 2022-01-11T21:11:51Z

I think I found the mistake. If the first test still fails, it is fine to adjust it -- I think it is an improvement to use the best of the available leap second tables rather than just the first even for auto_max_age = None.

pllim · 2022-01-11T22:49:04Z

Something still isn't quite right...

        # Use the fake files to make sure auto_max_age is safe.
        # Should have no warning here.
        with iers.conf.set_temp('auto_max_age', None):
            ls3 = iers.LeapSeconds.auto_open([fake_file1,
                                              iers.IERS_LEAP_SECOND_FILE])
>       assert ls3.meta['data_url'] == fake_file1
E       AssertionError: assert '/home/runner...ap_Second.dat' == '/tmp/pytest-...p_seconds.dat'
E         - /tmp/pytest-of-runner/pytest-0/test_fake_expired_file0/fake_leap_seconds.dat
E         + /home/runner/work/astropy/astropy/.tox/py310-test-cov/lib/python3.10/site-packages/astropy/utils/iers/data/Leap_Second.dat

../../.tox/py310-test-cov/lib/python3.10/site-packages/astropy/utils/iers/tests/test_leap_second.py:193: AssertionError

pllim · 2022-01-11T22:52:59Z

I don't understand this test... It expects the algorithm to actually pick the stale one, it seems like.

        # Between these and the built-in one, the built-in file is best.
        ls = iers.LeapSeconds.auto_open([fake_file1, fake_file2,
                                         iers.IERS_LEAP_SECOND_FILE])
        assert ls.meta['data_url'] == iers.IERS_LEAP_SECOND_FILE

        # ...

        # Use the fake files to make sure auto_max_age is safe.
        # Should have no warning here.
        with iers.conf.set_temp('auto_max_age', None):
            ls3 = iers.LeapSeconds.auto_open([fake_file1,
                                              iers.IERS_LEAP_SECOND_FILE])
        assert ls3.meta['data_url'] == fake_file1

mhvk · 2022-01-12T01:04:48Z

I think that test has simple changed because the result is now better. I've pushed the change (and added a bit to it for good measure).

mhvk

I think this is all OK now!

pllim · 2022-01-12T03:35:55Z

Thanks, @mhvk ! I'll wait a bit to see if this actually helps with @weaverba137 's use case before merge.

weaverba137 · 2022-01-13T16:57:39Z

OK, while this does eliminate the unnecessary warnings, I am not convinced that @mhvk's approach is correct. Remember, we are working in an HPC environment and must prevent any downloads. We do not want the value of good_enough to revert to default when auto_max_age = None. We want good_enough to have a value that will always prevent a download.

Can you walk me through the logic of this latest approach and demonstrate how this satisfies this requirement?

weaverba137

As mentioned previously, I think this comes down to knowing exactly how good_enough works in this method.

mhvk · 2022-01-13T21:13:21Z

@weaverba137 - conf.auto_download ensures that no file is ever downloaded (I think, do correct me if I'm wrong!! see line 1004). This PR was really just to get rid of the warning that the file would be stale. But when I looked at the code, I realized that among files already in the cache or otherwise on the computer, the old code would just pick the first rather than the best, which seemed a mistake, so I corrected it to pick the first that clearly is good enough or the best available.

weaverba137 · 2022-01-13T21:34:55Z

Aha, that makes more sense. Let me take one more look at the full code in that light.

weaverba137 · 2022-01-19T17:05:48Z

@mhvk, I am satisfied that the changes here will not inadvertently cause a download. However, I don't see the change you made to pick the "best" file over the "first" file. Is that change in this PR?

mhvk · 2022-01-19T17:12:16Z

@weaverba137 - the change in the time is what makes it take the "best" file: before, since good_enough was way in the past, it just picked the first file, but now it will keep looking for the best one unless it finds one that we know is not expired.

weaverba137

@mhvk, thank you for your clarifications. I think this is ready to go.

pllim

Thanks for the reviews! @mhvk , do you want me to rebase/squash? If not, feel free to merge.

mhvk · 2022-01-19T17:27:44Z

Let's just get it in! Thanks, all! Slowly but surely we are making sure astropy rocks on every platform!

… None

…713-on-v5.0.x Backport PR #12713 on branch v5.0.x (BUG: Skip good_enough when auto_max_age is None)

… change) The 'Time 5167445918.181001' line in the issue log is the observation epoch in seconds since the MJD epoch (1858-11-17), i.e. the standard MS TIME convention, not a corrupt timestamp. The accompanying numpy overflow/invalid RuntimeWarnings were produced by astropy <= 5.0.0: with iers.conf.auto_max_age = None, LeapSeconds.auto_open computed TimeDelta(180 - np.finfo(float).max) which overflowed the split() helper in astropy.time.utils. Fixed upstream in astropy 5.0.1 (astropy/astropy#12713); astropy >= 7.1 is required by this package.

pllim added Bug backport-v5.0.x labels Jan 10, 2022

pllim added this to the v5.0.1 milestone Jan 10, 2022

pllim mentioned this pull request Jan 10, 2022

iers.conf.auto_max_age = None produces distracting warnings #12708

Closed

github-actions Bot added the utils.iers label Jan 10, 2022

mhvk reviewed Jan 11, 2022

View reviewed changes

Comment thread astropy/utils/iers/iers.py Resolved

Comment thread astropy/utils/iers/iers.py Resolved

pllim and others added 2 commits January 11, 2022 09:03

BUG: Handle good_enough properly when auto_max_age is None

8107eb1

Co-authored-by: Marten van Kerkwijk <[email protected]>

Update test and add change log

e5015fc

pllim force-pushed the a-few-good-checks branch from 8b9a6aa to e5015fc Compare January 11, 2022 14:19

mhvk reviewed Jan 11, 2022

View reviewed changes

Comment thread astropy/utils/iers/iers.py Resolved

BUG: Do not subtract 180 twice from offset

d9472f9

Adjust test for better behaviour for auto_max_age=None

2f8325f

mhvk marked this pull request as ready for review January 12, 2022 01:05

mhvk approved these changes Jan 12, 2022

View reviewed changes

pllim added the Ready-for-final-review label Jan 12, 2022

pllim requested a review from weaverba137 January 12, 2022 03:35

weaverba137 reviewed Jan 13, 2022

View reviewed changes

Comment thread astropy/utils/iers/iers.py Resolved

weaverba137 approved these changes Jan 19, 2022

View reviewed changes

pllim commented Jan 19, 2022

View reviewed changes

Comment thread astropy/utils/iers/iers.py Resolved

mhvk merged commit 7ebf816 into astropy:main Jan 19, 2022

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Jan 19, 2022

Backport PR astropy#12713: BUG: Skip good_enough when auto_max_age is…

d025a15

… None

meeseeksmachine mentioned this pull request Jan 19, 2022

Backport PR #12713 on branch v5.0.x (BUG: Skip good_enough when auto_max_age is None) #12750

Merged

pllim deleted the a-few-good-checks branch January 19, 2022 17:31

pllim added a commit that referenced this pull request Jan 19, 2022

Merge pull request #12750 from meeseeksmachine/auto-backport-of-pr-12…

10eb5cb

…713-on-v5.0.x Backport PR #12713 on branch v5.0.x (BUG: Skip good_enough when auto_max_age is None)

pllim mentioned this pull request Mar 28, 2022

datacenter.iers.org SSL: CERTIFICATE_VERIFY_FAILED (Column year failed to convert: invalid literal for int() with base 10: '<!') #12998

Closed

tmolteno mentioned this pull request Sep 28, 2026

RuntimeWarning: overflow encountered in double_scalars tart-telescope/tart2ms#6

Closed

Conversation

pllim commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Checklist for package maintainer(s)

Uh oh!

github-actions Bot commented Jan 10, 2022

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pllim commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Jan 11, 2022

Uh oh!

Uh oh!

mhvk commented Jan 11, 2022

Uh oh!

pllim commented Jan 11, 2022

Uh oh!

pllim commented Jan 11, 2022

Uh oh!

mhvk commented Jan 12, 2022

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Jan 12, 2022

Uh oh!

weaverba137 commented Jan 13, 2022

Uh oh!

weaverba137 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk commented Jan 13, 2022

Uh oh!

weaverba137 commented Jan 13, 2022

Uh oh!

weaverba137 commented Jan 19, 2022

Uh oh!

mhvk commented Jan 19, 2022

Uh oh!

weaverba137 left a comment

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk commented Jan 19, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pllim commented Jan 10, 2022 •

edited

Loading

pllim commented Jan 11, 2022 •

edited

Loading