ASP-based solver: allow to reuse installed externals by alalazo · Pull Request #31558 · spack/spack

alalazo · 2022-07-13T12:54:35Z

Before this change if anything was matching an external condition, it was considered "external" and thus something to be "built". This was happening in particular to external packages that were re-read from the DB, which then couldn't be reused, causing the problems shown in #31484.

This PR fixes the issue by excluding specs with a "hash" from being considered "external"

becker33 · 2022-07-13T16:38:01Z

lib/spack/spack/solver/concretize.lp


 % if a package is not buildable (external_only), only externals are allowed
-external(Package) :- external_only(Package), node(Package).
+external(Package) :- external_only(Package),


I think this will allow you to reuse a non-external even when buildable: false for that package. Can you add a test for this?

I don't think this is wrong. It will allow what you say only if the non-external is already installed, so not buildable, but built. Disallowing that case causes the bug in this way:

Install an external that is not buildable

Can't reuse the same external at the next concretization

adamjstewart

Works like a charm, thanks for the quick fix!

Also tested that the tests do indeed fail without this change.

alalazo · 2022-07-15T14:36:15Z

lib/spack/spack/test/concretize.py

+    @pytest.mark.regression('31484')
+    def test_installed_builds_not_reused_buildable_false(
+            self, mutable_database, repo_with_changing_recipe
+    ):


@becker33 I have been getting back at this specific test, and I think this is behavior we don't want to add as it is coded here. The only things we currently say for buildable: false in our documentation are:

The addition of the buildable flag tells Spack that it should never build its own version of OpenMPI, and it will instead always rely on a pre-built OpenMPI.

The buildable does not need to be paired with external packages. It could also be used alone to forbid packages that may be buggy or otherwise undesirable.
[ ... ]

and they clearly stem from a world where reuse was not a considered at all. It seems thus a stretch to me to require that buildable: false prevents the reuse of software that is either installed locally or available from a buildcache.

That said, I concur, based on our discussion on Slack, that there's a gap in expressiveness that we need to fill. For instance, as you pointed out:

packages: mpi: buildable: false

likely means that the user wants to install only from one of the externals that might be listed in the same file. But what if it is:

packages: trilinos: buildable: false

Couldn't that be a way to say "Reuse a binary version of trilinos and never build that from sources"? I wouldn't find that request "crazy", if a binary cache is available.

I think the point is: buildable: false was added before reuse and we didn't figure out how the two should interact together. That is possibly what needs to get fixed (allow the user to express in which of the two cases they are), not excluding specs from reuse based on buildable: false.

TL;DR I agree that here we miss ways to express a few requests, but would it be possible to get the bugfix in as is, and work on a more extensive feature in a following PR? lf not I'm tempted to move this out of the v0.18.1 project, since it likely means it won't be solved soon.

I still don't see how someone using this is supposed to use system mpi, and I don't think that's a use case we can afford to break.

@becker33 Fair, I already moved this to the next bugfix release. Do you agree though that the test, as is, is not something we want to force implicitly?

becker33 · 2022-07-20T18:22:49Z

I still don't see how someone using this is supposed to use system mpi, and I don't think that's a use case we can afford to break.

sethrj · 2022-08-01T10:51:11Z

@alalazo I think this will also fix #31832 . Interestingly my libuuid external is a build-only dependency of the spec that's failing... so it doesn't appear in spack find -lvd. However I guess it still gets used by the concretizer?

adamjstewart · 2022-08-05T21:03:00Z

@becker33 @alalazo any updates on this? Spack is basically unusable on macOS if you update develop frequently. Not sure if there's a priority level higher than "impact-high" lol. I've been using this PR locally on a couple systems since this PR was first opened and I would like to no longer have to use that local modification.

alalazo · 2022-08-06T08:40:12Z

@adamjstewart There is currently an issue, that @becker33 pointed out, that is hard to solve in this PR alone. My thoughts on this are:

The second test added is imho not what we want from the perspective of the semantics
I am trying to push forward the review of Configure requirements for a package #27987 to have hard requirements in packages.yaml
When Configure requirements for a package #27987 is merged, we can remove the second test here (maybe add another one to ensure we don't have regression on the issue with mpi:buildable:false) and merge this

Rationale is that, if we have requirements, we can specify there what needs to be used.

sethrj · 2022-08-15T20:21:05Z

@alalazo @adamjstewart @becker33 is there a quick fix for getting this in for macos platforms?

alalazo · 2022-08-15T20:53:25Z

@sethrj #27987 will be merged today or tomorrow. With that in I plan to:

Rebase this PR
Substitute the last test with a similar one that uses requirements
🤞 that everything is ✔️
Merge the PR asap

sethrj · 2022-08-15T23:03:23Z

Amazing! Thanks @alalazo !

adamjstewart · 2022-08-15T23:04:29Z

Is the test the only thing that was preventing this PR from being merged? I was under the impression that there was a problem with the solution itself.

alalazo · 2022-08-16T07:16:13Z

@adamjstewart The issue that @becker33 was raising is that we don't have a way to discriminate between an external and a binary package when reuse:true. This bug (which gives an inconsistent internal solution in ASP) don't allow anything matching an external to be reused, so a user can say:

mpi:
  buildable: false
packages:
  openmpi:
    externals:
    - spec: "[email protected]"

and be sure it is using the external spec. I think with #27987 we can use hard-requirements to pin the required virtual into specs.

alalazo · 2022-08-17T11:09:26Z

I added a new test. Currently users can select an external, by imposing unsatisfiable requirements on other providers that could otherwise be reused. This is not ideal, but works. If people agree (@adamjstewart @sethrj @becker33) I would merge the bug fix like it is for the time being, and then start working on extending the packages:<pkg>:require attribute to virtual packages. Ideally I'd like to have:

packages:
  mpi:
    buildable: false
    require: '[email protected]'
  openmpi:
    externals:
    - spec: '[email protected]'
      prefix: /usr

applied whenever mpi is in the DAG as a virtual spec.

EDIT: Pinging @scheibelp as he may want to comment on the extension sketched above.

alalazo · 2022-08-18T06:43:52Z

@becker33 Would #31558 (comment) be fine with you?

psakievich · 2022-08-25T15:48:21Z

@becker33 any updates on this?

alalazo · 2022-08-25T15:55:08Z

For the record: I submitted #32369 today so I hope we'll converge to the syntax in #31558 (comment) pretty soon (no matter whether this PR or #32369 gets merged first).

fixes spack#31484 Before this change if anything was matching an external condition, it was considered "external" and thus something to be "built". This was happening in particular to external packages that were re-read from the DB, which then couldn't be reused, causing the problems shown in spack#31484. This PR fixes the issue by excluding specs with a "hash" from being considered "external"

Currently, users have to resort to a workaround, i.e. they have to put unsatisfiable requirements on reusable packages they could otherwise pick. This ought to be solved by extending the "require" attribute to virtual packages, so that one can: ```yaml mpi: require: 'multi-provider-mpi' ```

alalazo · 2022-08-26T20:34:32Z

#32369 has just been merged, and the second unit test updated accordingly

lib/spack/spack/solver/concretize.lp

becker33 · 2022-08-26T21:40:18Z

lib/spack/spack/test/concretize.py

+        assert external3.dag_hash() == external1.dag_hash()
+
+    @pytest.mark.regression("31484")
+    def test_user_can_select_externals_with_require(self, mutable_database):


If I understand correctly, this only ensures we can force the provider, not the specific spec.

$ spack install [email protected] $ spack config add packages:mvapich2:buildable:false $ spack config edit # add external for [email protected] $ spack install mvapich2

would reuse the [email protected] installed on the first line, and there is no recourse to force it to use the external as far as I understand.

True. Then you can call your external [email protected]. I hope this will be a niche case

…ct its origin

alalazo · 2022-08-29T16:39:34Z

@becker33 The bug occurring when:

A spec is installed
Later, a conflict is added for which the spec at 1 is a match
The spec installed at 1 cannot be reused

has been fixed in 30e8edc, and a unit test has been included to avoid regressions.

I checked the rules in which external(Package) is involved and it seems to me they all come from before we introduced reuse. This is to say that external in concretize.lp means "defined as external in packages.yaml" since the external(Package) atom was introduced. One example for that is for instance:

spack/lib/spack/spack/solver/concretize.lp

Lines 450 to 461 in 1c22af8

    
           % if a package is external its version must be one of the external versions 
        
           { external_version(Package, Version, Weight): 
        
               version_declared(Package, Version, Weight, "external") } 
        
               :- external(Package). 
        
           error(2, "Attempted to use external for '{0}' which does not satisfy any configured external spec", Package) 
        
             :- external(Package), 
        
                not external_version(Package, _, _). 
        
           error(2, "Attempted to use external for '{0}' which does not satisfy any configured external spec", Package) 
        
             :- external(Package), 
        
                external_version(Package, Version1, Weight1), 
        
                external_version(Package, Version2, Weight2), 
        
                (Version1, Weight1) < (Version2, Weight2). % see[1]

If a package is external we can only choose versions that are declared in packages.yaml. Not accounting for this, caused the bug in #31484¹ and is fixed here. In 27adf2a I renamed an internal atom to adhere better to where it originates. In e569ba0 I updated docs to add some details on buildable: false and its behavior. Let me know how these changes looks.²

What gets installed is "frozen" and can get out of sync with a changing packages.yaml ↩
In the long term I hope we can get rid of most of this custom logic for externals, and that Spack can automatically detect packages and pull them into the local store in a reserved namespace (e.g. spack.pkg.externals), without the need for users to declare them manually. I don't think though implementing ~~all that~~ anything more that is in this PR is necessary to fix this particular bug. Instead, I think this PR can solve a high-priority bug that has been open for a month and a half and give us time to consider how to design a better support for externals that blends with other requests to better tune what can be reused. ↩

lib/spack/docs/build_settings.rst

Co-authored-by: Greg Becker <[email protected]>

adamjstewart · 2022-08-31T20:21:48Z

Thanks for the hard work on this @alalazo and @becker33! It's unfortunate that it ended up being more involved than expected, but glad to see this bug finally fixed!

becker33 · 2022-09-01T02:11:07Z

Give @alalazo all the credit, I just complained about solutions and he came up with new ones

adamjstewart · 2022-09-01T03:23:43Z

Playing Devil's advocate and finding ways that a solution might break is just as important 😄

fixes spack#31484 Before this change if anything was matching an external condition, it was considered "external" and thus something to be "built". This was happening in particular to external packages that were re-read from the DB, which then couldn't be reused, causing the problems shown in spack#31484. This PR fixes the issue by excluding specs with a "hash" from being considered "external" * Test that users have a way to select a virtual This ought to be solved by extending the "require" attribute to virtual packages, so that one can: ```yaml mpi: require: 'multi-provider-mpi' ``` * Prevent conflicts to be enforced on specs that can be reused. * Rename the "external_only" fact to "buildable_false", to better reflect its origin

spackbot-app bot added the tests General test capability(ies) label Jul 13, 2022

alalazo force-pushed the bugfix/reuse_external_packages branch from 740297b to e5ac540 Compare July 13, 2022 12:56

alalazo added bugfix Something wasn't working, here's a fix hotfix labels Jul 13, 2022

becker33 requested changes Jul 13, 2022

View reviewed changes

adamjstewart previously approved these changes Jul 13, 2022

View reviewed changes

alalazo commented Jul 15, 2022

View reviewed changes

adamjstewart requested a review from becker33 July 18, 2022 23:50

adamjstewart added concretization external-packages labels Jul 20, 2022

tldahlgren assigned adamjstewart Jul 27, 2022

alalazo dismissed adamjstewart’s stale review via 5d44b27 August 17, 2022 11:04

alalazo force-pushed the bugfix/reuse_external_packages branch from e386132 to 5d44b27 Compare August 17, 2022 11:04

spackbot-app bot added the core PR affects Spack core functionality label Aug 17, 2022

alalazo force-pushed the bugfix/reuse_external_packages branch from 5d44b27 to 1491716 Compare August 17, 2022 15:37

psakievich mentioned this pull request Aug 18, 2022

New Syntax in Spack to apply constraints in packages.py sandialabs/spack-manager#333

Closed

alalazo added 2 commits August 26, 2022 22:27

Use "require" instead of hack

ad7d74c

alalazo force-pushed the bugfix/reuse_external_packages branch from 1491716 to ad7d74c Compare August 26, 2022 20:33

becker33 reviewed Aug 26, 2022

View reviewed changes

alalazo added 3 commits August 29, 2022 12:53

Prevent conflicts to be enforced on specs that can be reused.

30e8edc

Rename the "external_only" fact to "buildable_false", to better refle…

27adf2a

…ct its origin

Update documentation

e569ba0

spackbot-app bot added the documentation Improvements or additions to documentation label Aug 29, 2022

becker33 requested changes Aug 31, 2022

View reviewed changes

lib/spack/docs/build_settings.rst Outdated Show resolved Hide resolved

Update comment

184b59a

Co-authored-by: Greg Becker <[email protected]>

becker33 approved these changes Aug 31, 2022

View reviewed changes

becker33 enabled auto-merge (squash) August 31, 2022 17:32

adamjstewart approved these changes Aug 31, 2022

View reviewed changes

becker33 merged commit 0c6e318 into spack:develop Aug 31, 2022

alalazo deleted the bugfix/reuse_external_packages branch September 1, 2022 07:07

psakievich mentioned this pull request Sep 1, 2022

Update Spack for bug fixes sandialabs/spack-manager#340

Merged

greenc-FNAL mentioned this pull request Sep 9, 2022

Changes required for Cetmodules dependencies. FNALssi/spack#31

Merged

alalazo mentioned this pull request Oct 24, 2022

Spack ignores buildable=False if available in binary cache #33461

Closed

3 tasks

Conversation

alalazo commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

becker33 Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 15, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

becker33 commented Jul 20, 2022

Uh oh!

sethrj commented Aug 1, 2022

Uh oh!

adamjstewart commented Aug 5, 2022

Uh oh!

alalazo commented Aug 6, 2022

Uh oh!

sethrj commented Aug 15, 2022

Uh oh!

alalazo commented Aug 15, 2022

Uh oh!

sethrj commented Aug 15, 2022

Uh oh!

adamjstewart commented Aug 15, 2022

Uh oh!

alalazo commented Aug 16, 2022

Uh oh!

alalazo commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Aug 18, 2022

Uh oh!

psakievich commented Aug 25, 2022

Uh oh!

alalazo commented Aug 25, 2022

Uh oh!

alalazo commented Aug 26, 2022

Uh oh!

Uh oh!

becker33 Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

Uh oh!

adamjstewart commented Aug 31, 2022

Uh oh!

becker33 commented Sep 1, 2022

Uh oh!

adamjstewart commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alalazo commented Jul 13, 2022 •

edited

Loading

alalazo Jul 13, 2022 •

edited

Loading

alalazo Jul 20, 2022 •

edited

Loading

alalazo commented Aug 17, 2022 •

edited

Loading

alalazo commented Aug 29, 2022 •

edited

Loading