Skip to content

Toolchains: parse-time aliases for dependencies#50481

Merged
tgamblin merged 84 commits intodevelopfrom
features/toolchains
Jun 6, 2025
Merged

Toolchains: parse-time aliases for dependencies#50481
tgamblin merged 84 commits intodevelopfrom
features/toolchains

Conversation

@becker33
Copy link
Copy Markdown
Member

@becker33 becker33 commented May 15, 2025

This PR includes 2 primary components:

Conditional dependencies

Conditional dependencies can be expressed with a when= clause on the dependency edge attributes

For example:

hdf5 %[when=+fortran virtuals=fortran] gcc ^[when=^mpi, virtuals=mpi ] mpich

means that hdf5 depends on mpich for mpi when it depends on mpi, and depends on gcc for fortran when the fortran variant is turned on.

You may need to quote this on the CLI, like so:

> spack spec "hdf5 %[when=+fortran virtuals=fortran] gcc ^[when=^mpi, virtuals=mpi ] mpich"

Toolchains

Toolchains are parse-time aliases for dependencies. This allows complex dependency relationships to be encoded by simple aliases. The toolchains feature is designed to facilitate the use-case previously allowed by configuring compilers to have the fortran compiler from a different compiler set -- toolchains are however more general, and can apply to any dependencies.

Toolchains are configured in the toolchains config section, e.g.

toolchains:
  clang_gfortran: 
      - spec: %[virtuals=c] clang
        when: %c
      - spec: %[virtuals=cxx] clang
        when: %cxx
      - spec: %[virtuals=fortran] gcc
        when: %fortran
  gcc_mpich:
      - spec: %[virtuals=c] gcc
        when: %c
      - spec: %[virtuals=cxx] gcc
        when: %cxx
      - spec: %[virtuals=fortran] gcc
        when: %fortran
      - spec: %[virtuals=mpi] mpich
        when: %mpi

With this configuration, you can now use %clang_gfortran and %gcc_mpich as you would a compiler. You can add flags, e.g.:

gcc_mpich:
    - spec: cflags="-O3 -g"
    - spec: cxxflags="-O3 -g"
    - spec: fflags="-O3 -g"
    - spec: %[virtuals=c] gcc
      when: %c
    - spec: %[virtuals=cxx] gcc
      when: %cxx
    - spec: %[virtuals=fortran] gcc
      when: %fortran
    - spec: %[virtuals=mpi] mpich
      when: %mpi

The clang_gfortran toolchain is roughly equivalent to configuring a Spack v0.* compiler to have clang executables for c/cxx and gfortran for fortran. The gcc_mpich toolchain uses only gcc, but is extended to also include the mpi implementation to use.

You can currently only use % (direct dependency) in toolchain conditions, though we are working on enabling ^ conditions in a later PR.

You can write the same toolchains with conditional dependencies, but it's much harder to read:

toolchains:
  clang_gfortran: "%[when='%c' virtuals=c]clang %[when='%cxx' virtuals=cxx]clang %[when='%fortran' virtuals=fortran]gcc"
  gcc_mpich: "%[when='%c' virtuals=c]gcc %[when='%cxx' virtuals=cxx]gcc %[when='%fortran' virtuals=fortran]gcc %[when='%mpi' virtuals=mpi] mpich"

Some notes on using toolchains:

  • You likely want to use conditional dependencies as shown above. If you use unconditional dependencies, it will turn on variants to ensure the compiler can be used, and will fail for packages that don't depend on a given language.
  • For similar reasons, it's best to separately specify each condition for shared virtuals like c/cxx/fortran from the same package.

TODO:

  • unit tests for prefers/requires with conditional dependencies
  • unit tests for constraints with conditional dependencies (partially tested by concretization tests)
  • unit tests for satisfies with conditional dependencies (partially tested by concretization tests)
  • documentation

@spackbot-app spackbot-app bot added core PR affects Spack core functionality tests General test capability(ies) shell-support stand-alone-tests Stand-alone (or smoke) tests for installed packages dependencies new-variant new-version labels May 15, 2025
@tgamblin tgamblin self-requested a review May 21, 2025 06:49
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

I gave a first fast read to the PR. Will test drive it soon. A few comments:

  1. There seems to be a violation of the subset semantic by conditional specs, see the comment below.
  2. I think toolchains should be expanded only from %, and if possible the expansion shouldn't be so closely tied to the parser. Right now the spec parser depends on CONFIG, before it didn't.
  3. The motivation to only expand from % is that we are otherwise making ^ position dependent in the spec, whereas before it was not (e.g. foo ^bar ^toolchain might be different from foo ^toolchain ^bar, which is highly unexpected)

I also wonder how the when= argument that we added in many Spec related methods might relate to conditional dependencies expressed with depends_on(..., when=...) in packages, but that might be postponed to a post v1.0 discussion, assuming we do not expose these new methods to the package API (need to double check that).

I pointed out a few opportunities to extract trivial refactors, and reduce the amount of changes in this PR "unrelated" to the feature.

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented May 21, 2025

I agree with @alalazo's points, and I think we should restrict it just to % like he says.

You say:

Toolchains are parse-time aliases for dependencies.
but I think users are going to want flags and maybe variants in here too. Is that possible?

@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented May 21, 2025

I guess my other major comment is that the quoting here is going to put users off:

spack spec hdf5 %[when=\'+fortran\' virtuals=fortran] gcc ^[when=\'^mpi\', virtuals=mpi ] mpich

How can we avoid the '? I don't think that should be necessary. If the user can quote the whole thing it's much better:

spack spec "hdf5 %[when='+fortran' virtuals=fortran] gcc ^[when='^mpi', virtuals=mpi ] mpich"

but I don't really get why +fortran or ^mpi should require quotes. Are you just quoting that in case there are spaces in the when spec?

More relevantly, I think the CLI syntax for conditional specs isn't really necessary for the place where users are most likely to specify this stuff. Yes we need a CLI syntax but I think the YAML specification can be cleaned up to:

toolchains:
  clang_gfortran: 
      - spec: %c=clang
        when: %c
      - spec: %cxx=clang
        when: %cxx
      - spec: %fortran=gcc
        when: %fortran
  gcc_mpich:
      - spec: %c=gcc
        when: %c
      - spec: %cxx=gcc
        when: %cxx
      - spec: %fortran=gcc
        when: %fortran
      - spec: ^mpi=mpich
        when: ^mpi

If that constructs conditional specs behind the scenes, it's fine.

Note that the above is using the new syntax I'm developing separately -- in this PR that would be:

toolchains:
  clang_gfortran: 
      - spec: %[virtuals=c] clang
        when: %c
      - spec: %[virtuals=cxx] clang
        when: %cxx
      - spec: %[virtuals=fortran] gcc
        when: %fortran
  gcc_mpich:
      - spec: %[virtuals=c] gcc
        when: %c
      - spec: %[virtuals=cxx] gcc
        when: %cxx
      - spec: %[virtuals=fortran] gcc
        when: %fortran
      - spec: ^[virtuals=mpi] mpich
        when: ^mpi

which is still much cleaner and easier to read

@becker33 becker33 force-pushed the features/toolchains branch 3 times, most recently from 21ec146 to bdab3a6 Compare May 28, 2025 22:38
@tgamblin tgamblin force-pushed the features/toolchains branch from dcb3f1e to 737be22 Compare May 29, 2025 07:06
@alalazo
Copy link
Copy Markdown
Member

alalazo commented May 29, 2025

Just timed the current PR, using as base commit:

  • Spack: 1.0.0.dev0 (3f33c71)
  • Python: 3.13.0
  • Platform: linux-ubuntu20.04-icelake

radiuss.develop.csv
radiuss.pr.csv

radiuss

I see some slowdown in grounding, sometimes, but nothing too drastic.

@alalazo alalazo force-pushed the features/toolchains branch from 737be22 to 7d18e29 Compare May 29, 2025 12:19
@alalazo

This comment was marked as outdated.

@alalazo alalazo force-pushed the features/toolchains branch from 31f8991 to a14fe6b Compare May 29, 2025 14:16
alalazo

This comment was marked as outdated.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented May 30, 2025

Dealing with the parser error here: #50714

@alalazo alalazo force-pushed the features/toolchains branch from a14fe6b to 9c48925 Compare May 30, 2025 12:34
self.depflag = depflag
self.virtuals = tuple(sorted(set(virtuals)))
self.direct = direct
self.when = when or Spec()
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.

I'd rather set this initially to None to avoid extra object construction for every dependency on every spec -- does it hugely complicate things to do that? I realize it's nicer programmatically to use a bare Spec()

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It requires adding a conditional everywhere we check edge.when.satisfies() or call edge.when.constrain(). I don't think calling the spec constructor is relatively that expensive.

Comment on lines +1061 to +1062
if when is not None:
selected = (dep for dep in selected if dep.when == when)
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.

Why do dep.when == when here and not dep.when.satisfies(when)? I think the latter makes a lot more sense given the way we typically treat specs in the internal APIs. I could also see someone asking for, say, all the conditional deps that would apply to versions @4: -- and you'll miss the ones that are just @4 with ==.

Only thing I could think of this affecting is if you call select or some other method that calls with with when=Spec() to get the unconditional deps, but it looks like you check that explicitly with dep.when == Spec() everywhere it would be needed in this PR.

name: filter dependencies by package name
deptype: allowed dependency types
virtuals: allowed virtuals
when: condition on conditional dependency or Spec() for unconditional
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.

I find this description really hard to parse. how about:

Suggested change
when: condition on conditional dependency or Spec() for unconditional
when: optional spec to select matching dependency conditions

haampie
haampie previously requested changes Jun 2, 2025
Copy link
Copy Markdown
Member

@haampie haampie left a comment

Choose a reason for hiding this comment

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

blocking this over satisfies semantics #50481 (comment)

@alalazo alalazo force-pushed the features/toolchains branch 2 times, most recently from 38e4ad6 to 6d68c73 Compare June 2, 2025 10:22
Signed-off-by: Gregory Becker <becker33@llnl.gov>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
alalazo added 4 commits June 5, 2025 10:28
* Test the correct rhs edge in satisfies
* Do not fail concretization if a conditional dep
  refers to a non-existing variant

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
@alalazo alalazo force-pushed the features/toolchains branch from 6aa3d4c to e404aec Compare June 5, 2025 14:12
becker33 and others added 2 commits June 5, 2025 10:55
Signed-off-by: Gregory Becker <becker33@llnl.gov>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
@alalazo
Copy link
Copy Markdown
Member

alalazo commented Jun 5, 2025

Last version, always benchmarked against:

  • Spack: 1.0.0.dev0 (d05ac02)
  • Python: 3.13.0
  • Platform: linux-ubuntu20.04-icelake

radiuss.develop.20250605.csv
radiuss.pr.20250605.csv

radiuss

Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
@alalazo alalazo force-pushed the features/toolchains branch from a6665aa to 48b2462 Compare June 5, 2025 21:27
alalazo
alalazo previously approved these changes Jun 5, 2025
@alalazo
Copy link
Copy Markdown
Member

alalazo commented Jun 5, 2025

To be done in following PRs (likely after v1.0):

Also cleaned up outdated references to compilers as separate from
dependencies and ordering of example specs

Signed-off-by: Gregory Becker <becker33@llnl.gov>
@spackbot-app spackbot-app bot added the documentation Improvements or additions to documentation label Jun 6, 2025
@tgamblin tgamblin merged commit 2012ed8 into develop Jun 6, 2025
34 of 35 checks passed
@tgamblin tgamblin deleted the features/toolchains branch June 6, 2025 06:14
@tgamblin tgamblin added this to the v1.0.0 milestone Jun 6, 2025
Comment on lines +1168 to +1174
Direct dependencies specified with ``%`` also differ from general
dependencies because they associate with the most recent node, rather
than with the root of the DAG. So in the spec ``root ^dep1 ^dep2
^dep3`` all three dependencies are associated with the package
``root``, but in the spec ``root ^dep1 %dep2 %dep3`` the spec
``%dep2`` is associated with ``dep1`` and the spec ``%dep3`` is
associated with ``dep2``.
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.

Just a note that this is wrong. I'm fixing this in #50932

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 documentation Improvements or additions to documentation new-variant new-version shell-support 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.

4 participants