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.

Sign up for GitHub

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

Jump to bottom

Cache.download() deletes an expired file before knowing that it can download a new version of the file #7249

Open
ayshih opened this issue Oct 22, 2023 · 4 comments · May be fixed by #7265
Open

Cache.download() deletes an expired file before knowing that it can download a new version of the file #7249

ayshih opened this issue Oct 22, 2023 · 4 comments · May be fixed by #7265
Labels
Bug Probably a bug data Affects the data submodule Effort Low Requires a small time investment Good First Issue The best issues for new people to tackle! Package Intermediate Requires some knowledge of the internal structure of SunPy Priority Low Slow action required

Comments

@ayshih
Copy link
Member

ayshih commented Oct 22, 2023

Provide a general description of the issue or problem.

The download() method of our Cache class deletes an expired file:

sunpy/sunpy/data/data_manager/cache.py

Lines 76 to 79 in 7821330

if redownload or self._has_expired(details):
# if file is in cache and it has to be redownloaded or the cache has expired
# then remove the file and delete the details from the storage
os.remove(details['file_path'])

before attempting to download a new version of the file:

sunpy/sunpy/data/data_manager/cache.py

Line 84 in 7821330

file_path, file_hash, url = self._download_and_hash(urls, namespace)

That means that if the download does not succeed for whatever reason, the user now has no copy of the file. Such a harsh application of expiration would be appropriate if we were dealing with time bombs, but for virtually all use cases, an expired file is just as usable as a new version.

We should not delete the expired file until after successfully downloading a new version of the file. If the downloading fails, we retain the expired file, and we can emit a warning to the user that they will be working with a potentially stale file.
@ayshih ayshih added Priority Low Slow action required Effort Low Requires a small time investment Package Intermediate Requires some knowledge of the internal structure of SunPy data Affects the data submodule labels Oct 22, 2023
@Cadair Cadair added Bug Probably a bug Good First Issue The best issues for new people to tackle! labels Oct 23, 2023
@samish55 samish55 linked a pull request Oct 26, 2023 that will close this issue
Draft
@samish55
Copy link

You can check the open PR #7265.

@NucleonGodX
Copy link
Contributor

the PR of this issue is drafted for over 2.5months now can I take up this issue?

@nabobalis
Copy link
Contributor

That PR was waiting for unit tests to be added. IF you want to help @samish55 finish off the PR, you should contact them first.

@NucleonGodX
Copy link
Contributor

That PR was waiting for unit tests to be added. IF you want to help @samish55 finish off the PR, you should contact them first.

alrighty makes sense, he don't seem to have any particular contact on github, but since its just unit tests left I think @samish55 might address them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Probably a bug data Affects the data submodule Effort Low Requires a small time investment Good First Issue The best issues for new people to tackle! Package Intermediate Requires some knowledge of the internal structure of SunPy Priority Low Slow action required
Projects
None yet
Milestone
No milestone
5 participants
@ayshih @Cadair @nabobalis @NucleonGodX @samish55