[WIP] Bugfix/cflags requires by scheibelp · Pull Request #39880 · spack/spack

scheibelp · 2023-09-07T22:04:45Z

It appears an extra layer of indirection has been added in the form of condition_effect and condition_trigger since the last time I tried to understand the concretizer (between the requirement group member and the imposed constraint).

I observed in #39671 that our basic test was passing even though a simple example outside of the tests was failing, and I can say that is because:

we use separate numbering spaces for the imposed constraints and requirement group members
these spaces overlap (i.e. both start from 0)

I think we should use a global ID numbering system since I think all sorts of strange bugs could result from this.

…de_flag_source

…gs I tried, since these specify exact condition ids - not very useful)

scheibelp · 2023-09-28T22:40:06Z

lib/spack/spack/solver/concretize.lp

 % then the provider is the root package.
 attr("root", PackageNode) :- attr("virtual_root", VirtualNode), provider(PackageNode, VirtualNode).

+attr("virtual_root", VirtualNode) :- attr("root", PackageNode), provider(PackageNode, VirtualNode).


This isn't needed, but seems reasonable.

Hmm, I need to think about that. virtual_root was used to signal that the root spec is a virtual. Not sure in which context this new rule is helpful 🤔

scheibelp · 2023-09-28T22:42:06Z

lib/spack/spack/solver/concretize.lp

+error(100, "e2") :- provider(_, VirtualNode), not attr("virtual_node", VirtualNode).
+error(100, "e3") :- provider(PackageNode, _), not attr("node", PackageNode).
+
+error(100, "e4") :- attr("root", node(ID, PackageNode)), ID > min_dupe_id.


Introducing these error facts helped me figure out why

packages: virtual-w: require: cflags="-O1"

doesn't work (i.e. requirements from the virtual that do not explicitly mention an implementation): virtuals can't propagate most constraints to nodes.

scheibelp · 2023-09-29T18:25:27Z

The CI failure appears legitimate in the sense that it is cause by this PR, but I'll note that the associated spack.yaml appears to be malformed (share/spack/gitlab/cloud_pipelines/stacks/e4s-oneapi/spack.yaml):

  packages:
    mpi:
      require: "mpich"
  specs:
  - openmpi

I'm not sure what was happening before, but I think this file should be updated.

alalazo · 2023-09-29T18:35:16Z

@scheibelp Fwiw it also seemed a bug to me, but this is why we support that #38447 (comment)

scheibelp · 2023-09-29T19:40:26Z

Fwiw it also seemed a bug to me, but this is why we support that #38447 (comment)

There seems to be some openness in that thread to changing the semantics. That is what I would support here. The most succinct way I can express my disagreement is to say that our modeling in concretize.lp is not prepared to handle this desire in a conceptually consistent manner, i.e. that the perfectly reasonable expression:

1 { attr("virtual_node", node(0..X-1, Virtual)) : max_dupes(Virtual, X) } :-
  virtual_condition_holds(node(ProviderID, Provider), Virtual),
  attr("node", node(ProviderID, Provider)).

is the cause of the CI failure (I have reproduced this in #40261): we are relying on obscure and hard-to-explain choices made by the concretizer to support this case (namely, the concretizer succeeds by avoiding the creation of a virtual node, not something I'd like to depend on). If we want to support it, we should support it in a way that makes sense.

scheibelp · 2023-09-30T00:12:43Z

lib/spack/spack/solver/concretize.lp

-  activate_requirement(node(ID, Package), X),
-  imposed_constraint(Y,"node_flag_source", A1, A2, A3).
+% None of the following 3 are needed for `virtual-w:require: w cflags="-O1"`
+% (1) and (3) are needed for `y:require: cflags="-g"`


Regarding this and your point

That said, the mpi case works also with this patch - I'll try to understand why since the names of the virtual "mpi" and of the package "openmpi" don't match the pattern above

I will note that emit_facts_from_requirement_rules has differing behavior for virtuals vs. providers:

For y: pkg_fact("y",trigger_id(0)). pkg_fact("y",trigger_msg("y cflags=\"-g\"")). condition_requirement(0,"node","y"). condition_requirement(0,"node_flag","y","cflags","-g"). condition_requirement(0,"node_flag_source","y","cflags","y"). For w: pkg_fact("virtual-w",trigger_id(0)). pkg_fact("virtual-w",trigger_msg("virtual-w")). condition_requirement(0,"virtual_node","virtual-w").

the rule is easily satisfied for virtuals, but for non-virtuals the "condition" is equal to the constraint, I think that's to support groups, but IMO it ought to be refactored into something sensible.

Update: none of these are required if using:

{ impose(EffectID, node(ID, Package)) } :- pkg_fact(Package, condition_effect(Y, EffectID)), requirement_group_member(Y, Package, X), activate_requirement(node(ID, Package), X), requirement_group(Package, X).

(added above)

…m condition" This reverts commit da47a3d.

This reverts commit 664e9af.

alalazo · 2023-10-17T14:20:33Z

lib/spack/spack/solver/counter.py

+        for x in specs:
+            if not x.virtual:
+                self._possible_virtuals.update(str(i) for i in x.package_class.provided.keys())


Ok, this would be part of the changes in semantic mentioned above. Not sure why we would need it though 🤔

alalazo · 2023-10-17T14:25:04Z

lib/spack/spack/solver/concretize.lp

+1 { attr("virtual_node", node(0..X-1, Virtual)) : max_dupes(Virtual, X) } :-
+  virtual_condition_holds(node(ProviderID, Provider), Virtual),
+  attr("node", node(ProviderID, Provider)).


This will not blend with the semantic we want to merge in #35322. If a condition to provide a virtual holds, it is not necessary ~~for the node to provide the virtual~~ for the virtual node to be present.

Two points:

This only says that if X provides a virtual Y, that Y must exist as a virtual. It does not say that X must be the provider for Y. Therefore I don't think it actually conflicts (at least with the high-level goal stated in Cherry-picking virtual dependencies #35322)

Without this, note that packages:<virtual>:require cannot guarantee imposing requirements on providers

packages: mpi: require: openmpi +shared

i.e. this can only place constraints on MPI right now when MPI is a (virtual) node in the DAG. Another option is to take constraints like require: openmpi+shared and generate a secondary requirement for the provider (whether or not it is providing something).

This only says that if X provides a virtual Y, that Y must exist as a virtual.

I don't read it exactly like that, but rather: if X satisfies the conditions to provide the virtual Y, then Y must exist as a virtual. This is not always true. For instance, if in a solve intel-parallel-studio is used only for mpi there does not need to be a lapack virtual node in the answer set.

I don't read it exactly like that, but rather: if X satisfies the conditions to provide the virtual Y, then Y must exist as a virtual.

Agree.

if in a solve intel-parallel-studio is used only for mpi there does not need to be a lapack virtual node in the answer set.

What do you think should happen for something like:

packages: lapack: require: - one_of: [openblas, netlib-lapack] ... spack spec intel-parallel-studio+mkl

?

(I don't think this is a counter example to your claim, but I am curious about the desired behavior).

Overall, it sounds like you'd prefer the second strategy I described in #39880 (comment) (namely, infer provider-specific requirements as provider-specific requirement groups).

What do you think should happen for something like: ...

I think it should install, since we asked for the intel-parallel-studio+mkl package explicitly.

alalazo · 2023-10-17T14:40:52Z

lib/spack/spack/solver/concretize.lp


 requirement_group_satisfied(node(ID, Package), X) :-
-  1 { condition_holds(Y, node(ID, Package)) : requirement_group_member(Y, Package, X) } 1,
+  1 { requirement_condition_satisfied(Y, node(ID, Package)) : requirement_group_member(Y, Package, X) } 1,


Just a reminder that we need to make "any_of" symmetric too, in case.

alalazo · 2023-10-17T15:04:16Z

lib/spack/spack/solver/concretize.lp

+{ impose(EffectID, node(ID, Package)) } :-
+  pkg_fact(Package, condition_effect(Y, EffectID)),
+  requirement_group_member(Y, Package, X),
+  activate_requirement(node(ID, Package), X),
+  requirement_group(Package, X).
+
+{ impose(EffectID, node(ProviderID, Provider)) } :-
+  pkg_fact(Provider, condition_effect(Y, EffectID)),
+  provider(node(ProviderID, Provider), node(VirtualID, Virtual)),
+  requirement_group_member(Y, Virtual, X),
+  activate_requirement(node(VirtualID, Virtual), X),
+  requirement_group(Virtual, X).


I'll look at this better, but I don't immediately see why we can give a choice to impose something without having anything that says that the corresponding trigger condition is satisfied in the body of the rule.

The only explanation I see is that rules in requirements are always of the kind:

%---------------------------------------------------------------------------- % Trigger conditions %---------------------------------------------------------------------------- pkg_fact("openmpi",trigger_id(0)). pkg_fact("openmpi",trigger_msg("openmpi%gcc+static")). condition_requirement(0,"node","openmpi"). condition_requirement(0,"variant_value","openmpi","static","True"). condition_requirement(0,"node_compiler","openmpi","gcc"). condition_requirement(0,"node_compiler_version_satisfies","openmpi","gcc",":"). %---------------------------------------------------------------------------- % Imposed requirements %---------------------------------------------------------------------------- pkg_fact("openmpi",effect_id(0)). pkg_fact("openmpi",effect_msg("openmpi%gcc+static")). imposed_constraint(0,"node","openmpi"). imposed_constraint(0,"variant_set","openmpi","static","True"). imposed_constraint(0,"node_compiler_set","openmpi","gcc"). imposed_constraint(0,"node_compiler_version_satisfies","openmpi","gcc",":").

but that also means they work logically backwards with this choice - we give choice to impose the rules because they are designed in such a way that imposing the effect implies the causes.

I don't immediately see why we can give a choice to impose something without having anything that says that the corresponding trigger condition is satisfied in the body of the rule.

In this case there is no "valid" corresponding trigger condition:

the condition requirements generated (the ones you list in the example directly above) are not actually for triggering the impose, but rather to record whether requirement group members are satisfied (technically they also trigger the impose, but they are effectively useless for that purpose)

currently the only "influence" that the requirement group creates are error conditions when their members are not satisfied

"somehow", we have a means to thwart error facts by deriving attributes for all requirements other than cflags

IMO, it is not worth understanding how or why this works, and saying that we are allowed to impose things if anyone might want to impose them is reasonable

scheibelp · 2024-11-07T21:57:28Z

This has been partially covered by #40639. Although it does more than that PR, I figure this should be closed until it can be connected to a wider issue.

scheibelp added 27 commits September 6, 2023 14:34

initial experimentation on direct specification of cflags

acf42d2

avoid set_trace (but keep comment for now)

b4f3179

comment out original constraints

849d58e

remove note

7a6255b

first attempt at amending current expression (not successful)

d8bf25f

more debugging

7cf13a1

more debugging

9f8d88d

more debugging

5f4b0b5

more debugging

54eb71d

more debugging

a671683

more debugging

937c167

corrected original stanza

0e819ff

update comment

aacfe9f

remove some comments

5a70b13

disable overly-generic enabling stanza with corrected original for no…

72bfbfd

…de_flag_source

remove older debug clauses (which were just templates for actual thin…

f7e4e53

…gs I tried, since these specify exact condition ids - not very useful)

it should be possible to use imposed constraints

8e9d57b

add the extra bit that should work (but doesnt)

bbbb594

add the extra bit that should work (but doesnt)

e59e268

restore currently working arrangement (but it should change)

46544ea

fix first stanza

90b3126

fix second stanza

17d1df9

remove overly generic requirement activators

8384d0a

remove debugging stanzas

d948964

remove reference commented original (wrong) stanzas

9882588

remove debugging statements from test

1137651

remove extra space introduced by this diff

ea69136

scheibelp requested a review from alalazo September 7, 2023 22:04

spackbot-app bot added the core PR affects Spack core functionality label Sep 7, 2023

alalazo self-assigned this Sep 8, 2023

dont access .provided attr for virtuals

bbf3a0e

scheibelp commented Sep 28, 2023

View reviewed changes

scheibelp added 4 commits September 28, 2023 15:46

consolidate and use more-careful constraint

96d07e5

clearer variable names

7f435b8

renaming variables for clarity (no logic change)

592de6a

add extra note from experiments

1557e7e

scheibelp mentioned this pull request Sep 29, 2023

[WIP] Virtual constraint #40261

Open

scheibelp commented Sep 30, 2023

View reviewed changes

scheibelp added 8 commits October 9, 2023 10:20

intermediate work on imposing based on IDs vs specs

664e9af

more (unsuccessful) attempts at disentangling requirement from condition

da47a3d

Revert "more (unsuccessful) attempts at disentangling requirement fro…

36d4259

…m condition" This reverts commit da47a3d.

Revert "intermediate work on imposing based on IDs vs specs"

6f9fef7

This reverts commit 664e9af.

generic impose option for requirement groups

e47d181

updates to improve requirements for virtuals

a384bfe

auto style fix

4e825d1

merge from develop (minor conflict)

0b1b6d9

scheibelp changed the title ~~Bugfix/cflags requires~~ [WIP] Bugfix/cflags requires Oct 11, 2023

alalazo reviewed Oct 17, 2023

View reviewed changes

scheibelp mentioned this pull request Oct 20, 2023

Package requirements: support cflags #37180

Closed

scheibelp added a commit to scheibelp/spack that referenced this pull request Oct 20, 2023

reapply targeted changes from spack#39880

fbaba18

scheibelp mentioned this pull request Oct 20, 2023

Cflags requirements: regression fix #40639

Merged

scheibelp closed this Nov 7, 2024

Conversation

scheibelp commented Sep 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp Sep 28, 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!

scheibelp commented Sep 29, 2023

Uh oh!

alalazo commented Sep 29, 2023

Uh oh!

scheibelp commented Sep 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo Oct 17, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 20, 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!

Choose a reason for hiding this comment

Uh oh!

alalazo Oct 17, 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!

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 17, 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!

scheibelp commented Nov 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scheibelp Sep 28, 2023 •

edited

Loading

alalazo Oct 17, 2023 •

edited

Loading

scheibelp Oct 20, 2023 •

edited

Loading

alalazo Oct 17, 2023 •

edited

Loading

scheibelp Oct 17, 2023 •

edited

Loading