Skip to content

Spec.splice: Allow splices when multiples nodes in the DAG share a name#46382

Merged
tgamblin merged 34 commits intodevelopfrom
bugfix/splice-multiple-nodes-same-name
Sep 27, 2024
Merged

Spec.splice: Allow splices when multiples nodes in the DAG share a name#46382
tgamblin merged 34 commits intodevelopfrom
bugfix/splice-multiple-nodes-same-name

Conversation

@becker33
Copy link
Copy Markdown
Member

The current Spec.splice model 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.splice to 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.

splice

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

@spackbot-app spackbot-app bot added commands core PR affects Spack core functionality dependencies new-version stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) labels Sep 13, 2024
@becker33
Copy link
Copy Markdown
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 self, or the left hand side, for an intransitive splice).

@becker33 becker33 force-pushed the bugfix/splice-multiple-nodes-same-name branch from 378166a to ba2a2a7 Compare September 17, 2024 19:53
Copy link
Copy Markdown
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

LGTM -- one comment that doesn't need to be addressed here.

@tgamblin tgamblin merged commit 07e964c into develop Sep 27, 2024
@tgamblin tgamblin deleted the bugfix/splice-multiple-nodes-same-name branch September 27, 2024 20:58
f_blue._add_dependency(e_blue, depflag=depflag, virtuals=())
f_blue._add_dependency(g3_blue, depflag=depflag, virtuals=())

c_red = Spec("c color=red")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands core PR affects Spack core functionality dependencies new-version stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants