Lock-free cache implementation - #10437

#10430 fixes the test_name_resolve_cache failure. (It's a bad test, which that PR fixes.)

This whole approach may be impossible on Windows, which appears to have a 260-character limit on paths; the astropy cache lives pretty deep in the directory tree already, so users are going to run out of space pretty quickly.

https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file?redirectedfrom=MSDN

One could have both implementations side-by-side, using the existing one on Windows, but that seems like a maintenance nightmare. (Though the new implementation could be made more parallel to the old one, with locking context managers that just don't do anything and so forth.)

Edited to add: Solved by changing the approach.

There is also a family of solutions, still lock-free, that store the contents under the hash of the URL; the minor headache is that these then need to store the URL too, and these are not really very friendly to read. I'd like to know whether the basic approach actually helps on clusters, though.

Edited to add: Adopted.

There is also a family of solutions, still lock-free, that store the contents under the hash of the URL; the minor headache is that these then need to store the URL too, and these are not really very friendly to read

Maybe the choice of using a hash vs the URL could be configurable and default to hashes on Windows and URLs on Mac/Linux?

There is also a family of solutions, still lock-free, that store the contents under the hash of the URL; the minor headache is that these then need to store the URL too, and these are not really very friendly to read

Maybe the choice of using a hash vs the URL could be configurable and default to hashes on Windows and URLs on Mac/Linux?

It's not as bad as keeping the lock-y version around but it's still a bit of a headache in terms of code duplication. And, like, what do you do when your cache is a mix of the two? When it contains two different files for the same URL, one in each scheme? I think the ability to look at cache contents by eye, seeing past all the percent quoting, is just not worth the hassle. Let's see if this form is tolerable, concurrency-wise.

The scheme in the PR of a directory made up of the hash and url and content files inside it seems quite nice!

The scheme in the PR of a directory made up of the hash and url and content files inside it seems quite nice!

As long as I don't have to make it work on 8.3 filenames...

I'm at that joyful stage where I'm relying on CI to test on a variety of platforms. I thought tox might help but it takes forever and every tox run fails because some FITS files exist.

Fixes #9977
May resolve #9765 but needs testing

every tox run fails because some FITS files exist.

Darn, I thought that got solved! @pllim? Those definitely come from doctests, so you can work around it by only running the actual tests you are interested in.

Since I cannot remember how to do it directly via tox, I'll just give the way I have been told is not optimal and possibly confusing, but which works...

tox -e <whatever> --develop --notest  # whatever may just be "test"
souce .tox/<whatever>/bin/activate
pytest astropy/<relevant_test_directory>
# Edit and repeat pytest until things work
deactivate

Hello @aarchiba 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-08-05 16:54:30 UTC

I just tried this on our (LIGO) clusters and there were no problems!

I would prefer a variant that does not place severe limits on the length of the URL, so base32 or some other compressed encoding would be preferable.

I just tried this on our (LIGO) clusters and there were no problems!

I would prefer a variant that does not place severe limits on the length of the URL, so base32 or some other compressed encoding would be preferable.

Oooh that's excellent news!

The current implementation places no limit (well maybe a few GB) on URL length - the URL is just stored in a file. Lookups are fast because the data is stored in a directory based on the URL's hash, though reverse lookups do require going through all the cached files until you find the right URL.

Locking is eliminated, and concurrency issues are minimized by using renames to move populated directories into and out of place. The chief concurrency headache is sort of unavoidable - because the interface is based on file names there's no way to prevent someone from clearing the cache and deleting the file out from under you. So don't do that. Otherwise I think we have fairly aggressive concurrency tests with many threads writing and/or updating files simultaneously, so hopefully problems would have shown up in the testing.

So right now the URLs are written to text files on disk. Are we going to have headaches with non-ASCII URLs? Specifically are people going to break things by using different default encodings? I could enforce UTF-8, though perhaps this is mildly hostile to people using other text tools to browse through their cache contents?

Probably fixes #10401

I think using utf-8 is entirely reasonable.

Pretty clean now. I'm happy to squash and rebase when it won't interfere with review.

The current implementation places no limit (well maybe a few GB) on URL length - the URL is just stored in a file. Lookups are fast because the data is stored in a directory based on the URL's hash, though reverse lookups do require going through all the cached files until you find the right URL.

Are you at all concerned about hash collisions?

The current implementation places no limit (well maybe a few GB) on URL length - the URL is just stored in a file. Lookups are fast because the data is stored in a directory based on the URL's hash, though reverse lookups do require going through all the cached files until you find the right URL.

Are you at all concerned about hash collisions?

Last time I updated this I complained about the use of MD5, for which hash collisions can be created, and implemented a change to a more modern cryptographic hash, but there was opposition. Since it's a 128-bit hash the probability of an accidental collision is vanishingly small, and since what is being hashed is the URL being requested, I don't think collisions are a serious concern.

@zonca or @wumpus, would you be interested to take this PR for a spin?

Thank you, everyone!

This bit astroquery. I didn't follow this PR, so if either of you could be willing to have a look before I can get there in the weekend, it would be awesome.

https://travis-ci.org/github/astropy/astroquery/jobs/715679907#L4357

Yeah, looks like astroquery might have monkeypatched a hidden function. I think I can fix it.

Yes, hopefully it only affects the monkeypatching. If anything is felt outside on the userlevel, then this PR should not be backported to LTS, (even though it's a private function).

Turns out, it is not monkeypatching (ref https://github.com/astropy/astroquery/blob/dd1bb15276db363debb5afef6eb66ff904c5ddc5/astroquery/utils/commons.py#L397) but fixing it also won't affect user-facing API. I have to see what this save_fits is actually trying to do. It shouldn't have to access hidden function like that.

Turns out, it is not monkeypatching (ref https://github.com/astropy/astroquery/blob/dd1bb15276db363debb5afef6eb66ff904c5ddc5/astroquery/utils/commons.py#L397) but fixing it also won't affect user-facing API. I have to see what this save_fits is actually trying to do. It shouldn't have to access hidden function like that.

This is an unnecessary abuse of the cache internals. You could simply use download_file with sources=[] to obtain the filename and then sym/hard link it to the destination.

To be fair, astroquery existed when astropy was very young. 😄

Thanks a lot for doing this work! The warning brought up in #9970 has been driving me nuts.

Any idea when we can expect this code to make it into v4.0.2?

"Soon" 😉

@eteq is about to make a release. In the midst of the pandemic, we ask for your patience. Thanks!

Looks like this was backported in 4.1.x without problem, but a special PR #10683 exists to backport this manually to 4.0.x. I slapped a "Manual Backport" label here, but if I am misusing that label, feel free to remove.

p.s. We should maybe document usage of labels somewhere.