Add binary distribution cache manager#18359
Merged
scottwittenburg merged 3 commits intospack:developfrom Sep 30, 2020
Merged
Conversation
Contributor
Author
Contributor
Author
|
Just a heads up @gartung that this refactor of binary cache management is in the works and could be ready for review fairly soon. I'd like to add you as a reviewer when it's ready. |
scottwittenburg
force-pushed
the
add-binary-distribution-cache-manager
branch
4 times, most recently
from
5d6cd8d to
2d11afe
Compare
scottwittenburg
changed the title
Contributor
Author
opadron
reviewed
Sep 14, 2020
| def find_built_spec(self, spec): | ||
| """Find the built spec corresponding to ``spec``. | ||
|
|
||
| If the spec can be found among the confighured binary mirrors, an |
gartung
approved these changes
Sep 15, 2020
tldahlgren
reviewed
Sep 15, 2020
| """ | ||
|
|
||
| def __init__(self, cache_root=None): | ||
| self._cache_root = default_manager_root |
Contributor
There was a problem hiding this comment.
Recommend lines 60-62:
Suggested change
| self._cache_root = default_manager_root | |
| self._cache_root = cache_root or default_manager_root |
tldahlgren
reviewed
Sep 15, 2020
| cache_path = self._index_file_cache.cache_path(cache_key) | ||
|
|
||
| self._local_index_cache = {} | ||
| if os.path.exists(cache_path) and os.path.isfile(cache_path): |
Contributor
There was a problem hiding this comment.
It is not necessary to call exists and isfile on the same path. Suggest simply checking isfile to avoid the extra function call:
Suggested change
| if os.path.exists(cache_path) and os.path.isfile(cache_path): | |
| if os.path.isfile(cache_path): |
since
scottwittenburg
force-pushed
the
add-binary-distribution-cache-manager
branch
2 times, most recently
from
de92621 to
7449cad
Compare
tldahlgren
reviewed
Sep 25, 2020
| if os.path.isfile(cache_path): | ||
| with self._index_file_cache.read_transaction( | ||
| cache_key) as cache_file: | ||
| self._local_index_cache = json.load(cache_file) |
Contributor
There was a problem hiding this comment.
It would be good to have a test that covers this line since this is new code.
tldahlgren
reviewed
Sep 25, 2020
| locally_computed_hash = compute_hash(index_object_str) | ||
|
|
||
| if fetched_hash is not None and locally_computed_hash != fetched_hash: | ||
| msg_tmpl = ('Computed hash ({0}) did not match remote ({1}), ' |
Contributor
There was a problem hiding this comment.
Can you add a test to exercise this code?
tldahlgren
reviewed
Sep 25, 2020
| if spec_dag_hash not in self._built_spec_cache: | ||
| self._built_spec_cache[spec_dag_hash] = found_list | ||
| else: | ||
| current_list = self._built_spec_cache[spec_dag_hash] |
Contributor
There was a problem hiding this comment.
Can you add a test to exercise this path?
tldahlgren
reviewed
Sep 25, 2020
| current_list = self._built_spec_cache[spec_dag_hash] | ||
| for new_entry in found_list: | ||
| for cur_entry in current_list: | ||
| if new_entry['mirror_url'] == cur_entry['mirror_url']: |
Contributor
There was a problem hiding this comment.
Can you add unit tests to cover these paths?
tldahlgren
requested changes
Sep 25, 2020
Contributor
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
LGTM overall. While coverage is within the allowance, there are a few key places (marked) where it would be good to have unit tests exercising the (new) code.
Contributor
Author
|
Thanks @tldahlgren I'll work on covering the lines you indicated. |
tldahlgren
approved these changes
Sep 25, 2020
Contributor
tldahlgren
left a comment
tldahlgren
left a comment
There was a problem hiding this comment.
Extra tests for increasing patch coverage would be good to have; however, not worth holding up other important work.
This key used to live in the buildcache_spec schema. This change moves it into the spec schema so it can exist with each spec anywhere that schema is used (just buildcache_spec and database_index at the moment).
This change adds a class which provides two levels of caching to the module. At one level, it caches buildcache index files from mirrors, and uses a hash of the index contents to track whether the local caches need to be updates. Then the cached index files can be used to answer questions like "what binary specs are available to me, based on my configured mirrors". In that case, the cached index files are used to populate the other level of cache, the built spec cache. The built spec cache in the cache manager replaces the previous global "_cached_specs" variable with a cache that keeps track of which mirrors contains which concrete specs. However, if this cache does not contain a target spec, the direct approach to fetching a spec.yaml from a mirror is still supported. One goal of this work is to support a new install option which will tell spack to avoid installation from binary unless the full_hash of the binary spec on te mirror mathes that of the local concrete spec.
scottwittenburg
force-pushed
the
add-binary-distribution-cache-manager
branch
from
7449cad to
a1b07aa
Compare
This option defaults to false, preserving the previous behavior, which was to install the first binary package found that matched the DAG hash of the target spec. If this option is provided, spack will install the spec from source if it cannot find a binary spec on a remote mirror with a full_hash that matches that of the local target spec.
scottwittenburg
force-pushed
the
add-binary-distribution-cache-manager
branch
from
a1b07aa to
45e1d82
Compare
robertu94
pushed a commit
to robertu94/spack
that referenced
this pull request
Oct 2, 2020
…bution-cache-manager Add binary distribution cache manager
scottwittenburg
added a commit
that referenced
this pull request
Oct 2, 2020
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from #18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
scottwittenburg
added a commit
that referenced
this pull request
Oct 2, 2020
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from #18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
scottwittenburg
added a commit
that referenced
this pull request
Oct 2, 2020
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from #18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
3 tasks
scottwittenburg
added a commit
that referenced
this pull request
Oct 5, 2020
…tribution-cache-manager" This reverts commit 075c3e0.
scheibelp
pushed a commit
that referenced
this pull request
Oct 5, 2020
This reverts #18359 and follow-on PRs intended to address issues with #18359 because that PR changes the hash of all specs. A future PR will reintroduce the changes. * Revert "Fix location in spec.yaml where we look for full_hash (#19132)" * Revert "Fix fetch of spec.yaml files from buildcache (#19101)" * Revert "Merge pull request #18359 from scottwittenburg/add-binary-distribution-cache-manager"
scottwittenburg
added a commit
that referenced
this pull request
Oct 6, 2020
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from #18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
scottwittenburg
added a commit
to scottwittenburg/spack
that referenced
this pull request
Oct 16, 2020
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from spack#18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
scottwittenburg
added a commit
to scottwittenburg/spack
that referenced
this pull request
Oct 23, 2020
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from spack#18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
scottwittenburg
added a commit
to scottwittenburg/spack
that referenced
this pull request
Oct 29, 2020
When we attempt to determine whether a remote spec (in a binary mirror) is up-to-date or needs to be rebuilt, we compare the full_hash stored in the remote spec.yaml file against the full_hash computed from the local concrete spec. Since the full_hash moved into the spec (and is no longer at the top level of the spec.yaml), we need to look there for it. This oversight from spack#18359 was causing all specs to get rebuilt when the full_hash wasn't fouhd at the expected location.
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
4 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.
Provide the same features/benefits of #18212 but without having to change the spec hash and equality operators.