cc @tgamblin @becker33

Addressing the import check failure will require moving around some additional blocks of code into new modules, pushing up a pass at that now, but very open to alternative suggestions

I've had to move significantly more code than planned. The spack.solver module either needs a substantive re-org or the concretization cache should be left out of the spack.caches module, I'll hold on general consensus on how we want to handle that.

concretization cache should be left out of the spack.caches module

Go for it

I only quickly glanced at this, but what I've seen does not look process safe.

You could do file locking per cache entry / dir, but probably easier is to do atomic adds (write to safe tempfile in target dir and os.rename atomically) and reads (just an open call), minimize tree traversal, and assume that any time you traverse most operations can raise.

Also write in tell don't ask style, any if is_dir test may not be the case anymore the next line.

I only quickly glanced at this, but what I've seen does not look process safe.

You're right, Windows is single-spack only at the moment, so I often forget this, will address, thanks.

Looks like this still needs to handle splicing a bit better, will investigate further monday

Gitlab failures are currently rouge pipeline failures.

Did anybody try any performance benchmark on this PR? Otherwise I can try to compare with develop on a cold and hot cache.

@johnwparent had shared some numbers previously.

I agree it would be nice to have a record of that in the PR too.

@kwryankrattiger @alalazo benchmarks have been added to the PR description

Do we have a lockfile per cache entry:

$ ls -larth ~/.spack/cache/concretization/12
total 1,1M
-rwxrwxr-x 1 culpo culpo    0 feb 28 18:36 .128f2dc2834e4e295aca1f50be39c5003340cbcaaff2db8e94ee565d92c3bea1.lock
drwxrwxr-x 3 culpo culpo 4,0K feb 28 18:36 ..
-rw-rw-r-- 1 culpo culpo 1,1M feb 28 18:36 128f2dc2834e4e295aca1f50be39c5003340cbcaaff2db8e94ee565d92c3bea1
drwxrwxr-x 2 culpo culpo 4,0K feb 28 18:36 .

?

Do we have a lockfile per cache entry:

$ ls -larth ~/.spack/cache/concretization/12
total 1,1M
-rwxrwxr-x 1 culpo culpo    0 feb 28 18:36 .128f2dc2834e4e295aca1f50be39c5003340cbcaaff2db8e94ee565d92c3bea1.lock
drwxrwxr-x 3 culpo culpo 4,0K feb 28 18:36 ..
-rw-rw-r-- 1 culpo culpo 1,1M feb 28 18:36 128f2dc2834e4e295aca1f50be39c5003340cbcaaff2db8e94ee565d92c3bea1
drwxrwxr-x 2 culpo culpo 4,0K feb 28 18:36 .

?

Yes, each entry is locked individually as per the behavior of the underlying FileCache used to manage the cache entries.
We get two benefits with this approach, the entry locks are more available than if they were managed by a single, central lock, and using FileCache allows us to switch out the caching backend in the future to allow for remote caches.

I guess we can probably optimize in a later PR to use a single lock file per the entire cache, like we do for store and stages (and lock different bytes based on the entry, iirc).

@kwryankrattiger why did you merge this? Did you try it out? Please check with other reviewers before merging.

I would suggest to disable this feature, because getting cache hits reliably for even the simplest spec is impossible:

concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache miss at 18/18607911a1944d36e1a49f3728783e0a957a0a21c390138f22044afc1715ba4e
concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache miss at 28/286c42d84b58dbbc59d6e9197fb31560b1fafe6d1044a69b04f546e4f6e02f6d
concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache hit at 28/286c42d84b58dbbc59d6e9197fb31560b1fafe6d1044a69b04f546e4f6e02f6d
concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache hit at 18/18607911a1944d36e1a49f3728783e0a957a0a21c390138f22044afc1715ba4e
concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache miss at de/de4b86777b02dbba18b370a56ce6d40213ab03d775432dd888909622b9687cff
concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache miss at dc/dcdd9c18c0480908ae5676b4ec026a091f5c999e4d1f1bb366c0dd4e990f3f79
concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache miss at 4f/4f998c4b067724eab7de84ebcc43ba2bb3450648819d75fe0a14b816e7d24a0e
concretization $ spack -d  spec zlib |& grep -E '(hit|miss)'
==> Concretization cache miss at 43/4301e1911df4c87eaab53d3991c22986c8d64948ce6129564aaed2cfad570028

2 of 7 hits, that's really bad.

Also don't understand why the config option was put under config:concretization_cache instead of concretizer:cache. Also why is config:concretization_cache:url the key if it's a path, it doesn't accept arbitrary urls and does not even accept file urls.

I had tried it out and we had discussed this PR in another meeting with @tgamblin and @alalazo and it was fine to merge.

The issues causing non determinism were found post merge. The issues are known and being resolved by @johnwparent now. It isn't breaking workflows so no one jumped to revert.

The key is URL is because it is going to accept URLs in the near term for CI sharing of the cache.

The location of the config was chosen to match the other misc cache configs. We don't put mirror misc cache configs in mirrors.yaml for example.

We don't put mirror misc cache configs in mirrors.yaml for example.

Not the best example given that misc_cache is a combination of repo metadata, compiler output and build cache indices.

I still think it's better to put it in concretizer given it has various sub-options.