Skip to content

Generalize buildcache fetching#50596

Merged
scottwittenburg merged 10 commits intospack:developfrom
mvandenburgh:refactor-specs-from-cache-functions
Jun 3, 2025
Merged

Generalize buildcache fetching#50596
scottwittenburg merged 10 commits intospack:developfrom
mvandenburgh:refactor-specs-from-cache-functions

Conversation

@mvandenburgh
Copy link
Copy Markdown
Member

@mvandenburgh mvandenburgh commented May 21, 2025

This PR moves the functions responsible for retrieving blobs and manifests from a buildcache from binary_distribution.py into url_buildcache.py and generalizes the API so that any type of "buildcache component" (specs, blobs, indexes, keys, key indexes, tarballs, or layout JSONs) can be fetched. This refactor lays the groundwork for implementing buildcache analysis and pruning.

Changes

  • The new url_buildcache functions get_entries_from_cache, _entries_from_cache_aws_cli, and _entries_from_cache_fallback functions replace the old binary_distribution functions _spec_files_from_cache, _specs_from_cache_fallback, and _specs_from_cache_aws_cli, respectively.
  • The APIs remain largely the same, except for these changes
    • Instead of constructing a URLBuildcacheEntry, fetching its manifest, and returning a Spec, the url_buildcache functions now simply construct and return the URLBuildcacheEntry directly. Callers can still get a Spec from the URLBuildcacheEntry themselves if needed. This change will allow this function to be used to fetch arbitrary objects from the buildcache instead of being limited to specs.
    • The functions now take an optional argument, component_type, to allow fetching of any component type (instead of just specs). This parameter defaults to BuildcacheComponent.SPEC, so existing usage of it does not have to change (aside from the function name).
  • I also added some type annotations to assist with the refactor

Future work

  • Once this PR is finalized, I will follow up with another PR implementing buildcache pruning using these refactored APIs.

@spackbot-app spackbot-app bot added binary-packages core PR affects Spack core functionality fetching labels May 21, 2025
@mvandenburgh mvandenburgh force-pushed the refactor-specs-from-cache-functions branch 3 times, most recently from 86d8c4e to d6b7f92 Compare May 21, 2025 17:35
@mvandenburgh
Copy link
Copy Markdown
Member Author

mvandenburgh commented May 21, 2025

@spackbot fix style

@spackbot-app
Copy link
Copy Markdown

spackbot-app bot commented May 21, 2025

Let me see if I can fix that for you!

@spack spack deleted a comment from spackbot-app bot May 21, 2025
@spackbot-app
Copy link
Copy Markdown

spackbot-app bot commented May 21, 2025

I was able to run spack style --fix for you!

spack style --fix 
==> Running style checks on spack
  selected: import, isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/binary_distribution.py
  lib/spack/spack/url_buildcache.py
==> Running import checks
  import checks were clean
==> Running isort checks
Fixing /tmp/tmpjs3xv987/spack/lib/spack/spack/binary_distribution.py
Fixing /tmp/tmpjs3xv987/spack/lib/spack/spack/url_buildcache.py
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/url_buildcache.py
All done! ✨ 🍰 ✨
1 file reformatted, 1 file left unchanged.
  black checks were clean
==> Running flake8 checks
lib/spack/spack/url_buildcache.py:1150: [F401] 'spack.binary_distribution.buildcache_relative_specs_url' imported but unused
  flake8 found errors
==> Running mypy checks
Success: no issues found in 611 source files
  mypy checks were clean
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

@spack spack deleted a comment from spackbot-app bot May 21, 2025
@mvandenburgh mvandenburgh force-pushed the refactor-specs-from-cache-functions branch 3 times, most recently from 0fc3a4c to 96680ef Compare May 21, 2025 17:50
@mvandenburgh mvandenburgh force-pushed the refactor-specs-from-cache-functions branch 3 times, most recently from ed1fc56 to cc5c975 Compare June 2, 2025 20:29
@mvandenburgh mvandenburgh force-pushed the refactor-specs-from-cache-functions branch from cc5c975 to 38ffd9b Compare June 2, 2025 20:47
@mvandenburgh mvandenburgh marked this pull request as ready for review June 2, 2025 20:54
scottwittenburg
scottwittenburg requested changes Jun 2, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@scottwittenburg scottwittenburg left a comment

Choose a reason for hiding this comment

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

This is looking good @mvandenburgh, thanks! I requested a couple docstring changes and mentioned one other thing for you to look at, hopefully it should be quick.

And one other thing: I don't think that the unit tests run on images with aws installed, can you test that path manually? On a machine where you have the aws command available, you just need to rebuild in the index of a mirror that lives in s3. Since I know you have permission 😜 it could be as simple as:

spack buildcache update-index --keys s3://spack-binaries/develop/build_systems

Feel free to ping me if you have any questions, and thanks again!

@mvandenburgh mvandenburgh force-pushed the refactor-specs-from-cache-functions branch from 880386b to 7b953bf Compare June 3, 2025 14:37
@mvandenburgh
Copy link
Copy Markdown
Member Author

mvandenburgh commented Jun 3, 2025

@scottwittenburg I made a pass on updating the docstrings, PTAL.

I also ran the spack buildcache update-index command you mentioned and it completed successfully (and I verified that the specs were downloaded via aws with the _entries_from_cache_aws_cli function, and not _entries_from_cache_fallback w/ web_util).

scottwittenburg
scottwittenburg approved these changes Jun 3, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@scottwittenburg scottwittenburg left a comment

Choose a reason for hiding this comment

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

Thanks for testing the aws code path manually, as it's clear from the coverage report that it's not tested in unit tests. Otherwise, everything else here looks good to me, thanks @mvandenburgh!

@scottwittenburg scottwittenburg merged commit 7c3374e into spack:develop Jun 3, 2025
35 of 36 checks passed
@mvandenburgh mvandenburgh deleted the refactor-specs-from-cache-functions branch June 3, 2025 16:06
kshea21 pushed a commit to kshea21/spack that referenced this pull request Jun 18, 2025
@mvandenburgh @kshea21
Generalize buildcache fetching (spack#50596)
1cffccc 
Move the functions responsible for retrieving buildcache manifests into the
url_buildcache.py module, and generalize the API so that manifests for any
type of buildcache component (specs, indices, keys, etc) can be fetched the
same way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scottwittenburg scottwittenburg scottwittenburg approved these changes

@kwryankrattiger kwryankrattiger Awaiting requested review from kwryankrattiger

Assignees

No one assigned

Labels

binary-packages core PR affects Spack core functionality fetching

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mvandenburgh @scottwittenburg