Toolchains: parse-time aliases for dependencies#50481
Conversation
There was a problem hiding this comment.
I gave a first fast read to the PR. Will test drive it soon. A few comments:
- There seems to be a violation of the subset semantic by conditional specs, see the comment below.
- 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. - 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 ^toolchainmight be different fromfoo ^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.
|
I agree with @alalazo's points, and I think we should restrict it just to You say:
|
|
I guess my other major comment is that the quoting here is going to put users off: 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: 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: ^mpiIf 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: ^mpiwhich is still much cleaner and easier to read |
21ec146 to
bdab3a6
Compare
dcb3f1e to
737be22
Compare
|
Just timed the current PR, using as base commit:
radiuss.develop.csv I see some slowdown in grounding, sometimes, but nothing too drastic. |
737be22 to
7d18e29
Compare
This comment was marked as outdated.
This comment was marked as outdated.
31f8991 to
a14fe6b
Compare
|
Dealing with the parser error here: #50714 |
a14fe6b to
9c48925
Compare
| self.depflag = depflag | ||
| self.virtuals = tuple(sorted(set(virtuals))) | ||
| self.direct = direct | ||
| self.when = when or Spec() |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
lib/spack/spack/spec.py
Outdated
| if when is not None: | ||
| selected = (dep for dep in selected if dep.when == when) |
There was a problem hiding this comment.
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.
lib/spack/spack/spec.py
Outdated
| name: filter dependencies by package name | ||
| deptype: allowed dependency types | ||
| virtuals: allowed virtuals | ||
| when: condition on conditional dependency or Spec() for unconditional |
There was a problem hiding this comment.
I find this description really hard to parse. how about:
| when: condition on conditional dependency or Spec() for unconditional | |
| when: optional spec to select matching dependency conditions |
haampie
left a comment
There was a problem hiding this comment.
blocking this over satisfies semantics #50481 (comment)
38e4ad6 to
6d68c73
Compare
Signed-off-by: Gregory Becker <becker33@llnl.gov>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
* 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>
6aa3d4c to
e404aec
Compare
Signed-off-by: Gregory Becker <becker33@llnl.gov>
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
|
Last version, always benchmarked against:
|
Signed-off-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>
a6665aa to
48b2462
Compare
|
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>
| 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``. |
There was a problem hiding this comment.
Just a note that this is wrong. I'm fixing this in #50932


This PR includes 2 primary components:
Conditional dependencies
Conditional dependencies can be expressed with a
when=clause on the dependency edge attributesFor example:
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
toolchainsconfig section, e.g.With this configuration, you can now use
%clang_gfortranand%gcc_mpichas you would a compiler. You can add flags, e.g.:The
clang_gfortrantoolchain is roughly equivalent to configuring a Spack v0.* compiler to have clang executables for c/cxx and gfortran for fortran. Thegcc_mpichtoolchain 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:
Some notes on using toolchains:
TODO: