Skip to content

Allow packages to depend on my.version, variant=my.value, etc.#41700

Draft
tgamblin wants to merge 2 commits intodevelopfrom
sync-dependency-attributes
Draft

Allow packages to depend on my.version, variant=my.value, etc.#41700
tgamblin wants to merge 2 commits intodevelopfrom
sync-dependency-attributes

Conversation

@tgamblin
Copy link
Copy Markdown
Member

Based on #37418 (comment), we want to try to enable something like:

depends_on("geant4-data@{my.version}", when="@10:")

You can think of {my.version} as an expression template that expands out the depends_on() to all possible values of some spec attribute. my here is like a proxy for this package's spec, with a limited set of allowed attributes: basically the ones we know how to expand all values of. I think this makes it intuitive to filter by the when= clause (to be consistent with other directives). It also enables you to, e.g., only do version matching only on certain versions (10 or higher here).

Expressions could be more complex:

depends_on("geant4-data@{my.version.up_to(2)}", when="@10:")

And you could use this on variants as well:

depends_on("hdf5 mpi={my.variants['mpi'].value}")

The final advantage here is that we can push logic down to the solver instead of doing a for loop at the package level. Not all versions, variant values, etc. are known at package definition time. They can come in from the CLI, config, externals, and other places, so only the solver really knows all the values at setup time. Similarly, for variants with infinite legal values (e.g,. ints, strings) we only know the values at solver setup time, and we can't explode the solve to include all possible strings/ints/whatever.

@greenc-FNAL: this is a proof of concept to give you an idea of what parts of the code you'd need to touch to get something like this working. I have some ideas for how you could propagate an actual version/variant expressions from the depends_on() directive, through the spec, down to the solver. Ultimately I think those need to be evaluated in define_version_constraints (or a similar location for variants). That is, they should be run at the end of setup after all values from all specs in the solve are known.

The amount of concretizer logic required here is surprisingly small. It's basically one new type of fact in the encoding:

attr("sync", "dep_name", "node_version_satisfies", attr("version", "pkg_name"))

That says that you should take Version in any derived attr("version", Package, Version) and create a rule like attr("node_version_satisfies", Dependency, Version) to force the dependency to use the same version. The implementation of that is just:

attr(DepAttrName, DepNode, Value)
   :- depends_on(node(X, Package), DepNode),
      attr(PackageAttrName, node(X, Package), Value),
      attr("sync", DepNode, DepAttrName, attr(PackageAttrName, Package)).

- [x] allow caller of `condition()` to pass lists of transforms
- [x] all transform functions now take trigger *and* effect as parameters
- [x] add some utility functions to simplify `condition()`
@tgamblin tgamblin requested review from alalazo and becker33 December 14, 2023 21:58
@spackbot-app spackbot-app bot added core PR affects Spack core functionality dependencies new-version labels Dec 14, 2023
@tgamblin tgamblin changed the title Sync dependency attributes Allow packages to depend on my.version, variant=my.value, etc. Dec 14, 2023
@tgamblin tgamblin marked this pull request as draft December 14, 2023 22:08
@tgamblin tgamblin force-pushed the sync-dependency-attributes branch from 5613e69 to e3fc937 Compare December 14, 2023 22:28
Copy link
Copy Markdown
Member

@scheibelp scheibelp left a comment

Choose a reason for hiding this comment

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

This appears incomplete (not all functionality discussed in the PR description is yet implemented) so I'm going for higher-level comments:

depends_on("geant4-data@{my.version}", when="@10:")

looks reasonable

depends_on("hdf5 mpi={my.variants['mpi'].value}")

makes it look like my is the Spec and you can execute any function you want on it. This seems susceptible to confusion and IMO users would only want to do things like this for a limited set of values (which suggests to me that a limited set of identifiers would be appropriate vs. adding a mechanism to execute arbitrary {})

version("3.2.0", sha256="18d459400558f4ea99527bc9786c033965a3db45bf4c6a32eefdc07aa9e306a6")
version("3.1.1", sha256="18d459400558f4ea99527bc9786c033965a3db45bf4c6a32eefdc07aa9e306a6")
version("3.1.0", sha256="18d459400558f4ea99527bc9786c033965a3db45bf4c6a32eefdc07aa9e306a6")
version("3.0.0", sha256="18d459400558f4ea99527bc9786c033965a3db45bf4c6a32eefdc07aa9e306a6")
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.

You might want to introduce some "mismatches" so the tests can demonstrate how that is handled (it seems like when= might be sufficient to that purpose).

fn = AspFunctionBuilder()

TransformFunction = Callable[[spack.spec.Spec, List[AspFunction]], List[AspFunction]]
TransformFunction = Callable[
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.

This could use a docstring (here or in the transform method).

@adamjstewart
Copy link
Copy Markdown
Member

Commented this on Slack, but I think I would prefer names like:

  • {version}, {variants}, etc.
  • {spec.version}
  • {self.spec.version}

These names match Spack/Python syntax more than {my}. It's what we would use if we used f-string formatting.

@becker33
Copy link
Copy Markdown
Member

becker33 commented Dec 19, 2023

I think it's worth trying to generalize this slightly -- as you've formulated it, this can only apply to dependency relationships. It would be great if we could apply it to siblings within the same unification space. Maybe something like this?

attr(AffectedAttrName, AffectedNode, Value)
   :- attr("node", node(X, Package)),
      attr("node", AffectedNode),
      unification_set(ID, node(X, Package)),
      unification_set(ID, AffectedNode),
      attr(PackageAttrName, node(X, Package), Value),
      attr("sync", AffectedNode, AffectedAttrName, attr(PackageAttrName, Package)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core PR affects Spack core functionality dependencies new-version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants