Package requirements: support cflags#37180
Closed
scheibelp wants to merge 13 commits intospack:developfrom
Closed
Conversation
spackbot-app
bot
added
core
alalazo
reviewed
Apr 25, 2023
scheibelp
changed the title
alalazo
reviewed
Apr 28, 2023
Member
|
@spackbot run pipeline |
|
I've started that pipeline for you! |
alalazo
reviewed
May 4, 2023
Comment on lines
+2079
to
+2082
| for spec in specs: | ||
| if not spec.virtual: | ||
| for provided in spec.package_class.provided: | ||
| self.possible_virtuals.add(provided.name) |
Member
There was a problem hiding this comment.
I'll give a thorough review tomorrow, but curious why this was needed.
Member
Author
There was a problem hiding this comment.
When I introduced a choice rule for impose, it gave the concretizer the power to impose selective attributes about nodes without them being part of the dag, which was nonsensical, so I introduced
error(1, "Unexpected attribute {0}/{1}", Package, X) :- attr(X, Package), not attr("virtual_node", Package), not external(Package), not attr("node", Package).
i.e. things must be part of the dag for anny attribute to be set on them. Interestingly, this led to the discovery that in some circumstances there are "entities" in our DAG which are not nodes or virtual nodes but still have attributes set on them. I think specifically it is providers that are specified as root specs (but you can know for sure by removing this, which will cause test faulures), so logic like this was added to ensure that everything that we set attrs on is in fact a node or virtual node (or external).
Member
|
The PR seem to slow-down by 10% the solver, at least on the benchmark below. I'll double check whether this is due to the choice rules or errors (more likely), or both. Benchmark data: radiuss-impose.csv
Total time and speed-up
|
Member
|
After a few experiments, it seems that the main cause of slow-down are the choice rules. With more specific choice rules: diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index c38b67da62..836147c511 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -193,18 +193,15 @@ attr(Name, A1, A2) :- impose(ID), imposed_constraint(ID, Name, A1, A2).
attr(Name, A1, A2, A3) :- impose(ID), imposed_constraint(ID, Name, A1, A2, A3).
attr(Name, A1, A2, A3, A4) :- impose(ID), imposed_constraint(ID, Name, A1, A2, A3, A4).
-% If something is mentioned as a possible constraint, allow for the possibility
-% of imposing it
-{impose(ID)} :- imposed_constraint(ID, Name, A1).
-{impose(ID)} :- imposed_constraint(ID, Name, A1, A2).
-{impose(ID)} :- imposed_constraint(ID, Name, A1, A2, A3).
-{impose(ID)} :- imposed_constraint(ID, Name, A1, A2, A3, A4).
-
-% Packages can only set attrs if they are part of the DAG
-error(1, "Unexpected attribute {0}/{1}", Package, X) :- attr(X, Package), not attr("virtual_node", Package), not external(Package), not attr("node", Package).
-error(1, "Unexpected attribute {0}/{1}: {2}", Package, X1, X2) :- attr(X1, Package, X2), not attr("virtual_node", Package), not external(Package), not attr("node", Package).
-error(1, "Unexpected attribute {0}/{1}: {2}, {3}", Package, X1, X2, X3) :- attr(X1, Package, X2, X3), not attr("virtual_node", Package), not external(Package), not attr("node", Package).
-error(1, "Unexpected attribute {0}/{1}: {2}, {3}, {4}", Package, X1, X2, X3, X4) :- attr(X1, Package, X2, X3, X4), not attr("virtual_node", Package), not external(Package), not attr("node", Package).
+{ attr("node_flag", A1, A2, A3) } :-
+ requirement_group_member(Y, Package, _),
+ activate_requirement_rules(Package),
+ condition_requirement(Y,"node_flag", A1, A2, A3).
+
+{ attr("node_flag_source", A1, A2, A3) } :-
+ requirement_group_member(Y, Package, _),
+ activate_requirement_rules(Package),
+ condition_requirement(Y,"node_flag_source", A1, A2, A3).
virtual(Virtual) :- possible_provider(Package, Virtual).
attr("virtual_node", Virtual) :- provider(Package, Virtual).I get a smaller impact (2-5%) on performance: radiuss-impose3.csv
Total time and speed-up
|
alalazo
requested changes
May 5, 2023
Member
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
See the performance benchmarks above and a proposed improvement. I have also a question on self.possible_virtuals.
Member
Author
|
(closing as these issues are now articulated in #39880) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #37163
Fixes #34241
This addresses a specific problem in a generic way. Essentially compiler flags introduced as possible package constraints were unsatisfiable, since the concretizer had no means to include them in the solution (another good explanation in the issue thread). This PR attempts to resolve the issue generally by allowing the solver to impose anything that is mentioned as a possible constraint (I anticipate that this would mitigate future issues of a similar nature).
Looking at the overall .lp
so we have
impose -> attr -> condition_holds -> imposeand generally the cycle is broken when either
You can see this in action by comparing the facts that are generated by the various methods for assigning
cflags=-g:at its core, this allows another mechanism for breaking the cycle by adding a choice rule for
impose().