Split `satisfies(..., strict=True/False)` into two functions by alalazo · Pull Request #35681 · spack/spack

alalazo · 2023-02-24T17:06:33Z

This PR splits each satisfies method having a strict= argument into two function calls:

intersects: corresponds to strict=False and has "intersection" semantic.
satisfies: corresponds to strict=True and has "subset" semantic.

From 1 it follows that intersects is commutative, so in this PR I also tried to remove most of the non-commutative behaviors we had with strict=False.

Modifications:

Split satisfies(..., strict=...) into satisfies and intersects
Add unit tests to verify expected properties of the methods
Fixed bugs in ArchSpec related to targets, see ci: fixes for compiler bootstrapping #17563 (review)
Fixed semantics of variant, when compared with each other see 3b89375
Fixed semantics of flags (now they satisfy on exact match, not on set match - intersection is True with specs that don't have a flag set) see 3a5b534
Fixed semantic of constraining an abstract spec by a concrete one, see a2a9167 and 3466bd2
Make GitVersion consistent with the intersects and satisfies semantic (see 10c147f)

spackbot-app · 2023-02-24T17:06:51Z

@naughtont3 can you review this PR?

This PR modifies the following package(s), for which you are listed as a maintainer:

gcc
intel-tbb
openmpi
python

lib/spack/spack/spec.py

alalazo · 2023-02-24T17:12:12Z

lib/spack/spack/test/spec_semantics.py

+        # Architecture
+        (ArchSpec, "None-ubuntu20.04-None", "None-None-x86_64", (True, False, False)),
+        (ArchSpec, "None-ubuntu20.04-None", "linux-None-x86_64", (True, False, False)),
+        (ArchSpec, "None-None-x86_64:", "linux-None-haswell", (True, False, True)),
+        (ArchSpec, "None-None-x86_64:haswell", "linux-None-icelake", (False, False, False)),
+        (ArchSpec, "linux-None-None", "linux-None-None", (True, True, True)),
+        (ArchSpec, "darwin-None-None", "linux-None-None", (False, False, False)),
+        (ArchSpec, "None-ubuntu20.04-None", "None-ubuntu20.04-None", (True, True, True)),
+        (ArchSpec, "None-ubuntu20.04-None", "None-ubuntu22.04-None", (False, False, False)),
+        # Compiler
+        (CompilerSpec, "gcc", "clang", (False, False, False)),
+        (CompilerSpec, "gcc", "gcc@5", (True, False, True)),
+        (CompilerSpec, "gcc@5", "[email protected]", (True, False, True)),
+        (CompilerSpec, "gcc@5", "gcc@5-tag", (True, False, True)),
+        # Flags (flags are a map, so for convenience we initialize a full Spec)
+        # Note: the semantic is that of sv variants, not mv variants
+        (Spec, "cppflags=-foo", "cppflags=-bar", (False, False, False)),
+        (Spec, "cppflags='-bar -foo'", "cppflags=-bar", (False, False, False)),
+        (Spec, "cppflags=-foo", "cppflags=-foo", (True, True, True)),
+        (Spec, "cppflags=-foo", "cflags=-foo", (True, False, False)),


More cases to test are welcome here, if reviewers have some.

lib/spack/spack/test/spec_syntax.py

lib/spack/spack/test/variant.py

lib/spack/spack/variant.py

lib/spack/spack/spec.py

adamjstewart · 2023-02-24T19:21:26Z

I'm not caught up on the history of this but I do seem to recall that the following (which are all used frequently in packages) are slightly different:

if "+python" in self.spec:
if self.spec.satisfies("+python"):
if self.spec.satisfies("+python", strict=True):

With this PR, will the first two be equivalent and correct? Can't remember what the issue was with these originally.

alalazo · 2023-02-27T14:51:12Z

@adamjstewart This PR tries to have a slightly more formal notion of strict=True and strict=False, and splits the behavior around that argument in two functions. One is:

self.intersects(other)

which roughly corresponds to strict=False and has intersection semantic - if there exist at least one spec that matches both self and other then self.intersects(other) is true. Otherwise it's false. This function is commutative.

The other is:

self.satisfies(other)

which corresponds to strict=True and has subset semantic, i.e. all the specs matching self also match other.

This refactor was motivated by the bug in #35597 which, upon investigation, revealed a few inconsistencies in the current API that we are trying to fix here. In the course of preparing this PR I also discovered a few bugs in version comparison and target comparison, that are fixed here.

alalazo · 2023-03-03T23:16:43Z

lib/spack/spack/test/spec_semantics.py

+    def test_abstract_specs_can_constrain_each_other(self, lhs, rhs, expected):
+        """Test that lhs and rhs intersect with each other, and that they can be constrained
+        with each other. Also check that the constrained result match the expected spec.
+        """
+        lhs, rhs, expected = Spec(lhs), Spec(rhs), Spec(expected)

-def check_constrain(expected, spec, constraint):
-    exp = Spec(expected)
-    spec = Spec(spec)
-    constraint = Spec(constraint)
-    spec.constrain(constraint)
-    assert exp == spec
+        assert lhs.intersects(rhs)
+        assert rhs.intersects(lhs)

+        c1, c2 = lhs.copy(), rhs.copy()
+        c1.constrain(rhs)
+        c2.constrain(lhs)
+        assert c1 == c2
+        assert c1 == expected


Here I always check all the invariants we expect for each of the new parametrized tests.

alalazo · 2023-03-06T16:30:50Z

I tried to time a few concretizations with the same setup on develop:

and with this branch:

Here are the datasets:

radiuss_develop.csv
radiuss_split.csv

I don't see significant differences.

scheibelp · 2023-03-07T20:39:44Z

FWIW it looks like outside of the old concretizer, that x.satisfies(y) is only ever called when x is concrete. IMO that may be useful to encode somehow (given that the abstract-vs.-concrete possibilities make things more difficult to reason about): for example .satisfies could (by default) require that self is concrete.

haampie · 2023-03-07T21:25:53Z

The abstract vs concrete case isn't (very?) relevant for satisfies: if the rhs is concrete and has a dag hash, the lhs should have the same dag hash, and if only the lhs is concrete it doesn't matter how you interpret the lhs (as a singleton set with just the spec, or still an infinite set where elements have additional variants defined etc) for the result to make sense.

The abstract vs concrete case is relevant for intersects, since then the answer to "is there a more specific spec than a given concrete spec" determines whether intersects(concrete, abstract) returns true or false in some cases. If you see concrete specs as singletons, then intersects(concrete, abstract) with additional variants in abstract is false. If you see concrete specs sort of like very constrained abstract specs, then it could be true. [so for example intersects([email protected], a+x) where lhs is concrete and doesn't have x defined]

To me abstract specs as singletons make most sense, since they're elements picked from the solution space, so I would say intersects([email protected], a+x) == false. The only reason I don't insist for that to be fixed in this PR is because of Version(x) == VersionRange(x, x) rabbit holes 🙃

alalazo · 2023-03-08T07:33:38Z

To me abstract specs as singletons make most sense, since they're elements picked from the solution space, so I would say intersects([email protected], a+x) == false. The only reason I don't insist for that to be fixed in this PR is because of Version(x) == VersionRange(x, x) rabbit hole

Agreed, since they are basically the same issue.

alalazo · 2023-03-28T10:11:31Z

develop (39adb65, blue) vs. PR (d54611a, orange)

radiuss_develop.csv
radiuss_35681.csv
radiuss.txt

No impact on concretization time:

hppritcha

Looks good to me! can't seem to check off.

tgamblin · 2023-03-28T21:28:13Z

@hppritcha it's already merged :) @alalazo is just posting reports on concretization speed on PRs so we can track which ones may have impacted performance.

…5681) This commit formalizes `satisfies(lhs, rhs, strict=True/False)` and splits it into two functions: `satisfies(lhs, rhs)` and `intersects(lhs, rhs)`. - `satisfies(lhs, rhs)` means: all concrete specs matching the left hand side also match the right hand side - `intersects(lhs, rhs)` means: there exist concrete specs matching both lhs and rhs. `intersects` now has the property that it's commutative, which previously was not guaranteed. For abstract specs, `intersects(lhs, rhs)` implies that `constrain(lhs, rhs)` works. What's *not* done in this PR is ensuring that `intersects(concrete, abstract)` returns false when the abstract spec has additional properties not present in the concrete spec, but `constrain(concrete, abstract)` will raise an error. To accomplish this, some semantics have changed, as well as bugfixes to ArchSpec: - GitVersion is now interpreted as a more constrained version - Compiler flags are interpreted as strings since their order is important - Abstract specs respect variant type (bool / multivalued)

spackbot-app bot added commands core PR affects Spack core functionality dependencies environments intel modules new-version python tests General test capability(ies) update-package utilities versions labels Feb 24, 2023

spackbot-app bot requested review from adamjstewart, hppritcha, michaelkuhn, pradyunsg, rscohn2, scheibelp and skosukhin February 24, 2023 17:06

alalazo commented Feb 24, 2023

View reviewed changes

haampie reviewed Feb 24, 2023

View reviewed changes

lib/spack/spack/variant.py Show resolved Hide resolved

haampie reviewed Feb 24, 2023

View reviewed changes

lib/spack/spack/variant.py Show resolved Hide resolved

hppritcha previously approved these changes Feb 24, 2023

View reviewed changes

haampie reviewed Feb 24, 2023

View reviewed changes

lib/spack/spack/spec.py Outdated Show resolved Hide resolved

alalazo dismissed hppritcha’s stale review via 45e0dfb February 24, 2023 18:27

spackbot-app bot added the fetching label Feb 24, 2023

alalazo force-pushed the refactor/split_satisfies_intersects branch from 45e0dfb to a03fbd0 Compare February 27, 2023 07:27

alalazo added 8 commits March 4, 2023 00:09

Remove check_constrain, add the tests back as parameters

8404391

Remove check_invalid_constrain, add the tests back as parameters

41a860d

Add parametrized tests to check is a constraint changes a spec or not

9a90d9f

Fix semantic of variant intersection

3b89375

Fix flags semantic

3a5b534

Remove outdated comment

de9e674

Simplify version satisfies

9e97a44

Add a comment clarifying invariant when entering a function

00f68d9

alalazo commented Mar 3, 2023

View reviewed changes

alalazo added this to the v0.20.0 milestone Mar 3, 2023

alalazo requested a review from haampie March 3, 2023 23:40

Inline Spec._satisfies_dependency

cf3a79a

haampie approved these changes Mar 8, 2023

View reviewed changes

haampie merged commit d54611a into spack:develop Mar 8, 2023

alalazo deleted the refactor/split_satisfies_intersects branch March 8, 2023 12:01

alalazo mentioned this pull request Mar 8, 2023

Spec.__contains__ produces wrong results for intel-oneapi-mkl #35597

Closed

3 tasks

haampie mentioned this pull request Mar 13, 2023

julia: Some improvements to the package #36054

Merged

albestro mentioned this pull request Mar 15, 2023

Problem with git versions #36134

Closed

3 tasks

This was referenced Mar 17, 2023

Environment concretization NOP with no error #36190

Closed

spec.satisfies needs to be refactored (behaves wrongly inside packages) #4135

Closed

hppritcha reviewed Mar 28, 2023

View reviewed changes

alalazo mentioned this pull request Apr 7, 2023

Properly define the meaning of x.satisfies(y, strict). #29248

Closed

tgamblin mentioned this pull request Sep 29, 2025

spack info: show conditional dependencies and licenses; allow filtering #51137

Merged

8 tasks

Conversation

alalazo commented Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spackbot-app bot commented Feb 24, 2023

Uh oh!

Uh oh!

alalazo Feb 24, 2023 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adamjstewart commented Feb 24, 2023

Uh oh!

alalazo commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo Mar 3, 2023 • 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!

alalazo commented Mar 6, 2023

Uh oh!

scheibelp commented Mar 7, 2023

Uh oh!

haampie commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented Mar 8, 2023

Uh oh!

alalazo commented Mar 28, 2023

Uh oh!

hppritcha left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alalazo commented Feb 24, 2023 •

edited

Loading

alalazo Feb 24, 2023 •

edited

Loading

alalazo commented Feb 27, 2023 •

edited

Loading

alalazo Mar 3, 2023 •

edited

Loading

haampie commented Mar 7, 2023 •

edited

Loading