ASP-based solver: a package eligible to provide a virtual must provide it#26875
Conversation
spackbot-app
bot
added
dependencies
new-version
tests
|
... there's still something odd. |
|
Yep, this needs an adjustment. |
…e it fixes spack#26866 This semantics fits with the way Spack currently treats providers of virtual dependencies. It needs to be revisited when spack#15569 is reworked with a new syntax.
alalazo
force-pushed
the
fixes/multiple_possible_providers
branch
from
5aadfe6 to
2fc5e64
Compare
|
Woohoo, thanks @alalazo! I don't know how you did it, but it works! |
|
@spackbot run pipeline |
|
I've started that pipeline for you! |
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
Ok, I looked through the virtual logic and it's getting kind of convoluted. I have some questions.
High level: I think that we should rename provides_virtual for consistency. We have dependency_holds, which can be "canceled", e.g., if something is an external and we therefore do not model its deps. provides_virtual has pretty much the same meaning for virtuals. cf.:
% a dependency holds if its condition holds
dependency_holds(Package, Dependency, Type) :-
dependency_condition(ID, Package, Dependency),
dependency_type(ID, Type),
condition_holds(ID),
not external(Package).and
% The provider provides the virtual if some provider condition holds.
provides_virtual(Provider, Virtual) :-
provider_condition(ID, Provider, Virtual),
condition_holds(ID),
virtual(Virtual).They're both the same idea -- we should call provides_virtual something like virtual_condition_holds so that we don't confuse it with provider. It just means the triggers are present for something to be a virtual, not that it is one.
In addition to this, I have some questions about the logic for virtuals as a whole.
@alalazo: thoughts?
lib/spack/spack/solver/concretize.lp
Outdated
| % If a package meets the condition to be a provider, it needs to be a provider | ||
| :- not provider(Package, Virtual), | ||
| provides_virtual(Package, Virtual), | ||
| virtual_node(Virtual). |
There was a problem hiding this comment.
I think these three rules can be simplified:
-
(1)
:- not provider(Package, Virtual), provides_virtual(Package, Virtual), virtual_node(Virtual).
-
(2)
:- provides_virtual(Package, V1), provides_virtual(Package, V2), V1 != V2, provider(Package, V1), not provider(Package, V2), virtual_node(V1), virtual_node(V2).
-
(3)
provider(Package, Virtual) :- root(Package), provides_virtual(Package, Virtual).
-
(4)
1 { provider(Package, Virtual) : possible_provider(Package, Virtual) } 1 :- virtual_node(Virtual).
(1) (this one I'm commenting) is just a stronger version of (2) (the one below) -- it says you can't have a provider condition that holds if you're not going to make that thing the provider. (2) seems like it does the same thing, but only if a package can provide two or more virtuals, which is weird. I think (1) covers that case already -- it just says they all have to be providers. So I think these are redundant.
But, if I look a little deeper, can't we replace (1), (2), and (3) with a simpler (3)? Basically can't the first 3 rules just become:
provider(Package, Virtual) :- node(Package), provides_virtual(Package, Virtual).What am I missing here? If you write the top 3 rules this way, can't you then also replace (4) with something like this:
:- 1 < #count { provider(Package, Virtual) }, virtual_node(Virtual).We've done this before and seen performance improvements, and the changes above would reuse the 4 rules here with 2 much simpler ones, unless I'm getting confused, which is entirely possible.
There was a problem hiding this comment.
Update -- tried this:
provider(Package, Virtual) :- node(Package), provides_virtual(Package, Virtual).as a replacement for the first 3 rules, and all of test/concretize.py passes. So that's good.
When I replaced (4) with my count thing above, things started failing, so I obviously have something wrong there. We can at least do the first simplification. I'll fiddle with using #count and an integrity constraint a bit more later.
@alalazo what do you think?
There was a problem hiding this comment.
Ok never mind the rewriting this as an integrity constraint; we need the rule as-is to turn virtual_nodes into providers:
1 { provider(Package, Virtual) : possible_provider(Package, Virtual) } 1
:- virtual_node(Virtual).I've pushed the other simplifications I suggested, though. We can revert the commit if @alalazo doesn't like them.
These three rules in `concretize.lp` are overly complex:
```prolog
:- not provider(Package, Virtual),
provides_virtual(Package, Virtual),
virtual_node(Virtual).
```
```prolog
:- provides_virtual(Package, V1), provides_virtual(Package, V2), V1 != V2,
provider(Package, V1), not provider(Package, V2),
virtual_node(V1), virtual_node(V2).
```
```prolog
provider(Package, Virtual) :- root(Package), provides_virtual(Package, Virtual).
```
and they can be simplified to just:
```prolog
provider(Package, Virtual) :- node(Package), provides_virtual(Package, Virtual).
```
- [x] simplify virtual rules to just one implication
- [x] rename `provides_virtual` to `virtual_condition_holds`
|
Thx! |
3 participants
fixes #26866
This semantics fits with the way Spack currently treats providers of virtual dependencies. It needs to be revisited when #15569 is reworked with a new syntax.
Modifications: