Remove unnecessary remote_data in (mostly) coordinates. #10474

mhvk · 2020-06-09T02:10:54Z

In #10311, @pllim made the tests run by default using no download for IERS. This means that a lot of tests can now be run (again) without remote_data. It turned out, though, that the tests of astropy.config messed up the configuration setting. This PR fixes that, and removes the now no longer needed remote_data (I hope I didn't miss any... do we need a test that remote_data actually tries to access the internet???)

Note that the first commit is a missed item in #10337, which I include here since without the work here, the fix would not get tested (which was why it passed by in the first place). As such: fixes #10471

EDIT: Close #10469

mhvk · 2020-06-09T12:00:38Z

Hmm, the double test fails too, so there must be another place where the configuration gets changed...

And still the mysterious error from the docs...

pllim · 2020-06-09T17:18:54Z

Docs error is unrelated but the double one definitely is. Turns out the "double run" command is very different from the rest:

python -c 'import sys; from astropy import test; test(); sys.exit(test())'

I don't know why yet, but I have a feeling that it is not completely equivalent to running pytest in the source dir. For example, one documented behavior difference is at #8153 , where Dan even called for getting rid of the test runner entirely.

pllim · 2020-06-09T17:22:25Z

@mhvk , what if we forcefully do a astropy.utils.iers.conf.auto_download = False between Run 1 and Run 2 on this line?

astropy/tox.ini

Line 90 in c17ce8b

    
           double: python -c 'import sys; from astropy import test; test(); sys.exit(test())'

mhvk · 2020-06-09T17:54:52Z

@pllim - indeed, that may be a hack to get things to pass, but, really, astropy.test() should not mess with the configuration! I'll see if I can narrow it down a bit.

Here, it should not assume that the configuration is all stored in a file, but include any configuration done programmatically. This is particularly useful to keep utils.iers.conf.auto_download to any setting done in conftest.py.

mhvk · 2020-06-09T20:31:49Z

OK, I think it was a combination of set_temp_config being rather liberal in clearing earlier configuration (I guess assuming it was all on disk, so would just be re-read) and my earlier fix for the configuration tests not being quite good enough. It should now work... (and I rebased on master to pick up the fix for the docs, so ideally all will be green 🤞)

pllim · 2020-06-09T21:01:13Z

Hmm... Looks like some of the tests might be relying on the old behavior... The diff makes no sense at a glance.

mhvk · 2020-06-09T21:05:35Z

And then I clearly didn't bother to check the normal testing still worked!

mhvk · 2020-06-09T21:34:27Z

Actually, very weird. I cannot reproduce the travis failures. What has io.votable to do with anything!?!?

mhvk · 2020-06-10T01:07:01Z

So, that was funny: it turned out I couldn't reproduce the travis failures because locally I have a normal, all-default astropy config file. When I replaced that with the temporary one created in .travis.yml, I could reproduce the failures. Since it actually seems better to not have to set up a fake config file, I just removed it from .travis.yml. But, @astrofrog, since you added the line originally, if you have a chance to have a look whether this all makes sense, that would be very welcome

mhvk · 2020-06-10T01:21:00Z

.travis.yml

     # Check CC variable
    - echo "CC="$CC

-    # Write configuration items to standard location to make sure they are


I'm not completely sure why, but with the other changes that ensure the configuration never needs to be reloaded, this writing of a configuration file now causes failures, so I removed it.

This was to fix a problem with parallel tests. :(

Added in #8727 to address #8154 .

What about... Instead of removing this, we add a line to the astropy.cfg here to set auto_download = False?

I wondered about it, but it seemed to me that your solution of a change in astropy/conftest.cfg was really more elegant, and works for everybody.

Do you think we still need this at all? It doesn't seem to give failures, and it is hard to see how we can mess up the configuration on travis.

Oh, I didn't realize I made it unnecessary. I'll have to remember what I did and get back to you.

I am curious... You said this command made the tests fail in Travis CI. However, the same command was also added in #8727 to CircleCI and that didn't affect your PR. How could that be? 🤔

I'm totally confused! But with this still in place (one but last commit), both Initial tests on travis failed (see https://travis-ci.org/github/astropy/astropy/builds/696611940). I'm actually not quite sure why they failed, but trying to determine why the tests passed locally I realized that the only difference was the config file (and locally tests also fail if I use the now-removed lines as config).

Given that it still exists in CircleCI for "32 bit and parallel" tests and things pass there, probably okay to remove here. If @astrofrog disagrees when he is available again, we can always do a follow-up PR.

mhvk · 2020-06-10T01:22:09Z

astropy/config/paths.py

        from .configuration import _cfgobjs
-
-        path = super().__enter__()
+        self._cfgobjs_copy = _cfgobjs.copy()


The changes here are to keep the configuration instead of deleting it at the end so that it gets reloaded from the configuration file. This is because some config items can have been set programmatically (in particular, utils.iers.conf.auto_download)

Is the "reset all the cached config objects" comment on L243 above still accurate? If not, should remove/modify it.

And this is a subtle API change for set_temp_config, right? I wonder if it warrants a change log.

Yes, I guess you are right. I now updated the comment and added a changelog entry.

mhvk · 2020-06-10T01:22:51Z

astropy/config/tests/test_configs.py

+OLD_CONFIG = {}
+
+
+def setup_module():


Arguably, all the test in this file should be written such that they do not change the configuration, but this was easier...

mhvk · 2020-06-10T01:23:18Z

astropy/config/tests/test_configs.py


    assert not os.path.exists(temp_config_dir)
+    # Check that we have returned to our old configuration.
+    assert configuration._cfgobjs == OLD_CONFIG


This verifies that set_temp_config does not destroy the old configuration.

mhvk · 2020-06-10T01:24:10Z

I added some comments to make review easier. Beyond the first few files, everything is just removal of remote_data.

pllim

Yes, I agree we you that we might not need to write out the astropy.cfg anymore in CI. Let's just leave it as you implemented and we're early in the dev cycle for 4.2 anyway, so hopefully anything we overlooked would be flushed out before its release.

Hmm, I have no idea why these failed in the remote data job. Are they related?

FAILED ../../.tox/py37-test-devdeps/lib/python3.7/site-packages/astropy/coordinates/tests/test_intermediate_transformations.py::test_earth_orientation_table
FAILED ../../.tox/py37-test-devdeps/lib/python3.7/site-packages/astropy/time/tests/test_basic.py::test_scale_conversion

pllim · 2020-06-12T22:15:26Z

astropy/config/tests/test_configs.py

+
+def setup_module():
+    OLD_CONFIG.clear()
+    OLD_CONFIG.update(configuration._cfgobjs)


The need to call "hidden" stuff might indicate that we should expose the API properly? But that is discussion for another day.

Yes, or one should have a context manager option that ensures the configuration gets returned in its original state. But I'd indeed like to keep that for another PR.

pllim · 2020-06-12T22:20:27Z

.travis.yml

     # Check CC variable
    - echo "CC="$CC

-    # Write configuration items to standard location to make sure they are


I am curious... You said this command made the tests fail in Travis CI. However, the same command was also added in #8727 to CircleCI and that didn't affect your PR. How could that be? 🤔

mhvk · 2020-06-19T00:44:47Z

@pllim - thanks for noticing the remote data failures - they are indeed related, since those two specifically test that an up-to-date IERS file is used. They have remote_data, but now I should add that we do allow downloads there.

mhvk · 2020-06-19T01:16:05Z

astropy/time/tests/test_basic.py

-def test_scale_conversion():
+def test_scale_conversion(monkeypatch):
+    monkeypatch.setattr('astropy.utils.iers.conf.auto_download', True)
+    with iers.earth_orientation_table.set(iers.IERS_B.open()):


Note: I was very surprised about the single line in the test as written so rewrite to make it similar to the original purpose of this test (which was removed because it seemed tricky to do; indeed, resulting in a discussion that was not actually resolved... https://github.com/astropy/astropy/pull/6606/files#r140902373).

Hmm, that new test for exception with bundled table doesn't need to be marked as remote data, right? Can it be in its own function? Or does it still somehow needs internet?

Looking at this again, there really is no point in testing the exception, so I removed it again. Instead, I now added a comment so it is clearer why the test exists.

mhvk · 2020-06-19T01:19:49Z

@pllim - you may want to look just at the last commit, which will hopefully fix the remote-data failures (though I fear some numpy-dev stuff may be coming...).

pllim · 2020-06-19T01:25:53Z

astropy/coordinates/tests/test_intermediate_transformations.py


    Use the here and now to be sure we get a difference.
    """
+    monkeypatch.setattr('astropy.utils.iers.conf.auto_download', True)


Usually not a fan of monkeypatching but it does seem less clunky than the try ... finally ... implementation I did in #10311, so 👍

There is also with iers.iers.Conf.auto_download.set_temp(True) but I felt this was a bit more natural. I actually rather liked your setup_class and teardown_class, but it seemed too much effort for a single function.

mhvk · 2020-06-19T11:26:26Z

Darn, getting that thing that even using tox apparently doesn't always solve: an error here on dev that I cannot reproduce locally.

mhvk · 2020-06-19T13:52:00Z

OK, by removing the catch_warnings and relying on pytest to error, the tests now pass. @pllim - sorry that this picks one piece of your PR!

Anyway, should now be ready!

pllim

A few minor comments. I'll approve after they are resolved. We're really close. Thank you!

pllim · 2020-06-19T21:00:15Z

.travis.yml

     # Check CC variable
    - echo "CC="$CC

-    # Write configuration items to standard location to make sure they are


Given that it still exists in CircleCI for "32 bit and parallel" tests and things pass there, probably okay to remove here. If @astrofrog disagrees when he is available again, we can always do a follow-up PR.

pllim · 2020-06-19T21:03:38Z

astropy/config/paths.py

        from .configuration import _cfgobjs
-
-        path = super().__enter__()
+        self._cfgobjs_copy = _cfgobjs.copy()


Is the "reset all the cached config objects" comment on L243 above still accurate? If not, should remove/modify it.

And this is a subtle API change for set_temp_config, right? I wonder if it warrants a change log.

pllim · 2020-06-19T21:11:56Z

astropy/coordinates/tests/test_intermediate_transformations.py


    with iers.earth_orientation_table.set(iers.IERS_B.open()):
-        with catch_warnings() as w:
+        with pytest.warns(AstropyWarning, match='after IERS data'):


I must be going crazy. Pretty sure I needed some .* in the regex when I worked on it, but tests passed here without. 🤷

pllim · 2020-06-19T21:12:20Z

astropy/coordinates/tests/test_intermediate_transformations.py

-        with catch_warnings() as w:
+        with pytest.warns(AstropyWarning, match='after IERS data'):
            altaz_b = sc.transform_to(altaz)
-        assert len(w) == 1


So you don't want to check to see if this is the only warning emitted anymore?

I think it is fine to just test that we get the right warning. If there are others, pytest would presumably fail.

Not really. Consider this example:

import warnings import pytest def myfunc(): warnings.warn('Catch this', UserWarning) warnings.warn('Nope', UserWarning) # Even if you change the warning type, same result below def test1(): with pytest.warns(UserWarning, match='Catch this'): myfunc() # passes def test2(): with pytest.warns(UserWarning, match='Catch this') as w: myfunc() assert len(w) == 1 # fails

It comes down to whether you care if the functionality emits other warnings you are not checking for or not.

I think in this case we check the code enough in other places that I don't worry, but clearly that is not the behaviour I had expected....

Both in a comment and in the changelog.

pllim

Definitely an improvement to be able to run more tests without remote data. Thank you!

mhvk · 2020-06-22T21:32:48Z

Yes, this was a nice collaboration on getting rid of the need for remote data!

mhvk added testing no-changelog-entry-needed labels Jun 9, 2020

mhvk added this to the v4.2 milestone Jun 9, 2020

mhvk requested a review from pllim June 9, 2020 02:11

This was referenced Jun 9, 2020

TST: TestManipulation::test_broadcast_to fails with older dependencies #10471

Closed

EXP: Run TestManipulation without remote data #10469

Closed

mhvk added 5 commits June 9, 2020 16:29

Add needs_array_function to broadcasting test of SkyCoord

fbb7227

Ensure config tests do not change configuration.

96d7219

Remove unnecessary remote_data in coordinates.

1c877a3

Remove unnecessary remote_data in utils.iers.

793c711

Ensure set_temp_config restores old configuration.

bb1368e

Here, it should not assume that the configuration is all stored in a file, but include any configuration done programmatically. This is particularly useful to keep utils.iers.conf.auto_download to any setting done in conftest.py.

mhvk force-pushed the coord-test-fixup-and-remote-remote-data branch from 0cd11ce to bb1368e Compare June 9, 2020 20:29

Remove bad default configuration from travis setup

7c00d0d

mhvk commented Jun 10, 2020

View reviewed changes

mhvk added the Ready-for-final-review label Jun 10, 2020

pllim reviewed Jun 12, 2020

View reviewed changes

pllim mentioned this pull request Jun 17, 2020

TST: Revisit tests marked as remote data that can now use offline IERS #10457

Open

mhvk commented Jun 19, 2020

View reviewed changes

pllim reviewed Jun 19, 2020

View reviewed changes

pllim mentioned this pull request Jun 19, 2020

MNT: Stop using our own catch_warnings and raises #10504

Merged

20 tasks

Ensure download is OK for remote-data tests that do need IERS

09acba0

mhvk force-pushed the coord-test-fixup-and-remote-remote-data branch from 21a0586 to 09acba0 Compare June 19, 2020 12:52

pllim reviewed Jun 19, 2020

View reviewed changes

Clarify the slightly changed behaviour of set_temp_config.

e184a18

Both in a comment and in the changelog.

mhvk removed the no-changelog-entry-needed label Jun 20, 2020

pllim approved these changes Jun 22, 2020

View reviewed changes

pllim merged commit 7fcf2ba into astropy:master Jun 22, 2020

mhvk deleted the coord-test-fixup-and-remote-remote-data branch June 22, 2020 21:32

pllim mentioned this pull request Jul 9, 2020

TST: Fix missing import in test_intermediate_transformations.py #10550

Merged

Uh oh!

Remove unnecessary remote_data in (mostly) coordinates. #10474

Remove unnecessary remote_data in (mostly) coordinates. #10474

Uh oh!

Conversation

mhvk commented Jun 9, 2020 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Jun 9, 2020

Uh oh!

pllim commented Jun 9, 2020

Uh oh!

pllim commented Jun 9, 2020

Uh oh!

mhvk commented Jun 9, 2020

Uh oh!

mhvk commented Jun 9, 2020

Uh oh!

pllim commented Jun 9, 2020

Uh oh!

mhvk commented Jun 9, 2020

Uh oh!

mhvk commented Jun 9, 2020

Uh oh!

mhvk commented Jun 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 10, 2020

Uh oh!

pllim 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 commented Jun 19, 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!

mhvk commented Jun 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 19, 2020

mhvk commented Jun 9, 2020 •

edited by pllim

Loading

pllim Jun 11, 2020 •

edited

Loading