Skip to content
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

BUG: Fix SDSS download caching #3123

Merged
merged 3 commits into from
Dec 20, 2024
Merged

BUG: Fix SDSS download caching #3123

merged 3 commits into from
Dec 20, 2024

Conversation

keflavich
Copy link
Contributor

Fixes #3121.

However, I've only implemented a trivial thing so far; there's some digging to do before we move on with this.

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.54%. Comparing base (7d96ebd) to head (ad2e207).
Report is 30 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3123   +/-   ##
=======================================
  Coverage   67.54%   67.54%           
=======================================
  Files         233      233           
  Lines       18493    18493           
=======================================
  Hits        12491    12491           
  Misses       6002     6002           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bsipocz
Copy link
Member

bsipocz commented Dec 16, 2024

the linelist change seems to be unrelated, but I'm al for wrapping up draft PRs (there is one for linelists already :) )

@keflavich
Copy link
Contributor Author

I've determined that the second part of my suggestion here:
#3121 (comment)
is infeasible. We would have to change the download behavior of FileContainer entirely to gain control over the paths.

Astroquery is using get_readable_fileobj, which then uses astropy tools down to the bottom turtle to find the path. We could hack XDG_CONFIG_HOME, but that is a Bad Idea; we'd be better off refactoring FileContainer, and I think that is a low priority.

def get_readable_fileobj(*args, **kwargs):
"""
Overload astropy's get_readable_fileobj so that we can safely monkeypatch
it in astroquery without affecting astropy core functionality
"""
return aud.get_readable_fileobj(*args, **kwargs)

https://github.com/astropy/astropy/blob/fda05b894e093a282ec46b1447d2ad24bf8b21da/astropy/utils/data.py#L201

https://github.com/astropy/astropy/blob/fda05b894e093a282ec46b1447d2ad24bf8b21da/astropy/utils/data.py#L357-L359

https://github.com/astropy/astropy/blob/fda05b894e093a282ec46b1447d2ad24bf8b21da/astropy/utils/data.py#L1505

https://github.com/astropy/astropy/blob/fda05b894e093a282ec46b1447d2ad24bf8b21da/astropy/utils/data.py#L1870

https://github.com/astropy/astropy/blob/fda05b894e093a282ec46b1447d2ad24bf8b21da/astropy/config/paths.py#L90

https://github.com/astropy/astropy/blob/fda05b894e093a282ec46b1447d2ad24bf8b21da/astropy/config/paths.py#L23

@bsipocz I'm not sure there's a good test for this. I'll keep working to try to figure one out, but I'd recommend merging this as-is. My by-hand test says it works:

$ python3 -c 'from astroquery.sdss import SDSS; SDSS.get_spectra(plate=827, fiberID=23, mjd=52312, cache=False)'
Downloading https://data.sdss.org/sas/dr17/sdss/spectro/redux/26/spectra/0827/spec-0827-52312-0023.fits
|==========================================================================================================================================================================================================| 892k/892k (100.00%)         0s
$ ls -lh ~/.astropy/cache/download/url/0ad9*
ls: /Users/adam/.astropy/cache/download/url/0ad9*: No such file or directory

@bsipocz
Copy link
Member

bsipocz commented Dec 20, 2024

OK, let's merge this as is. as for

we'd be better off refactoring FileContainer

Yes, I agree and it's on my roadmap (we'll have to work on some download tools for pyvo, too, so I foresee to upstream or remove or at least refactor a lot of astroquery along the way, too)

@bsipocz bsipocz added the sdss label Dec 20, 2024
@bsipocz bsipocz marked this pull request as ready for review December 20, 2024 02:20
@bsipocz bsipocz changed the title Bugfix WIP: Fix SDSS download caching (and maybe more broadly, download caching) BUG: Fix SDSS download caching Dec 20, 2024
@bsipocz bsipocz added this to the v0.4.8 milestone Dec 20, 2024
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added changelog and one more caching consistency.

@bsipocz bsipocz merged commit 45b93c0 into astropy:main Dec 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDSS caches files despite cache=False
2 participants