Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

concretize.lp: enforce target compatibility through DAG #29694

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

becker33
Copy link
Member

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
Contributor

@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
Member Author

@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
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)

Copy link
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.

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  hdf5@1.12.1%gcc@9.4.0 [ ... ]

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  hdf5@1.12.1%gcc@9.4.0 [ ... ]

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

lib/spack/spack/solver/concretize.lp Outdated Show resolved Hide resolved
@alalazo
Copy link
Member

alalazo commented Mar 29, 2022

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.

@becker33 becker33 added this to In progress in Spack reuse specs by default via automation Apr 4, 2022
@becker33 becker33 force-pushed the bugfix/target-compatibility branch from ce2cb4e to dd9a6d6 Compare April 7, 2022 23:54
@becker33
Copy link
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
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  hdf5@1.12.1%gcc@9.4.0~cxx~fortran~hl~ipo~java~mpi+shared~szip~threadsafe+tools api=default build_type=RelWithDebInfo patches=ee351eb arch=linux-ubuntu20.04-skylake
r5zacq4      ^cmake@3.23.0%gcc@9.4.0~doc+ncurses+ownlibs~qt build_type=Release arch=linux-ubuntu20.04-skylake
vkvafs4          ^ncurses@6.2%gcc@9.4.0~symlinks+termlib abi=none arch=linux-ubuntu20.04-skylake
gmcewy3              ^pkgconf@1.8.0%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
memequu          ^openssl@1.1.1n%gcc@9.4.0~docs~shared certs=system arch=linux-ubuntu20.04-skylake
nlnc57n              ^perl@5.34.1%gcc@9.4.0+cpanm+shared+threads arch=linux-ubuntu20.04-skylake
yniz5iz                  ^berkeley-db@18.1.40%gcc@9.4.0+cxx~docs+stl patches=b231fcc arch=linux-ubuntu20.04-skylake
exsz2mg                  ^bzip2@1.0.8%gcc@9.4.0~debug~pic+shared arch=linux-ubuntu20.04-skylake
2nlyt7g                      ^diffutils@3.8%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
surcip3                          ^libiconv@1.16%gcc@9.4.0 libs=shared,static arch=linux-ubuntu20.04-skylake
ts2vtbx                  ^gdbm@1.19%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
upmrpdl                      ^readline@8.1%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
boagcht                  ^zlib@1.2.12%gcc@9.4.0+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  hdf5@1.12.1%gcc@9.4.0~cxx~fortran~hl~ipo~java~mpi+shared~szip~threadsafe+tools api=default build_type=RelWithDebInfo patches=ee351eb arch=linux-ubuntu20.04-skylake
r5zacq4      ^cmake@3.23.0%gcc@9.4.0~doc+ncurses+ownlibs~qt build_type=Release arch=linux-ubuntu20.04-skylake
vkvafs4          ^ncurses@6.2%gcc@9.4.0~symlinks+termlib abi=none arch=linux-ubuntu20.04-skylake
gmcewy3              ^pkgconf@1.8.0%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
memequu          ^openssl@1.1.1n%gcc@9.4.0~docs~shared certs=system arch=linux-ubuntu20.04-skylake
nlnc57n              ^perl@5.34.1%gcc@9.4.0+cpanm+shared+threads arch=linux-ubuntu20.04-skylake
yniz5iz                  ^berkeley-db@18.1.40%gcc@9.4.0+cxx~docs+stl patches=b231fcc arch=linux-ubuntu20.04-skylake
exsz2mg                  ^bzip2@1.0.8%gcc@9.4.0~debug~pic+shared arch=linux-ubuntu20.04-skylake
2nlyt7g                      ^diffutils@3.8%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
surcip3                          ^libiconv@1.16%gcc@9.4.0 libs=shared,static arch=linux-ubuntu20.04-skylake
ts2vtbx                  ^gdbm@1.19%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
upmrpdl                      ^readline@8.1%gcc@9.4.0 arch=linux-ubuntu20.04-skylake
boagcht                  ^zlib@1.2.12%gcc@9.4.0+optimize+pic+shared patches=0d38234 arch=linux-ubuntu20.04-x86_64

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

@alalazo alalazo added this to In progress in Spack v0.18.0 release via automation Apr 8, 2022
Spack v0.18.0 release automation moved this from In progress to Reviewer approved Apr 8, 2022
Spack reuse specs by default automation moved this from In progress to Reviewer approved Apr 8, 2022
@alalazo
Copy link
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
Spack v0.18.0 release automation moved this from Reviewer approved to Done Apr 8, 2022
@alalazo alalazo deleted the bugfix/target-compatibility branch April 8, 2022 09:01
Spack reuse specs by default automation moved this from Reviewer approved to Done Apr 8, 2022
@becker33
Copy link
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
Contributor

@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
Labels
tests General test capability(ies)
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants