-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix IERS_Auto logic now that IERS-A is bundled #16187
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
Just a heads-up that this patch seems to touch some of the same lines than #16070, so merge conflicts might ensue. That said, my own PR is currently stuck so I don't have a problem with this one going in first, mind you. I'm also happy to serve as a reviewer if needed ! |
I think #16070 will be sufficiently orthogonal – improving how IERS-A and IERS-B are combined versus merely ensuring that the combination even happens – that any merge conflicts should be trivial to resolve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Does the content of https://docs.astropy.org/en/stable/utils/data.html also need updating?
Ah, indeed, that page should be edited too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much! I have one question on the implementation, whether it might not be even simpler to just preserve whatever has been downloaded (which in itself would seem more logical). What do you think? I am happy to go with what you have as well, as it resolves the biggest issues.
Beyond that, only a nitpick on the changelog entry...
docs/changes/utils/16187.api.rst
Outdated
@@ -0,0 +1 @@ | |||
The return type of ``IERS_Auto.open()`` is no longer ``IERS_B`` when automatic updating of the IERS-A file is disabled or the downloading of the new file fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add what it is now! Maybe
IERS_Auto.open() now defaults to the combination of IERS_A and B data bundled
in the astropy-iers-data dependency rather than to IERS_B only when automatic ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overhauled text
@@ -786,7 +790,7 @@ def open(cls): | |||
|
|||
""" | |||
if not conf.auto_download: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the alternative path, which would preserve anything downloaded, would simply be to remove this stanza and in the else
clause of the for
loop warn only if conf.auto_download
is set.
Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion concluded we should leave this for follow-up
Yes, that could be done, but the edge case that worries me is a user who does not want automatic downloading but at some point in the past happened to download IERS-A. Once that file is in the cache, that stale IERS-A file would continue to be used (by default) despite any subsequent updates of Of course, the natural solution would be to inspect both the IERS-A file from |
@ayshih - yes, good point. So, I guess to get this to work one would have to also change p.s. We can obviously move this to a new issue/PR! |
Let's get this PR in to solve the easy-to-encounter bug, and defer the improved handling of two IERS-A files to a future PR. I've updated the documentation more, including https://docs.astropy.org/en/stable/utils/data.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! (modulo the failure of course!)
docs/utils/data.rst
Outdated
@@ -265,25 +263,19 @@ systems. The parallel access to the home directory can also trigger concurrency | |||
problems in the Astropy data cache, though we have tried to minimize these. We | |||
therefore recommend the following guidelines: | |||
|
|||
* Do one of the following: | |||
* Set ``astropy.utils.iers.conf.auto_download = False`` in your Astropy config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change to have just one recommended way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayshih - I have a few more nitpicky comments on docstrings, to join those of @eerovaher. Will merge once those are done.
Thanks!
@@ -786,7 +790,7 @@ def open(cls): | |||
|
|||
""" | |||
if not conf.auto_download: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion concluded we should leave this for follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for doing this. Not only is it fixing a subtle bug that has already confused several users, it's also making the underlying logic easier to reason about ! I only have a couple small suggestions and questions, but overall LGTM
if PYTEST_LT_8_0: | ||
ctx1 = ctx2 = nullcontext() | ||
ctx1 = nullcontext() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we might want to rename this variable ctx
now. More importantly: thanks, seeing this test being simplified back is a nice surprise !
"full IERS file with predictions was already downloaded and cached). " | ||
"Enable auto-downloading of the latest IERS-A data. If set to False " | ||
"then the bundled IERS-A file will be used by default (even if a " | ||
"newer version of the IERS-A file was already downloaded and cached). " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor, but to me, "already" may suggest that this could have happened while running the current process.
"newer version of the IERS-A file was already downloaded and cached). " | |
"newer version of the IERS-A file was previously downloaded and cached). " |
The IERS A file is not part of astropy. It can be downloaded from | ||
``iers.IERS_A_URL`` or ``iers.IERS_A_URL_MIRROR``. See ``iers.__doc__`` | ||
for instructions on use in ``Time``, etc. | ||
If the package IERS A file (```iers.IERS_A_FILE``) is out of date, a new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the package IERS A file (```iers.IERS_A_FILE``) is out of date, a new | |
If the package IERS A file (``iers.IERS_A_FILE``) is out of date, a new |
# auto_download = False is tested in test_IERS_B_parameters_loading_into_IERS_Auto() | ||
|
||
def teardown_class(self): | ||
iers.conf.auto_download = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that this could potentially create test pollution if the default value was ever changed (for instance if we were to deprecated this parameter), or worse, it could hide pollution from other tests if they change this config value but forget to clean up after themselves.
While unlikely, these situation would be very painful to debug so I suggest to go with the cleaner approach from the get go, resetting the value to whatever it was before setup_class
instead of hardcoding the current default value here.
# Double-check that auto downloading is off | ||
assert not iers.conf.auto_download |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a guard clause, we could use a pytest fixture, or a context manager here. Any reason not to ?
* Do one of the following: | ||
* Set ``astropy.utils.iers.conf.auto_download = False`` in your ``astropy`` config | ||
file (see :ref:`astropy_config`) or in your code so that ``astropy`` will not | ||
automatically attempt to download a newer version of the IERS-A table than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something and it's actually still possible to trigger the download manually ?Otherwise, I think this adverb could be dropped.
automatically attempt to download a newer version of the IERS-A table than | |
attempt to download a newer version of the IERS-A table than |
@@ -495,7 +476,7 @@ def test_iers_download_error_handling(tmp_path): | |||
"malformed IERS table from https://google.com" | |||
) | |||
assert str(record[2].message).startswith( | |||
"unable to download valid IERS file, using local IERS-B" | |||
"unable to download valid IERS file, using local IERS-A" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this right ?
"unable to download valid IERS file, using local IERS-A" | |
"unable to download valid IERS file, using bundled IERS-A" |
Description
#14819 moved the IERS files to a separate package (
astropy-iers-data
), and now IERS-A is essentially bundled withastropy
. However, there persist some remnants of the old logic, where IERS-A is available only if the download were triggered. This results in the peculiar behavior that if automatic downloading of IERS-A files is disabled (iers.conf.auto_download = False
) or fails, the bundled IERS-A file is ignored even though it is present (see #13227 (comment)).This PR fixes the logic so that the bundled IERS-A file is used if
iers.conf.auto_download = False
, with corresponding fixes in documentation and tests.This PR does not attempt to fix the logic so that a previously downloaded IERS-A file is used if
iers.conf.auto_download
is subsequently set toFalse
. I didn't want to wrap my head around whether the downloaded file should have priority over the bundled file, given thatastropy-iers-data
could also be upgraded.Fixes #13227: With
astropy
6.0, the response to the report would be to upgradeastropy-iers-data
to obtain a recent IERS-A table. This PR then fixes the residual bug where settingiers.conf.auto_download = False
would ignore that bundled IERS-A table.Fixes #16128: Because the bundled IERS-A is now no longer ignored by the default
IERS_Auto
when downloading is blocked, it's now typically impossible to trigger the "degraded accuracy" error/warning unless the user expressly forces the use of the IERS-B data alone (e.g., through theearth_orientation_table
ScienceState
).Other related issues: