variants: Unify metadata dictionaries to index by when#44425
variants: Unify metadata dictionaries to index by when#44425
when#44425Conversation
ded1c63 to
038b2f4
Compare
|
@ax3l FYI |
fbea315 to
f5552a9
Compare
|
@alalazo can you do a concretizer timing on this? |
|
Timed on:
radiuss.develop.csv Concretization is generally slower (15%-20%), mostly due to Note that Details
|
2d2dd80 to
f6ed5a8
Compare
|
@alalazo ok try it with the latest commit. I avoided inverting the variant map in many places and setup should be much faster. |
The
radiuss.pr.csv It seems we have some slight speed-up and some slight slowdown at this point. Changes comes from Details
|
e37be0d to
9392a67
Compare
var/spack/repos/builtin.mock/packages/depends-on-develop/package.py
Outdated
Show resolved
Hide resolved
230b393 to
803c228
Compare
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
Signed-off-by: Todd Gamblin <[email protected]>
bc91daf to
b70227c
Compare
alalazo
left a comment
There was a problem hiding this comment.
Went through all the recent changes. LGTM. I'm going to benchmark concretization again, and post the results.
|
I timed this PR against
for two times. First run is: radiuss.develop.csv Second one is: radiuss.pr.csv In both cases I see consistent results that this branch is slightly slower on solve. The plot below is from the second run: EDIT: Briefly looking at commits since last review, I don't see changes in this PR that modify concretize.lp. Wondering if this is just "sampling" with a slightly different repository. |
|
I have some ideas for a heuristic for this but think it's good enough to merge as-is. Thanks! |
|
Gitlab failures are unrelated to this PR. |



Fixes #38302.
Enables #43961.
Continuing the work started in #40326, his changes the structure of Variant metadata on Packages from a single variant definition per name with a list of
whenspecs:to a Variant definition per
when_specper name:With this change, everything on a package except versions is keyed by
whenspec. This:when=: Change of Default for Single/Multi-Variant Transition #38302). We can now have, e.g., different defaults for the same variant in different versions of a package.Some notes:
This required some pretty deep changes to the solver. Previously, the solver's job was to select value(s) for a single variant definition per name per package. Now, the solver needs to:
a. Determine which variant definition should be used for a given node, which can depend on the node's version, compiler, target, other variants, etc.
b. Select valid value(s) for variants for each node based on the selected variant definition.
When multiple variant definitions are enabled via their
when=clause, we will always prefer the last matching definition, by declaration order in packages. This is implemented by adding aprecedenceto each variant at definition time, and we ensure they are added to the solver in order of precedence.This has the effect that variant definitions from derived classes are preferred over definitions from superclasses, and the last definition within the same class sticks. This matches python semantics. Some examples:
The global variant in
hipblaswill always supersede thewhen="+rocm"variant inROCmPackage. Ifhipblas's variant was also conditional on+rocm(as it probably should be), we would again filter out the definition fromROCmPackagebecause it could never be activated. If you instead have:The variant on
hipblaswill win for+rocm+foobut the one onROCmPackagewill win withrocm~foo.So, if we can statically determine if a variant is overridden, we filter it out. This isn't strictly necessary, as the solver can handle many definitions fine, but this reduces the complexity of the problem instance presented to
clingo, and simplifies output inspack infofor derived packages. e.g.,spack info hipblasnow shows only one definition ofamdgpu_targetwhere before it showed two, one of which would never be used.Nearly all access to the
variantsdictionary on packages has been refactored to use the following class methods onPackageBase:variant_names(cls) -> List[str]: get all variant names for a packagehas_variant(cls, name) -> bool: whether a package has a variant with a given namevariant_definitions(cls, name: str) -> List[Tuple[Spec, Variant]]: all definitions of variantnamethat are possible, along with theirwhenspecs.variant_items() ->: iterate overpkg.variants.items(), with impossible variants filtered out.Consolidating to these methods seems to simplify the code a lot.
The solver does a lot more validation on variant values at setup time now. In particular, it checks whether a variant value on a spec is valid given the other constraints on that spec. This allowed us to remove the crufty logic in
update_variant_validate, which was needed because we previously didn't know after a solve which variant definition had been used. Now, variant values from solves are constructed strictly based on which variant definition was selected -- no more heuristics.The same prevalidation can now be done in package audits, and you can run:
This turns up around 18 different places where a variant specification isn't valid given the conditions on variant definitions in packages. I haven't fixed those here but will open a separate PR to iterate on them. I plan to make strict checking the defaults once all existing package issues are resolved. It's not clear to me that strict checking should be the default for the prevalidation done at solve time.
There are a few other changes here that might be of interest:
generatorvariant inCMakePackageis now only defined whenbuild_system=cmake.spack infohas been updated to support the new metadata layout..lpfile in thesolvercode.variant.py.