bugfix: original concretizer is sensitive to dependency order#41975
Merged
bugfix: original concretizer is sensitive to dependency order#41975
Conversation
Needed for #40326, which can changes the iteration order over package dependencies during concretization. While clingo doesn't have this problem, the original concretizer (which we still use for bootstrapping) can be sensitive to iteration order when evaluating dependency constraints in `when` conditions. This can cause it to ignore conditional dependencies unless the dependencies in the condition are lsited first in the package. The issue was in the way the original concretizer would disconnect specs *every* time `normalize()` ran. When specs were disconnected, `^dependency` constraints wouldn't see the dependency in the dependency condition loop. - [x] refactor `flat_dependencies()` to not disconnect the spec by default. - [x] disconnect only once at the beginning of `normalize()` or `concretize()`. - [x] add a test that perturbs dependency iteration order to ensure this doesn't regress.
spackbot-app
bot
added
core
tgamblin
force-pushed
the
bugfix-original-concretizer-ignores-dep-constraints
branch
from
05c0253 to
fba3c7f
Compare
tgamblin
added
concretization
bootstrap
alalazo
reviewed
Jan 8, 2024
Member
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
Basically LGTM. Let me know if you want to tweak docstring etc. in following PRs. In that case this can be merged.
Comment on lines
-3132
to
-3133
| if copy: | ||
| spec = spec.copy(deps=False) |
Member
There was a problem hiding this comment.
This seems to be the only semantic change in the function. I tried to inspect closely, and I think it's ok to remove the copy. Do you recall why we added the copy here in the first place?
Member
Author
There was a problem hiding this comment.
It dates back to 2014 in 5f073ae. I guess it lasted not quite ten years :). _dup used to use flat_dependencies() to do its copying, but doesn't anymore. All remaining calls were passing copy=False, so I removed the kwarg.
| if self.satisfies(when_spec): | ||
| if dep is None: | ||
| dep = dp.Dependency(self.name, Spec(name), depflag=0) | ||
| dep = dp.Dependency(Spec(self.name), Spec(name), depflag=0) |
Member
There was a problem hiding this comment.
Even here it seems safe to use a copy instead of self. Reviewing this code makes me appreciate the new concretizer more.
Member
Author
There was a problem hiding this comment.
This change was really just to ensure early Spec parsing (and thus validation). It didn't have a self ref before -- just the name.
Comment on lines
+3432
to
+3438
| # remove any leftover dependents outside the spec from, e.g., pruning externals | ||
| valid = {id(spec) for spec in all_spec_deps.values()} | {id(self)} | ||
| for spec in all_spec_deps.values(): | ||
| remove = [dep for dep in spec.dependents() if id(dep) not in valid] | ||
| for dep in remove: | ||
| del spec._dependents.edges[dep.name] | ||
| del dep._dependencies.edges[spec.name] |
Member
There was a problem hiding this comment.
OT (comment, no action needed): I noticed slight variations of this pattern in many places in the code. I introduced in #40636 a class that helps keeping consistency between objects in memory and specs. See the PR for three similar cases that have been unified in the code where we want to ensure that a set of specs doesn't refere to external specs, and have the same id in memory.
alalazo
approved these changes
Jan 8, 2024
Member
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
On a second thought, let me merge this and submit a tweak to the docstring - that will give us a fix in develop sooner 🙂
tgamblin
deleted the
bugfix-original-concretizer-ignores-dep-constraints
branch
RikkiButler20
pushed a commit
to RikkiButler20/spack
that referenced
this pull request
Jan 25, 2024
…41975) Needed for spack#40326, which can changes the iteration order over package dependencies during concretization. While clingo doesn't have this problem, the original concretizer (which we still use for bootstrapping) can be sensitive to iteration order when evaluating dependency constraints in `when` conditions. This can cause it to ignore conditional dependencies unless the dependencies in the condition are listed first in the package. The issue was in the way the original concretizer would disconnect specs *every* time `normalize()` ran. When specs were disconnected, `^dependency` constraints wouldn't see the dependency in the dependency condition loop. We now only only disconnect *all* dependencies at the start of `concretize()` and `normalize()`, and we disconnect any leftover dependents from replaced externals at the *end* of `normalize()`. This trims stale connections while keeping the ones that are needed to trigger dependency conditions. - [x] refactor `flat_dependencies()` to not disconnect the spec by default. - [x] `flat_dependencies()` is never called with `copy=True` -- remove the `copy` kwarg. - [x] disconnect only once at the beginning of `normalize()` or `concretize()`. - [x] add a test that perturbs dependency iteration order to ensure this doesn't regress. - [x] disconnect unused dependents at end of `normalize()`
RikkiButler20
pushed a commit
to RikkiButler20/spack
that referenced
this pull request
Jan 31, 2024
…41975) Needed for spack#40326, which can changes the iteration order over package dependencies during concretization. While clingo doesn't have this problem, the original concretizer (which we still use for bootstrapping) can be sensitive to iteration order when evaluating dependency constraints in `when` conditions. This can cause it to ignore conditional dependencies unless the dependencies in the condition are listed first in the package. The issue was in the way the original concretizer would disconnect specs *every* time `normalize()` ran. When specs were disconnected, `^dependency` constraints wouldn't see the dependency in the dependency condition loop. We now only only disconnect *all* dependencies at the start of `concretize()` and `normalize()`, and we disconnect any leftover dependents from replaced externals at the *end* of `normalize()`. This trims stale connections while keeping the ones that are needed to trigger dependency conditions. - [x] refactor `flat_dependencies()` to not disconnect the spec by default. - [x] `flat_dependencies()` is never called with `copy=True` -- remove the `copy` kwarg. - [x] disconnect only once at the beginning of `normalize()` or `concretize()`. - [x] add a test that perturbs dependency iteration order to ensure this doesn't regress. - [x] disconnect unused dependents at end of `normalize()`
kwryankrattiger
pushed a commit
to kwryankrattiger/spack
that referenced
this pull request
Jul 9, 2024
…41975) Needed for spack#40326, which can changes the iteration order over package dependencies during concretization. While clingo doesn't have this problem, the original concretizer (which we still use for bootstrapping) can be sensitive to iteration order when evaluating dependency constraints in `when` conditions. This can cause it to ignore conditional dependencies unless the dependencies in the condition are listed first in the package. The issue was in the way the original concretizer would disconnect specs *every* time `normalize()` ran. When specs were disconnected, `^dependency` constraints wouldn't see the dependency in the dependency condition loop. We now only only disconnect *all* dependencies at the start of `concretize()` and `normalize()`, and we disconnect any leftover dependents from replaced externals at the *end* of `normalize()`. This trims stale connections while keeping the ones that are needed to trigger dependency conditions. - [x] refactor `flat_dependencies()` to not disconnect the spec by default. - [x] `flat_dependencies()` is never called with `copy=True` -- remove the `copy` kwarg. - [x] disconnect only once at the beginning of `normalize()` or `concretize()`. - [x] add a test that perturbs dependency iteration order to ensure this doesn't regress. - [x] disconnect unused dependents at end of `normalize()`
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
2 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.
Needed for #40326, which can changes the iteration order over package dependencies during concretization.
While clingo doesn't have this problem, the original concretizer (which we still use for bootstrapping) can be sensitive to iteration order when evaluating dependency constraints in
whenconditions. This can cause it to ignore conditional dependencies unless the dependencies in the condition are listed first in the package.The issue was in the way the original concretizer would disconnect specs every time
normalize()ran. When specs were disconnected,^dependencyconstraints wouldn't see the dependency in the dependency condition loop.We now only only disconnect all dependencies at the start of
concretize()andnormalize(), and we disconnect any leftover dependents from replaced externals at the end ofnormalize(). This trims stale connections while keeping the ones that are needed to trigger dependency conditions.flat_dependencies()to not disconnect the spec by default.flat_dependencies()is never called withcopy=True-- remove thecopykwarg.normalize()orconcretize().normalize()