Enable concretization caching (redux)#50905
Merged
tgamblin merged 37 commits intospack:developfrom Oct 18, 2025
Merged
Conversation
spackbot-app
bot
added
core
johnwparent
changed the title
johnwparent
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
c4523e4 to
5331a69
Compare
johnwparent
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
3adf53d to
cbb74bc
Compare
johnwparent
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
cbb74bc to
c1ea08e
Compare
johnwparent
commented
Aug 29, 2025
johnwparent
commented
Aug 29, 2025
johnwparent
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
c1ea08e to
fdfdd7b
Compare
johnwparent
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
fdfdd7b to
8bcaa07
Compare
johnwparent
commented
Sep 19, 2025
johnwparent
commented
Sep 19, 2025
johnwparent
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
6bb4a47 to
a207f1c
Compare
johnwparent
commented
Sep 19, 2025
tgamblin
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
d932010 to
c26f08a
Compare
tgamblin
requested changes
Oct 13, 2025
tgamblin
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
c26f08a to
3e86583
Compare
johnwparent
commented
Oct 14, 2025
tgamblin
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
3e86583 to
70d5122
Compare
Contributor
Author
|
@spackbot fix style |
|
Let me see if I can fix that for you! |
|
I was able to run spack style --fix==> Running style checks on spack
selected: import, isort, black, flake8, mypy
==> Modified files
lib/spack/spack/database.py
lib/spack/spack/llnl/util/filesystem.py
lib/spack/spack/paths.py
lib/spack/spack/schema/concretizer.py
lib/spack/spack/schema/config.py
lib/spack/spack/solver/asp.py
lib/spack/spack/solver/input_analysis.py
lib/spack/spack/solver/runtimes.py
lib/spack/spack/test/concretization/core.py
lib/spack/spack/test/conftest.py
==> Running import checks
import checks were clean
==> Running isort checks
isort checks were clean
==> Running black checks
reformatted lib/spack/spack/solver/asp.py
All done! ✨ 🍰 ✨
1 file reformatted, 9 files left unchanged.
black checks were clean
==> Running flake8 checks
flake8 checks were clean
==> Running mypy checks
Success: no issues found in 617 source files
mypy checks were clean
==> spack style checks were clean
I've updated the branch with style fixes. |
Add test for cache cleanup Cleanup existing tests use correct cache limit config name Switch misc_cache -> user_cache_path Remove dropped race condition handling Improve lockfile cleanup Document config options Signed-off-by: John Parent <[email protected]>
…moved Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
…sistent fixture root Signed-off-by: John Parent <[email protected]>
johnwparent
force-pushed
the
concretization-cache/cleanup-and-test
branch
from
305bbf6 to
1d0d190
Compare
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: John Parent <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
tgamblin
approved these changes
Oct 18, 2025
tgamblin
changed the title
tgamblin
added a commit
that referenced
this pull request
Oct 19, 2025
the ConcretizationCache, enabled in #50905, could still use a few more simplifications. - [x] Use `len` instead of `sum(1 ...)` - [x] Use `WriteTransaction` and `ReadTransaction` context managers instead of reimplementing. - [x] Clean up fetch a bit for better control flow Signed-off-by: Todd Gamblin <[email protected]>
3 tasks
alalazo
added a commit
to alalazo/spack
that referenced
this pull request
Oct 20, 2025
spack#50905 added a few tests that are using the `builtin` repository instead of `builtin.mock`. This is a fix to that small issue. Signed-off-by: Massimiliano Culpo <[email protected]>
johnwparent
pushed a commit
that referenced
this pull request
Oct 20, 2025
#50905 added a few tests that are using the `builtin` repository instead of `builtin.mock`. This is a fix to that small issue. Signed-off-by: Massimiliano Culpo <[email protected]>
tgamblin
added a commit
that referenced
this pull request
Oct 20, 2025
the ConcretizationCache, enabled in #50905, could still use a few more simplifications. - [x] Use `len` instead of `sum(1 ...)` - [x] Use `WriteTransaction` and `ReadTransaction` context managers instead of reimplementing. - [x] Clean up fetch a bit for better control flow Signed-off-by: Todd Gamblin <[email protected]>
alalazo
reviewed
Oct 23, 2025
|
|
||
| """ | ||
| # path must be absolute at this point | ||
| assert path.is_absolute() |
Member
There was a problem hiding this comment.
I tried to bump the Spack commit in spack-packages, which is currently at 501dae3
It turns out this PR breaks all Windows builds in pipelines because of this assertion (and probably the corresponding one in read_transaction). This is an example of a failing build on Windows:
System.Management.Automation.RemoteException
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\bootstrap\core.py", line 426, in ensure_executables_in_path_or_raise
if current_bootstrapper.try_search_path(executables, abstract_spec):
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\bootstrap\core.py", line 318, in try_search_path
concrete_spec = spack.concretize.concretize_one(abstract_spec_str)
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\concretize.py", line 201, in concretize_one
result = Solver().solve([spec], tests=tests, allow_deprecated=allow_deprecated)
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\solver\asp.py", line 4132, in solve
result, _, _ = self.solve_with_stats(specs, **kwargs)
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\solver\asp.py", line 4120, in solve_with_stats
result = self.driver.solve(
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\solver\asp.py", line 1253, in solve
self._conc_cache.store(cache_key, result, self.control.statistics)
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\solver\asp.py", line 785, in store
with self.write_transaction(cache_path, timeout=30):
File "C:\builds\spack\spack-packages\.ci\tmp\spack\lib\spack\spack\solver\asp.py", line 773, in write_transaction
assert path.is_absolute()
Is it necessary to check that the path is absolute here (I didn't read the code yet)? If it is, we should work out why in pipelines we don't have an absolute path, and add a better error message than just a blank assertion error.
For completeness the PR bumping the Spack commit is: spack/spack-packages#989
Contributor
Author
There was a problem hiding this comment.
I don't think it needs to be absolute, we pass path.name to the lock anyway. I think the difference in behavior is coming from cannonicalize_path although not sure why that wouldn't have shown up in unit tests, so perhaps a CI setting issue.
I'll PR a quick fix for the abs paths and then a follow up to make the behavior consistent.
Contributor
Author
There was a problem hiding this comment.
Can't replicate locally, smells like a CI config issue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third time is the charm?
Redux of #49589 which was reverted by #50851 due to race conditions, which efforts have since been made to remove.
#48198 Introduces the concretization cache, and associated manifest. However it only added testing for the cache itself, not the manifest or cleanup procedures.
This PR:
FileCacheclass was refactored into an abstract baseCacheclass which provides the general cache read/write transaction interface, and aFileCacheandDirectoryCachechild classes,FileCachewhich should be functionally identical to theFileCacheclass pre refactor, deals with caches at the individual file level, andDirectoryCache, which is mean to provide a directory level or "bucket" caching level.