concretizer: use only attr() for Spec attributes#31202
Conversation
|
This looks really good to me, I took a relatively close pass over it and found no issues at all past what @becker33 already mentioned. |
tgamblin
force-pushed
the
attr-encoding
branch
4 times, most recently
from
784b292 to
9f1ec6b
Compare
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
It seems there are tests that need their assertion slightly changed:
# ensure that variant diffs are in here the result
> assert ['variant_value', 'mpileaks debug False'] in c['a_not_b']
E AssertionError: assert ['variant_value', 'mpileaks debug False'] in [['attr', 'hash mpileaks qek6lz5qlm7tgckn5j3pcnlxxlj57nqe'], ['attr', 'variant_value mpileaks debug False']]
tgamblin
force-pushed
the
attr-encoding
branch
from
9f1ec6b to
6beaeec
Compare
tgamblin
force-pushed
the
attr-encoding
branch
3 times, most recently
from
b4de145 to
419c050
Compare
tgamblin
force-pushed
the
attr-encoding
branch
2 times, most recently
from
b719fe2 to
1030f8d
Compare
tgamblin
force-pushed
the
attr-encoding
branch
2 times, most recently
from
34feb96 to
61c6b74
Compare
| if name == "error": | ||
| priority = function_tuple[1][0] | ||
| return (-5, priority) | ||
| elif name == "hash": |
There was a problem hiding this comment.
This needs to be restored for error messages to work properly.
There was a problem hiding this comment.
| def error(self, priority, msg, *args): | ||
| msg = msg.format(*args) | ||
|
|
||
| # For variant formatting, we sometimes have to construct specs | ||
| # to format values properly. Find/replace all occurances of | ||
| # Spec(...) with the string representation of the spec mentioned | ||
| specs_to_construct = re.findall(r"Spec\(([^)]*)\)", msg) | ||
| for spec_str in specs_to_construct: | ||
| msg = msg.replace("Spec(%s)" % spec_str, str(spack.spec.Spec(spec_str))) | ||
| raise UnsatisfiableSpecError(msg) |
There was a problem hiding this comment.
This needs to be restored, looks like a rebase issue that removed it
There was a problem hiding this comment.
Nope -- this was intentional. It's moved to here: https://github.com/spack/spack/pull/31202/files#diff-50f1229db6f3f9305546f946f508c28f829ab7e778c0a6be4e3a9f4dd6c9b1b7R649-R662
| #defined virtual_node/1. | ||
| #defined virtual_root/1. | ||
| #defined virtual_condition_holds/2. | ||
| #defined external/1. |
There was a problem hiding this comment.
Shouldn't external be an attr, since it's something we use in the reconstruction of the spec?
There was a problem hiding this comment.
I don't see where we have a handler for it. We have a handle for external_spec_selected, and attr("external_spec_selected", Package, LocalIndex) is used to get the information back to the solver. That was previously in display.lp and is used by SpecBuilder, but external/1 is just input metadata.
| #show attr/2. | ||
| #show attr/3. | ||
| #show attr/4. |
There was a problem hiding this comment.
Is there any way to print only attr/N atoms which match a certain first argument? Before in complex debugging situations we could comment all but a few atoms. I wonder if now, that are all aggregated under the same name, we can do something similar.
There was a problem hiding this comment.
Unfortunately, there isn't.
|
I timed the solve on this list of specs: and obtained the following results: Plotting an histogram in seconds of the various phases, on this branch: while on
|
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
Only minor comments to the code. I'll try to check clingo internal stats next, to see if I can make any sense of them 🤯
| node_flag = fn.node_flag | ||
| node_flag_propagate = fn.node_flag_propagate | ||
| variant_propagate = fn.variant_propagate | ||
| node = fn.attr("node") |
There was a problem hiding this comment.
Wondering if using enum to collect all attr is a good idea or not 🤔
There was a problem hiding this comment.
Not all attr need to be herein spec_clauses. There are internal solver attributes that are also passed through our general conditions that need to be attr's but we don't want them to be listed here.
There was a problem hiding this comment.
Sure. I meant if we should enumerate all the literal strings we use in attr and use the enum throughout this entire file (instead of repeating the string everywhere).
tgamblin
force-pushed
the
attr-encoding
branch
from
61c6b74 to
4cd054a
Compare
|
@alalazo the performance numbers you reported are between 15 - 32% overhead for grounding vs. |
|
I'm attaching a tarball radiuss.tar.gz with some analysis I'm trying to do to get insight on the timing. All the numbers (atoms, choices, etc.) seem slightly lower in this new encoding, as we expect. The only one that is higher is "Problems". Using: I obtain the following statistics on and the following from this PR: |
|
I think this line might be interesting: which suggest to me that it takes more chaining of rules in this PR to find a conflict and backtrack. |
All Spec attributes are now represented as `attr(attribute_name, ... args ...)`, e.g.
`attr(node, "hdf5")` instead of `node("hdf5")`, as we *have* to maintain the `attr()`
form anyway, and it simplifies the encoding to just maintain one form of the Spec
information.
Background
----------
In #20644, we unified the way conditionals are done in the concretizer, but this
introduced a nasty aspect to the encoding: we have to maintain everything we want in
general conditions in two forms: `predicate(...)` and `attr("predicate", ...)`. For
example, here's the start of the table of spec attributes we had to maintain:
```prolog
node(Package) :- attr("node", Package).
virtual_node(Virtual) :- attr("virtual_node", Virtual).
hash(Package, Hash) :- attr("hash", Package, Hash).
version(Package, Version) :- attr("version", Package, Version).
...
```
```prolog
attr("node", Package) :- node(Package).
attr("virtual_node", Virtual) :- virtual_node(Virtual).
attr("hash", Package, Hash) :- hash(Package, Hash).
attr("version", Package, Version) :- version(Package, Version).
...
```
This adds cognitive load to understanding how the concretizer works, as you have to
understand the equivalence between the two forms of spec attributes. It also makes the
general condition logic in #20644 hard to explain, and it's easy to forget to add a new
equivalence to this list when adding new spec attributes (at least two people have been
bitten by this).
Solution
--------
- [x] remove the equivalence list from `concretize.lp`
- [x] simplify `spec_clauses()`, `condition()`, and other functions in `asp.py` that need
to deal with `Spec` attributes.
- [x] Convert all old-form spec attributes in `concretize.lp` to the `attr()` form
- [x] Simplify `display.lp`, where we also had to maintain a list of spec attributes. Now
we only need to show `attr/2`, `attr/3`, and `attr/4`.
- [x] Simplify model extraction logic in `asp.py`.
Performance
-----------
This seems to result in a smaller grounded problem (as there are no longer duplicated
`attr("foo", ...)` / `foo(...)` predicates in the program), but it also adds a slight
performance overhead vs. develop. Ultimately, simplifying the encoding will be a win,
particularly for improving error messages.
Notes
-----
This will simplify future node refactors in `concretize.lp` (e.g., not identifying nodes
by package name, which we need for separate build dependencies).
I'm still not entirely used to reading `attr()` notation, but I thnk it's ultimately
clearer than what we did before. We need more uniform naming, and it's now clear what is
part of a solution. We should probably continue making the encoding of `concretize.lp`
simpler and more self-explanatory. It may make sense to rename `attr` to something like
`node_attr` and to simplify the names of node attributes. It also might make sense to do
something similar for other types of predicates in `concretize.lp`.
tgamblin
force-pushed
the
attr-encoding
branch
from
4cd054a to
e114263
Compare
|
I'm a little confused because on my M1 I see basically no difference in timing (for hdf5 and ascent), and if I output the ASP and run it through One thought: are you running with a bootstrapped |
|
I got the stats in #31202 (comment) using |
|
Also, fwiw the details on my setup are:
so |
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
The average slowdown from the plots in #31202 (comment) is 5.85% - so I think it's acceptable even though I couldn't figure out the exact cause of it 😟
|
Ok looking at this again -- the total overhead vs. I really wish I knew where the differences are coming from. We should probably start updating the bootstrap binaries to newer clingo now that we require Python 3.6 |
All Spec attributes are now represented as `attr(attribute_name, ... args ...)`, e.g.
`attr(node, "hdf5")` instead of `node("hdf5")`, as we *have* to maintain the `attr()`
form anyway, and it simplifies the encoding to just maintain one form of the Spec
information.
Background
----------
In spack#20644, we unified the way conditionals are done in the concretizer, but this
introduced a nasty aspect to the encoding: we have to maintain everything we want in
general conditions in two forms: `predicate(...)` and `attr("predicate", ...)`. For
example, here's the start of the table of spec attributes we had to maintain:
```prolog
node(Package) :- attr("node", Package).
virtual_node(Virtual) :- attr("virtual_node", Virtual).
hash(Package, Hash) :- attr("hash", Package, Hash).
version(Package, Version) :- attr("version", Package, Version).
...
```
```prolog
attr("node", Package) :- node(Package).
attr("virtual_node", Virtual) :- virtual_node(Virtual).
attr("hash", Package, Hash) :- hash(Package, Hash).
attr("version", Package, Version) :- version(Package, Version).
...
```
This adds cognitive load to understanding how the concretizer works, as you have to
understand the equivalence between the two forms of spec attributes. It also makes the
general condition logic in spack#20644 hard to explain, and it's easy to forget to add a new
equivalence to this list when adding new spec attributes (at least two people have been
bitten by this).
Solution
--------
- [x] remove the equivalence list from `concretize.lp`
- [x] simplify `spec_clauses()`, `condition()`, and other functions in `asp.py` that need
to deal with `Spec` attributes.
- [x] Convert all old-form spec attributes in `concretize.lp` to the `attr()` form
- [x] Simplify `display.lp`, where we also had to maintain a list of spec attributes. Now
we only need to show `attr/2`, `attr/3`, and `attr/4`.
- [x] Simplify model extraction logic in `asp.py`.
Performance
-----------
This seems to result in a smaller grounded problem (as there are no longer duplicated
`attr("foo", ...)` / `foo(...)` predicates in the program), but it also adds a slight
performance overhead vs. develop. Ultimately, simplifying the encoding will be a win,
particularly for improving error messages.
Notes
-----
This will simplify future node refactors in `concretize.lp` (e.g., not identifying nodes
by package name, which we need for separate build dependencies).
I'm still not entirely used to reading `attr()` notation, but I thnk it's ultimately
clearer than what we did before. We need more uniform naming, and it's now clear what is
part of a solution. We should probably continue making the encoding of `concretize.lp`
simpler and more self-explanatory. It may make sense to rename `attr` to something like
`node_attr` and to simplify the names of node attributes. It also might make sense to do
something similar for other types of predicates in `concretize.lp`.
All Spec attributes are now represented as `attr(attribute_name, ... args ...)`, e.g.
`attr(node, "hdf5")` instead of `node("hdf5")`, as we *have* to maintain the `attr()`
form anyway, and it simplifies the encoding to just maintain one form of the Spec
information.
Background
----------
In spack#20644, we unified the way conditionals are done in the concretizer, but this
introduced a nasty aspect to the encoding: we have to maintain everything we want in
general conditions in two forms: `predicate(...)` and `attr("predicate", ...)`. For
example, here's the start of the table of spec attributes we had to maintain:
```prolog
node(Package) :- attr("node", Package).
virtual_node(Virtual) :- attr("virtual_node", Virtual).
hash(Package, Hash) :- attr("hash", Package, Hash).
version(Package, Version) :- attr("version", Package, Version).
...
```
```prolog
attr("node", Package) :- node(Package).
attr("virtual_node", Virtual) :- virtual_node(Virtual).
attr("hash", Package, Hash) :- hash(Package, Hash).
attr("version", Package, Version) :- version(Package, Version).
...
```
This adds cognitive load to understanding how the concretizer works, as you have to
understand the equivalence between the two forms of spec attributes. It also makes the
general condition logic in spack#20644 hard to explain, and it's easy to forget to add a new
equivalence to this list when adding new spec attributes (at least two people have been
bitten by this).
Solution
--------
- [x] remove the equivalence list from `concretize.lp`
- [x] simplify `spec_clauses()`, `condition()`, and other functions in `asp.py` that need
to deal with `Spec` attributes.
- [x] Convert all old-form spec attributes in `concretize.lp` to the `attr()` form
- [x] Simplify `display.lp`, where we also had to maintain a list of spec attributes. Now
we only need to show `attr/2`, `attr/3`, and `attr/4`.
- [x] Simplify model extraction logic in `asp.py`.
Performance
-----------
This seems to result in a smaller grounded problem (as there are no longer duplicated
`attr("foo", ...)` / `foo(...)` predicates in the program), but it also adds a slight
performance overhead vs. develop. Ultimately, simplifying the encoding will be a win,
particularly for improving error messages.
Notes
-----
This will simplify future node refactors in `concretize.lp` (e.g., not identifying nodes
by package name, which we need for separate build dependencies).
I'm still not entirely used to reading `attr()` notation, but I thnk it's ultimately
clearer than what we did before. We need more uniform naming, and it's now clear what is
part of a solution. We should probably continue making the encoding of `concretize.lp`
simpler and more self-explanatory. It may make sense to rename `attr` to something like
`node_attr` and to simplify the names of node attributes. It also might make sense to do
something similar for other types of predicates in `concretize.lp`.
|
develop (10d10b6, blue) vs. PR (8756204, orange) radiuss_develop.csv
There seems to be a small slowdown in solve and ground, comparable to what reported in #31202 (review) |
5 participants
concretizer: use only
attr()for Spec attributesAll Spec attributes are now represented as
attr(attribute_name, ... args ...), e.g.attr(node, "hdf5")instead ofnode("hdf5"), as we have to maintain theattr()form anyway, and it simplifies the encoding to just maintain one form of the Spec
information.
Background
In #20644, we unified the way conditionals are done in the concretizer, but this
introduced a nasty aspect to the encoding: we have to maintain everything we want in
general conditions in two forms:
predicate(...)andattr("predicate", ...). Forexample, here's the start of the table of spec attributes we had to maintain:
This adds cognitive load to understanding how the concretizer works, as you have to
understand the equivalence between the two forms of spec attributes. It also makes the
general condition logic in #20644 hard to explain, and it's easy to forget to add a new
equivalence to this list when adding new spec attributes (at least two people have been
bitten by this).
Solution
concretize.lpspec_clauses(),condition(), and other functions inasp.pythat needto deal with
Specattributes.concretize.lpto theattr()formdisplay.lp, where we also had to maintain a list of spec attributes. Nowwe only need to show
attr/2,attr/3, andattr/4.asp.py.Performance
This seems to result in a smaller grounded problem (as there are no longer duplicated
attr("foo", ...)/foo(...)predicates in the program), but it also adds a slightperformance overhead vs. develop. Ultimately, simplifying the encoding will be a win,
particularly for improving error messages.
Notes
This will simplify future node refactors in
concretize.lp(e.g., not identifying nodesby package name, which we need for separate build dependencies).
I'm still not entirely used to reading
attr()notation, but I thnk it's ultimatelyclearer than what we did before. We need more uniform naming, and it's now clear what is
part of a solution. We should probably continue making the encoding of
concretize.lpsimpler and more self-explanatory. It may make sense to rename
attrto something likenode_attrand to simplify the names of node attributes. It also might make sense to dosomething similar for other types of predicates in
concretize.lp.