Spec.splice: Allow splices when multiples nodes in the DAG share a name#46382
Merged
Spec.splice: Allow splices when multiples nodes in the DAG share a name#46382
Conversation
Member
Author
|
After further consideration, the intransitive splice in the image is incorrect. The blue g3 node should not appear in the resulting spec, and all of its incoming edges should go to the red g2 node (marked as Xed out in the image) instead. This is truer to the intended contract for an intransitive splice (all conflicting dependencies are resolved to |
updates intransitive splice with ideas learned implementing transitive splice algorithm is simpler in complexity than previous updates intransitive splice test for new semantics regarding duplicate nodes
378166a to
ba2a2a7
Compare
tgamblin
requested changes
Sep 26, 2024
test split dependencies for build and link deps test that invalid dependency splits fail
tgamblin
reviewed
Sep 27, 2024
Member
tgamblin
left a comment
There was a problem hiding this comment.
LGTM -- one comment that doesn't need to be addressed here.
tgamblin
approved these changes
Sep 27, 2024
alalazo
reviewed
Sep 30, 2024
| f_blue._add_dependency(e_blue, depflag=depflag, virtuals=()) | ||
| f_blue._add_dependency(g3_blue, depflag=depflag, virtuals=()) | ||
|
|
||
| c_red = Spec("c color=red") |
Member
There was a problem hiding this comment.
We should use pkg-a, pkg-b, pkg-c etc. since now c is also a valid dependency, and will eventually exist also in the builtin.mock repo. I'll submit a PR to fix these tests, but fyi.
alalazo
added a commit
to alalazo/spack
that referenced
this pull request
Sep 30, 2024
spack#45205 already removed previous use of single letter packages from unit tests, in view of reserving `c` as a language (see spack#45191). Some use of them has been re-introduced accidentally in spack#46382, and is making unit-tests fail in the feature branch spack#45189 since there `c` is a virtual package. Signed-off-by: Massimiliano Culpo <[email protected]>
tldahlgren
pushed a commit
that referenced
this pull request
Oct 1, 2024
#45205 already removed previous use of single letter packages from unit tests, in view of reserving `c` as a language (see #45191). Some use of them has been re-introduced accidentally in #46382, and is making unit-tests fail in the feature branch #45189 since there `c` is a virtual package. Signed-off-by: Massimiliano Culpo <[email protected]>
becker33
added a commit
that referenced
this pull request
Oct 10, 2024
This PR allows users to configure explicit splicing replacement of an abstract spec in the concretizer.
concretizer:
splice:
explicit:
- target: mpi
replacement: mpich/abcdef
transitive: true
This config block would mean "for any spec that concretizes to use mpi, splice in mpich/abcdef in place of the mpi it would naturally concretize to use. See #20262, #26873, #27919, and #46382 for PRs enabling splicing in the Spec object. This PR will be the first place the splice method is used in a user-facing manner. See https://spack.readthedocs.io/en/latest/spack.html#spack.spec.Spec.splice for more information on splicing.
This will allow users to reuse generic public binaries while splicing in the performant local mpi implementation on their system.
In the config file, the target may be any abstract spec. The replacement must be a spec that includes an abstract hash `/abcdef`. The transitive key is optional, defaulting to true if left out.
Two important items to note:
1. When writing explicit splice config, the user is in charge of ensuring that the replacement specs they use are binary compatible with whatever targets they replace. In practice, this will likely require either specific knowledge of what packages will be installed by the user's workflow, or somewhat more specific abstract "target" specs for splicing, to ensure binary compatibility.
2. Explicit splices can cause the output of the concretizer not to satisfy the input. For example, using the config above and consider a package in a binary cache `hdf5/xyzabc` that depends on mvapich2. Then the command `spack install hdf5/xyzabc` will instead install the result of splicing `mpich/abcdef` into `hdf5/xyzabc` in place of whatever mvapich2 spec it previously depended on. When this occurs, a warning message is printed `Warning: explicit splice configuration has caused the the concretized spec {concrete_spec} not to satisfy the input spec {input_spec}".
Highlighted technical details of implementation:
1. This PR required modifying the installer to have two separate types of Tasks, `RewireTask` and `BuildTask`. Spliced specs are queued as `RewireTask` and standard specs are queued as `BuildTask`. Each spliced spec retains a pointer to its build_spec for provenance. If a RewireTask is dequeued and the associated `build_spec` is neither available in the install_tree nor from a binary cache, the RewireTask is requeued with a new dependency on a BuildTask for the build_spec, and BuildTasks are queued for the build spec and its dependencies.
2. Relocation is modified so that a spack binary can be simultaneously installed and rewired. This ensures that installing the build_spec is not necessary when splicing from a binary cache.
3. The splicing model is modified to more accurately represent build dependencies -- that is, spliced specs do not have build dependencies, as spliced specs are never built. Their build_specs retain the build dependencies, as they may be built as part of installing the spliced spec.
4. There were vestiges of the compiler bootstrapping logic that were not removed in #46237 because I asked alalazo to leave them in to avoid making the rebase for this PR harder than it needed to be. Those last remains are removed in this PR.
Co-authored-by: Nathan Hanford <[email protected]>
Co-authored-by: Gregory Becker <[email protected]>
Co-authored-by: Tamara Dahlgren <[email protected]>
arezaii
pushed a commit
to arezaii/spack
that referenced
this pull request
Oct 22, 2024
spack#45205 already removed previous use of single letter packages from unit tests, in view of reserving `c` as a language (see spack#45191). Some use of them has been re-introduced accidentally in spack#46382, and is making unit-tests fail in the feature branch spack#45189 since there `c` is a virtual package. Signed-off-by: Massimiliano Culpo <[email protected]>
arezaii
pushed a commit
to arezaii/spack
that referenced
this pull request
Oct 22, 2024
This PR allows users to configure explicit splicing replacement of an abstract spec in the concretizer.
concretizer:
splice:
explicit:
- target: mpi
replacement: mpich/abcdef
transitive: true
This config block would mean "for any spec that concretizes to use mpi, splice in mpich/abcdef in place of the mpi it would naturally concretize to use. See spack#20262, spack#26873, spack#27919, and spack#46382 for PRs enabling splicing in the Spec object. This PR will be the first place the splice method is used in a user-facing manner. See https://spack.readthedocs.io/en/latest/spack.html#spack.spec.Spec.splice for more information on splicing.
This will allow users to reuse generic public binaries while splicing in the performant local mpi implementation on their system.
In the config file, the target may be any abstract spec. The replacement must be a spec that includes an abstract hash `/abcdef`. The transitive key is optional, defaulting to true if left out.
Two important items to note:
1. When writing explicit splice config, the user is in charge of ensuring that the replacement specs they use are binary compatible with whatever targets they replace. In practice, this will likely require either specific knowledge of what packages will be installed by the user's workflow, or somewhat more specific abstract "target" specs for splicing, to ensure binary compatibility.
2. Explicit splices can cause the output of the concretizer not to satisfy the input. For example, using the config above and consider a package in a binary cache `hdf5/xyzabc` that depends on mvapich2. Then the command `spack install hdf5/xyzabc` will instead install the result of splicing `mpich/abcdef` into `hdf5/xyzabc` in place of whatever mvapich2 spec it previously depended on. When this occurs, a warning message is printed `Warning: explicit splice configuration has caused the the concretized spec {concrete_spec} not to satisfy the input spec {input_spec}".
Highlighted technical details of implementation:
1. This PR required modifying the installer to have two separate types of Tasks, `RewireTask` and `BuildTask`. Spliced specs are queued as `RewireTask` and standard specs are queued as `BuildTask`. Each spliced spec retains a pointer to its build_spec for provenance. If a RewireTask is dequeued and the associated `build_spec` is neither available in the install_tree nor from a binary cache, the RewireTask is requeued with a new dependency on a BuildTask for the build_spec, and BuildTasks are queued for the build spec and its dependencies.
2. Relocation is modified so that a spack binary can be simultaneously installed and rewired. This ensures that installing the build_spec is not necessary when splicing from a binary cache.
3. The splicing model is modified to more accurately represent build dependencies -- that is, spliced specs do not have build dependencies, as spliced specs are never built. Their build_specs retain the build dependencies, as they may be built as part of installing the spliced spec.
4. There were vestiges of the compiler bootstrapping logic that were not removed in spack#46237 because I asked alalazo to leave them in to avoid making the rebase for this PR harder than it needed to be. Those last remains are removed in this PR.
Co-authored-by: Nathan Hanford <[email protected]>
Co-authored-by: Gregory Becker <[email protected]>
Co-authored-by: Tamara Dahlgren <[email protected]>
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
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.
The current
Spec.splicemodel is very limited by the inability to splice specs that contain multiple nodes with the same name. This is an artifact of the original algorithm design predating the separate concretization of build dependencies, which was the first feature to allow multiple specs in a DAG to share a name.This PR provides a complete reimplementation of
Spec.spliceto avoid that limitation. At the same time, the new algorithm ensures that build dependencies for spliced specs are not changed, since the splice by definition cannot change the build-time information of the spec. This is handled by splitting the dependency edges and link/run edges into separate dependencies as needed.The attached image shows a pair of splices between two specs, with the transitive splice on the lower-left and the intransitive splice on the lower-right. In both cases, only the link/run DAG is considered.
This PR includes tests for these cases to augment the less complex splicing tests that predate it.
@tldahlgren and @tgamblin this follows the algorithm we discussed on 9/10