TST: Minimize calls to remote sites.json for testing by pllim · Pull Request #10304 · astropy/astropy

pllim · 2020-05-05T00:12:22Z

Description

This pull request is to ~~expose force_builtin option in EarthLocation when looking up sites.json to~~ minimize calls to sites.json that resides on the remote data server.

The portion of the tests that require IERS leap seconds table download still are marked as remote data.

Fixes #10298

~~Address part of #10255~~ (Nope, removing call to of_site did not change anything for this one.)

TODO

~~Add change log when new section is available.~~
Check remote data job. There will still be failures if astropy data server blacklisted the CI but not as many.
Squash commits?

bsipocz · 2020-05-05T00:36:49Z

I'm 👎 for expanding sites.json. Why these obs, why not others. What to do with update PRs to this builtin file. This file started its life in astropy core, but exactly for these reasons got moved out.

bsipocz · 2020-05-05T00:39:33Z

+
+    >>> from astropy.coordinates import EarthLocation, SkyCoord
+    >>> loc = EarthLocation.of_site('Royal Observatory Greenwich', force_builtin=True)
+    >>> keck = EarthLocation.of_site('Keck', force_builtin=True)


instead of relying on of_site() here, you could use it's coordinates hard wired (or append the local sites.json on the fly if Keck is missing from it during doctest)

Oh yeah, I guess hardcoding lon/lat at testsetup would be even better.

Hmmm! This means maybe I don't need to expose force_builtin after all...

yes, I think if that's an acceptable hack we can avoid both force_builtin and the extension of the files. It's a bit of a cheat as the users will see different code than what's tested, but I suppose this one instance it's forgivable.

mhvk

I like the use of test setup, but am not that keen on the added keyword argument and wonder whether cannot be cleverer still. Inline a suggestion for the sites (untested...).

For the sky coordinates, since caching was just implemented for them, would it be possible to check the URL that gets created by name_resolve.get_icrs_coordinates and simply insert that in the cache as part of the test setup? The advantage is that there will not be any need for doctest skip (and the doctest is thus much closer to reality)

Actually, a related alternative would be to populate the site registry with 'Keck', etc., inside the test setup, so that you can leave the built-in file alone.

Anyway, not sure if it is worth the extra work - though getting rid of these remote_data tests is definitely nice!

mhvk · 2020-05-05T00:38:23Z

+.. testsetup::
+
+    >>> from astropy.coordinates import EarthLocation, SkyCoord
+    >>> loc = EarthLocation.of_site('Royal Observatory Greenwich', force_builtin=True)


I like this idea of a test setup, but couldn't for the locations at least it be done simpler with:

from astropy.coordinates import EarthLocation EarthLocation._get_site_registry(force_builtin=True)

Re: caching -- I don't think #9162 affects of_site?

EarthLocation._get_site_registry(force_builtin=True) is problematic because I need to use force_download to undo it, which defeats the purpose of not hitting data server.

I think in the other thread you're converging on hard-coding, which would be OK, though how about even more cheekily setting EarthLocation._site_registry = None at the end of tests? Probably overkill...

p.s. Yes, caching is only for SkyCoord. But only useful if the sites works too. My sense: go with @bsipocz suggestion and have an issue about perhaps prepopulating the cache instead.

I don't know how to pre-populate cache on CI. I was under the impression that we need to store the tarball of the cache somewhere and then have the CI pull it but I have no idea how to set up the tarball in the first place.

Yes, I am in the works to hardcode stuff. Will push a commit soon.

pllim · 2020-05-05T00:56:32Z

So it seems like I shouldn't been exposing force_builtin and changing sites.json? Okay, I'll ponder a bit more. Thanks!

pllim · 2020-05-05T02:17:57Z

-    >>> target.radial_velocity_correction(obstime=obstime, location=keck).to('km/s')  # doctest: +REMOTE_DATA +FLOAT_CMP
+    >>> target = SkyCoord.from_name('M31')  # doctest: +SKIP
+    >>> keck = EarthLocation.of_site('Keck')  # doctest: +SKIP
+    >>> target.radial_velocity_correction(obstime=obstime, location=keck).to('km/s')  # doctest: +FLOAT_CMP  +REMOTE_DATA


If we can figure out the IERS stuff, we can remove +REMOTE_DATA here too but that is a battle for another day.

pllim · 2020-05-05T02:20:14Z

-    ...                         observer=alma, target=ttau)  # doctest: +IGNORE_WARNINGS +REMOTE_DATA
-    >>> sc_ttau  # doctest: +REMOTE_DATA +FLOAT_CMP
+    ...                         observer=alma, target=ttau)  # doctest: +IGNORE_WARNINGS
+    >>> sc_ttau  # doctest: +FLOAT_CMP +REMOTE_DATA


Like the other example above, +REMOTE_DATA here is due to IERS leap seconds table, not of_site anymore.

I wonder if we cannot set tests to execute with an offline or auto_download=False config - really, the bundled IERS is good enough for these tests so it should not download anything. But obviously Time cannot know this... Anyway, separate issue!

bsipocz · 2020-05-05T02:21:24Z

Now, that this is rescoped to test refactoring only, I think it can even go back to 4.0.2. What do you think?

pllim · 2020-05-05T02:26:41Z

@bsipocz , I moved the milestone to 4.1.1 because half the stuff I touched here were added for 4.1.

Satellites example need remote data after all.

pllim · 2020-05-05T03:33:02Z

Weird, the coordinates failures are completely gone in the remote data job. I was expecting at least one to still fail. I can't tell if this patch is too successful or if GitHub stopped blacklisting CI.

The wcsapi failure is already handled in a different PR.

StuartLittlefair

Looks good to me; a very tedious job of going through and coding everything by hand so thanks!

astrofrog · 2020-05-05T10:16:36Z

+        tree = dict(location=rog)
+        assert_roundtrip_tree(tree, tmpdir)
+    finally:
+        EarthLocation._site_registry = orig_sites


Maybe we should just make this a context manager as it seems like a useful thing to have?

Possibly just as a setup_class, teardown_class, though for now this would seem good enough.

I briefly thought about turning it into a test class but decided that the line count would blow up and maybe not worth it. 🤷

I thought I opened an issue a long time ago to request changes to API allowing user to specify their own local sites.json but I cannot find that anymore. I think changing the API to context manager is out of scope here.

mhvk

I'd be quite happy to get this in - only two small comments that may be good to address.

mhvk · 2020-05-05T13:44:21Z

+        tree = dict(location=rog)
+        assert_roundtrip_tree(tree, tmpdir)
+    finally:
+        EarthLocation._site_registry = orig_sites


Possibly just as a setup_class, teardown_class, though for now this would seem good enough.

mhvk · 2020-05-05T13:48:49Z

-    ...                         observer=alma, target=ttau)  # doctest: +IGNORE_WARNINGS +REMOTE_DATA
-    >>> sc_ttau  # doctest: +REMOTE_DATA +FLOAT_CMP
+    ...                         observer=alma, target=ttau)  # doctest: +IGNORE_WARNINGS
+    >>> sc_ttau  # doctest: +FLOAT_CMP +REMOTE_DATA


I wonder if we cannot set tests to execute with an offline or auto_download=False config - really, the bundled IERS is good enough for these tests so it should not download anything. But obviously Time cannot know this... Anyway, separate issue!

mhvk · 2020-05-05T17:37:51Z

@pllim - if it would help clarify the other PRs, go ahead and merge, I think this is a good solution for now!

pllim · 2020-05-05T17:54:29Z

Hmm, I'll leave the merging to others. I try not to merge my own PR unless it is trivial. 😬

TST: Minimize calls to remote sites.json for testing

TST: Minimize calls to remote sites.json

a17495b

pllim added this to the v4.2 milestone May 5, 2020

pllim requested review from StuartLittlefair, adrn, astrofrog, bsipocz, eteq and mhvk May 5, 2020 00:12

pllim mentioned this pull request May 5, 2020

DOC: Un-skip coordinate example for doctest #10305

Merged

1 task done

This comment has been minimized.

Sign in to view

bsipocz reviewed May 5, 2020

View reviewed changes

mhvk reviewed May 5, 2020

View reviewed changes

pllim added the coordinates label May 5, 2020

pllim commented May 5, 2020

View reviewed changes

bsipocz added the no-changelog-entry-needed label May 5, 2020

bsipocz modified the milestones: v4.2, v4.0.2 May 5, 2020

pllim commented May 5, 2020

View reviewed changes

Comment thread docs/coordinates/index.rst Resolved

pllim changed the title ~~Expose force_builtin to minimize calls to remote sites.json for testing~~ TST: Minimize calls to remote sites.json for testing May 5, 2020

pllim added the testing label May 5, 2020

pllim modified the milestones: v4.0.2, v4.1.1 May 5, 2020

This comment has been minimized.

Sign in to view

Undo sites.json changes. Remove force_builtin exposure.

0bc0f4e

Satellites example need remote data after all.

pllim force-pushed the local-sites-json branch from bcca834 to 0bc0f4e Compare May 5, 2020 02:33

StuartLittlefair approved these changes May 5, 2020

View reviewed changes

astrofrog reviewed May 5, 2020

View reviewed changes

mhvk reviewed May 5, 2020

View reviewed changes

This was referenced May 5, 2020

TST: Disable IERS download for testing #10311

Merged

EXP: EarthLocation context manager to force builtin sites #10313

Closed

mhvk merged commit 220ee75 into astropy:master May 5, 2020

pllim deleted the local-sites-json branch May 5, 2020 18:03

pllim mentioned this pull request May 5, 2020

MNT: Move earth.py circular imports into relevant method #10314

Merged

astrofrog pushed a commit that referenced this pull request May 19, 2020

Merge pull request #10304 from pllim/local-sites-json

c6ee2d7

TST: Minimize calls to remote sites.json for testing

astrofrog modified the milestones: v4.1.1, v4.1 May 22, 2020

pllim mentioned this pull request Jul 24, 2020

fix earth location info #10592

Merged

Conversation

pllim commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

bsipocz commented May 5, 2020

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented May 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented May 5, 2020

Uh oh!

Uh oh!

pllim commented May 5, 2020

Uh oh!

This comment has been minimized.

pllim commented May 5, 2020

Uh oh!

StuartLittlefair left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 5, 2020

Uh oh!

pllim commented May 5, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pllim commented May 5, 2020 •

edited

Loading