Skip to content

concretize.lp: enforce target compatibility through DAG#29694

Merged
alalazo merged 2 commits intodevelopfrom
bugfix/target-compatibility
Apr 8, 2022
Merged

concretize.lp: enforce target compatibility through DAG#29694
alalazo merged 2 commits intodevelopfrom
bugfix/target-compatibility

Conversation

@becker33
Copy link
Copy Markdown
Member

@becker33 becker33 commented Mar 23, 2022

Related to #29672

29672 involves two separate issues

  1. Spack allows dependencies to be concretized for an architecture incompatible with the root
  2. Spack does not consider the host architecture when using reuse to pick concrete specs

This PR fixes the first issue.

@bvanessen with this fix you will be able to work around the first issue by explicitly requesting target=power8le for your root specs with the matrix we discussed yesterday.

@spackbot-app spackbot-app bot added the tests General test capability(ies) label Mar 23, 2022
@bvanessen
Copy link
Copy Markdown
Contributor

bvanessen commented Mar 23, 2022

@becker33 Do I have to explicitly ask for the target=power8le or will that be inferred because of the platform that I am on?

@becker33
Copy link
Copy Markdown
Member Author

becker33 commented Mar 23, 2022

@bvanessen you have to explicitly ask, the inferred part is the task 2 identified above for a future PR (future because I want to get this in quickly to provide a workaround.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Mar 29, 2022

I'll have a closer look at the implementation, but at a high level wouldn't it be a valid use case that of being able to specify a root with a target that is "more generic" than the dependency? Shouldn't that just mean that the most specific target is required to run/use the software?

Use cases (admittedly niche) that come to mind:

  1. Downgrade the target of a node because the optimization level on that node triggers some compiler bug
  2. Splice optimized math libraries in a software built otherwise with generic optimization (e.g. target=x86_64)

alalazo
alalazo reviewed Mar 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The implementation, including the test, LGTM since it performs perfectly what is stated in the description. I have a performance concern, as this PR increases the wall-clock time for me of a 30-40% on a simple test.

On develop:

$ spack solve -l --timers hdf5~mpi target=skylake ^zlib target=x86_64
Time:
    setup:         6.5062
    load:          0.0235
    ground:        1.3863
    solve:         1.9863
Total: 9.9276
[ ... ]
a7ujmwo  [email protected]%[email protected] [ ... ]

while with this PR:

$ spack solve -l --timers hdf5~mpi target=skylake ^zlib target=x86_64
Time:
    setup:         6.9691
    load:          0.0270
    ground:        3.2747
    solve:         4.2144
Total: 14.5106
[ ... ]
a7ujmwo  [email protected]%[email protected] [ ... ]

Should we consider tying this behavior to some configuration option in concretizer.yaml?

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Mar 29, 2022 •
edited
Loading

For point 2. in the description I tried to apply this simple diff:

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 89c9108fc1..0c108f909f 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1405,12 +1405,12 @@ def target_defaults(self, specs):
         self.gen.h2('Target compatibility')
 
         compatible_targets = [uarch] + uarch.ancestors
-        additional_targets_in_family = sorted([
-            t for t in archspec.cpu.TARGETS.values()
-            if (t.family.name == uarch.family.name and
-                t not in compatible_targets)
-        ], key=lambda x: len(x.ancestors), reverse=True)
-        compatible_targets += additional_targets_in_family
+        # additional_targets_in_family = sorted([
+        #     t for t in archspec.cpu.TARGETS.values()
+        #     if (t.family.name == uarch.family.name and
+        #         t not in compatible_targets)
+        # ], key=lambda x: len(x.ancestors), reverse=True)
+        # compatible_targets += additional_targets_in_family
         compilers = self.possible_compilers
 
         # this loop can be used to limit the number of targets

which effectively allows clingo only to consider targets that are ancestors of the host. Runtime improves by ~10% on icelake by trimming all the targets that are either AMD or not in the icelake root DAG. I wonder if we can figure out other options to limit the number of possible architectures and speed-up concretization (maybe in another PR, since it's related but orthogonal). For instance we could have a "generic levels only" kind of flag that will leave concretization with only x86_v* definitions.

@alalazo alalazo mentioned this pull request Apr 1, 2022
Merged
3 tasks
@becker33 becker33 force-pushed the bugfix/target-compatibility branch from ce2cb4e to dd9a6d6 Compare April 7, 2022 23:54
@becker33
Copy link
Copy Markdown
Member Author

becker33 commented Apr 7, 2022

@alalazo performance should be improved with my latest commit

This does not get fully back to the performance of develop, but it's much closer. When combined with #29926 (another bugfix related to targets, but it helps performance also) then performance is approximately equal to the current develop

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Apr 8, 2022

With the latest version, on develop:

$ spack solve -l --timers hdf5~mpi target=skylake ^zlib target=x86_64 
Time:
    setup:         4.0466
    load:          0.0191
    ground:        1.3779
    solve:         1.6693
Total: 7.1250

==> Best of 6 considered solutions.
==> Optimization Criteria:
  Priority  Criterion                                            Installed  ToBuild
  1         number of packages to build (vs. reuse)                      -        0
  2         deprecated versions used                                     0        0
  3         version weight                                               0        0
  4         number of non-default variants (roots)                       0        0
  5         preferred providers for roots                                0        0
  6         default values of variants not being used (roots)            0        0
  7         number of non-default variants (non-roots)                   0        0
  8         preferred providers (non-roots)                              0        0
  9         compiler mismatches                                          0        0
  10        OS mismatches                                                0        0
  11        non-preferred OS's                                           0        0
  12        version badness                                              0        4
  13        default values of variants not being used (non-roots)        0        1
  14        non-preferred compilers                                      0        0
  15        target mismatches                                            0        3
  16        non-preferred targets                                        0       64

ug54dun  [email protected]%[email protected]~cxx~fortran~hl~ipo~java~mpi+shared~szip~threadsafe+tools api=default build_type=RelWithDebInfo patches=ee351eb arch=linux-ubuntu20.04-skylake
r5zacq4      ^[email protected]%[email protected]~doc+ncurses+ownlibs~qt build_type=Release arch=linux-ubuntu20.04-skylake
vkvafs4          ^[email protected]%[email protected]~symlinks+termlib abi=none arch=linux-ubuntu20.04-skylake
gmcewy3              ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
memequu          ^[email protected]%[email protected]~docs~shared certs=system arch=linux-ubuntu20.04-skylake
nlnc57n              ^[email protected]%[email protected]+cpanm+shared+threads arch=linux-ubuntu20.04-skylake
yniz5iz                  ^[email protected]%[email protected]+cxx~docs+stl patches=b231fcc arch=linux-ubuntu20.04-skylake
exsz2mg                  ^[email protected]%[email protected]~debug~pic+shared arch=linux-ubuntu20.04-skylake
2nlyt7g                      ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
surcip3                          ^[email protected]%[email protected] libs=shared,static arch=linux-ubuntu20.04-skylake
ts2vtbx                  ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
upmrpdl                      ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
boagcht                  ^[email protected]%[email protected]+optimize+pic+shared patches=0d38234 arch=linux-ubuntu20.04-x86_64

while on this PR:

$ spack solve -l --timers hdf5~mpi target=skylake ^zlib target=x86_64 
Time:
    setup:         4.0627
    load:          0.0194
    ground:        1.4667
    solve:         1.7911
Total: 7.3519

==> Best of 4 considered solutions.
==> Optimization Criteria:
  Priority  Criterion                                            Installed  ToBuild
  1         number of packages to build (vs. reuse)                      -        0
  2         deprecated versions used                                     0        0
  3         version weight                                               0        0
  4         number of non-default variants (roots)                       0        0
  5         preferred providers for roots                                0        0
  6         default values of variants not being used (roots)            0        0
  7         number of non-default variants (non-roots)                   0        0
  8         preferred providers (non-roots)                              0        0
  9         compiler mismatches                                          0        0
  10        OS mismatches                                                0        0
  11        non-preferred OS's                                           0        0
  12        version badness                                              0        4
  13        default values of variants not being used (non-roots)        0        1
  14        non-preferred compilers                                      0        0
  15        target mismatches                                            0        3
  16        non-preferred targets                                        0       64

ug54dun  [email protected]%[email protected]~cxx~fortran~hl~ipo~java~mpi+shared~szip~threadsafe+tools api=default build_type=RelWithDebInfo patches=ee351eb arch=linux-ubuntu20.04-skylake
r5zacq4      ^[email protected]%[email protected]~doc+ncurses+ownlibs~qt build_type=Release arch=linux-ubuntu20.04-skylake
vkvafs4          ^[email protected]%[email protected]~symlinks+termlib abi=none arch=linux-ubuntu20.04-skylake
gmcewy3              ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
memequu          ^[email protected]%[email protected]~docs~shared certs=system arch=linux-ubuntu20.04-skylake
nlnc57n              ^[email protected]%[email protected]+cpanm+shared+threads arch=linux-ubuntu20.04-skylake
yniz5iz                  ^[email protected]%[email protected]+cxx~docs+stl patches=b231fcc arch=linux-ubuntu20.04-skylake
exsz2mg                  ^[email protected]%[email protected]~debug~pic+shared arch=linux-ubuntu20.04-skylake
2nlyt7g                      ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
surcip3                          ^[email protected]%[email protected] libs=shared,static arch=linux-ubuntu20.04-skylake
ts2vtbx                  ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
upmrpdl                      ^[email protected]%[email protected] arch=linux-ubuntu20.04-skylake
boagcht                  ^[email protected]%[email protected]+optimize+pic+shared patches=0d38234 arch=linux-ubuntu20.04-x86_64

so it is really a few %, which seems fine.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Apr 8, 2022

@becker33 Just for the record, did you try to write target_compatible facts from Python instead of deducing them? I think we might even ship with a target_compatibility.lp file, since that depends only on the version of archspec we use with Spack. Anyhow, I see this as a possible optimization, so if it ever happens it should be in a separate PR.

@alalazo alalazo merged commit 79ba0c5 into develop Apr 8, 2022
@alalazo alalazo deleted the bugfix/target-compatibility branch April 8, 2022 09:01
@becker33
Copy link
Copy Markdown
Member Author

becker33 commented Apr 8, 2022

@alalazo yes, I tried writing them from python instead of deducing them, it didn't make any noticeable difference in performance.

@bvanessen
Copy link
Copy Markdown
Contributor

bvanessen commented Apr 11, 2022

@alalazo @becker33 I wanted to confirm that this worked for me.

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

Reviewers

@alalazo alalazo alalazo approved these changes

Assignees

No one assigned

Labels

tests General test capability(ies)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@becker33 @bvanessen @alalazo