Repository navigation
two coordinates tests fail if jplephem is not installed (in 4.3rc1) #11857
Description
Without jplephem installed, I see two coordinates-related failures, at
- test_aa_high_precision : astropy/coordinates/tests/test_intermediate_transformations.py:708
- test_horizons_consistency_with_precision: astropy/coordinates/tests/test_solar_system.py:305
both are ImportError: Solar system JPL ephemeris calculations require the jplephem package (https://pypi.org/project/jplephem/)
Probably these just need a pytest.skipif when jplephem is not installed.
Activity
pllim commentedon Jun 18, 2021
I think skipping is fine but maybe @mhvk can confirm.
mhvk commentedon Jun 18, 2021
Yes to both. The first seems a backport mistake, the second is wrong in main as well (I guess our remote_data=any tests also have jplephem installed!?)
mhvk commentedon Jun 18, 2021
See #11858 for the second one.
Of course, any test that needs
jplephemneeds remotedata, just the reverse is not true. Perhaps to be solved by some smart caching in the future...mhvk commentedon Jun 18, 2021
@eteq - do you want to do the first directly in 4.3.x? Or would you prefer a quick PR?
pllim commentedon Jun 21, 2021
pllim commentedon Jun 21, 2021
@mhvk , is it #11712 ? That one is milestoned to 5.0. Should its milestone be changed?
mhvk commentedon Jun 21, 2021
@pllim - I must admit I'm now confused: I think I meant with "backport mistake" that somehow the part that skipped if jplephem was not installed was removed due to some backport. But looking further, I'm not sure any more. In any case, my sense would be to just try adding
pytest.mark.skipif('not HAS_JPLEPHEM'), and worry further if that does not work...pllim commentedon Jun 22, 2021
Looking back at #11700 and #11712 (both milestoned for 5.0), I am not sure if blind skipping is enough. I think they both need to be backported.
pllim commentedon Jun 22, 2021
p.s. We do have a test with minimal deps but it doesn't run
remote_data=True. That's how we missed it.astropy/.github/workflows/ci_workflows.yml
Lines 56 to 59 in dda9fa1
mhvk commentedon Jun 22, 2021
Yes, I fear you may well be right....
dhomeier commentedon Jun 22, 2021
The only test enabling
remote_dataat all seems to be with the dev versions, which of course has jplephem and in addition is an allowed failure.pllim commentedon Jun 23, 2021
Backports are done and should be in 4.3.1.
mhvk commentedon Jun 23, 2021
@dhomeier - sadly, the
remote_datatests fail too often because of retrieval issues to be in anything except allowed failure... Though I think now that we have a way of setting up the cache astropy uses, it should be possible to move some of those tests over to using that. There is really no good reason for us to be downloading the JPL ephemerides every time our tests run - it is not like those files change!dhomeier commentedon Jun 24, 2021
I was more surprised there was really only one test run with
remote_data, but then of course enabling them in others would mean they would also have to be marked as allowed failures.Seems pytest could offer a bit finer control e.g. by marking "non-critical" vs. "critical" failures, or even flagging network-related failures...
But that might still leave one with rather limited information as to when those tests were actually passing; relying more on the cache would certainly be a better solution.
pllim commentedon Jun 24, 2021
Unit testing can only do so much. We cannot test for all the possible combo with the free resources we get from Actions. But maybe we can compromise by adding a "remote data + minimal deps" job to the daily cron or something. If you are interested to pursue this matter further, please open a new issue. This particular issue has been resolved. Thanks!