bugfix: don't fetch package metadata for unknown concrete specs#36990
Merged
bugfix: don't fetch package metadata for unknown concrete specs#36990
Conversation
Member
Author
|
@eugeneswalker: does this fix your issue? |
Contributor
|
This PR does indeed resolve #36847. Thank you! |
Member
Author
|
@eugeneswalker see if the second commit fixes #34108. |
fe8e8e4 to
0106478
Compare
alalazo
previously approved these changes
Apr 18, 2023
lib/spack/spack/solver/asp.py
Outdated
| # Ensure Spack will not coconcretize this with another provider | ||
| # for the same virtual | ||
| for virtual in dep.package.virtuals_provided: | ||
| # Skip virtual node constriants for renamed/deleted packages, |
Member
There was a problem hiding this comment.
Suggested change
| # Skip virtual node constriants for renamed/deleted packages, | |
| # Skip virtual node constraints for renamed/deleted packages, |
lib/spack/spack/solver/asp.py
Outdated
Comment on lines
+1571
to
+1572
| # NOTE: with current specs (which lack edge attributes) this | ||
| # can allow concretizations with two providers, but it's unlikely. |
Member
There was a problem hiding this comment.
This is a bit scary 😟 Not sure what we could do more than this though. I agree it's unlikely, since one of the provider must be in a buildcache and not in the current repository.
Member
Author
There was a problem hiding this comment.
I think we don't rename things (especially MPI and other virtuals) frequently enough for this to matter. Though @eugeneswalker and the E4S team are trying hard to make it matter with their 100,000 package buildcache.
Goal should be to get virtual info on edges before it matters 😅
Member
Author
There was a problem hiding this comment.
I think the value of reuse here outweighs the corner case.
The concretizer can fail with `reuse:true` if a buildcache or installation contains a package with a dependency that has been renamed or deleted in the main repo (e.g., `netcdf` was refactored to `netcdf-c`, `netcdf-fortran`, etc., but there are still binary packages with dependencies called `netcdf`). We should still be able to install things for which we are missing `package.py` files. `Spec.inject_patches_variant()` was failing this requirement by attempting to look up the package class for concrete specs. This isn't needed -- we can skip it. - [x] swap two conditions in `Spec.inject_patches_variant()`
`spec_clauses()` attempts to look up package information for concrete specs in order to determine which virtuals they may provide. This fails for renamed/deleted dependencies of buildcaches and installed packages. This will eventually be fixed by #35258, which adds virtual information on edges, but we need a workaround to make older buildcaches usable. - [x] make an exception for renamed packages and omit their virtual constraints - [x] add a note that this will be solved by adding virtuals to edges
We currently throw a nasty error if you try to reuse packages from some other namespace (e.g., OLCF), but we should be able to reuse patched local versions of builtin packages. Right now the only obstacle to that is that we try to look up virtual info for unknown namespaces, and we can't get the package from the repo to do that. We *can* assume that a package with a known namespace is similar, and that its virtual provider information is reasonably accurate, so we now do that. This isn't 100% accurate, but neither is relying on the package itself, as it may have gone out of date. The real solution here is virtual edge information, but this is a stopgap until we have that.
a892475 to
7517ed5
Compare
alalazo
approved these changes
May 15, 2023
Contributor
This was referenced May 15, 2023
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.
Fixes #36847.
Fixes #34108.
This fixes two issues:
The concretizer can fail with
reuse:trueif a buildcache or installation contains a package with a dependency that has been renamed or deleted in the main repo (e.g.,netcdfwas refactored tonetcdf-c,netcdf-fortran, etc., but there are still binary packages with dependencies callednetcdf).We should still be able to install things for which we are missing
package.pyfiles.Spec.inject_patches_variant()was failing this requirement by attempting to look up the package class for concrete specs. This isn't needed -- we can skip it.spec_clauses()attempts to look up package information for concrete specs in order to determine which virtuals they may provide. This fails for renamed/deleted dependencies of buildcaches and installed packages.This will eventually be fixed by Add virtual information on DAG edges #35258, but we need a workaround to make older buildcaches usable.
Spec.inject_patches_variant()