target optimization: re-norm optimization scale so that 0 is best. #29926
Conversation
This file contains 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
|
@tgamblin this is the PR we discussed earlier |
tgamblin
moved this from In progress
to Review in progress
in Spack reuse specs by default
|
Ok I dug into this. I added this to packages:
ncurses:
target: [x86_64_v4]And bisected like this: git bisect start
git bisect good 8ddaa08ed2aacb4b5e587a33c625492cbdd4886e
git bisect bad develop
git bisect run bash -c '[ $(spack spec --json -lIt --reuse ncurses | jq -r ".spec.nodes[0].hash") != ljewpulsil7kybn6j7vnv7xspod6rgjy ]'You'll need to pick an installed hash and package name of your own for this to work. Note also that I reversed "bad" and "good" to get With that, it finds #29933 (268c671) as the place where this was fixed, and I can confirm that it's fixed on that commit and not fixed on Do we still need to merge this one? |
becker33
changed the title
becker33
changed the title
becker33
force-pushed
the
bugfix/preferred-targets
branch
from
de03eba
to
d3abd3b
Compare
|
@tgamblin I don't think we need to merge it with any urgency, but we discussed out of channel that it would be good renorm these so 0 is best. |
|
This needs to be reworked to move the math back to the asp.py level, because math in clingo is killing performance. |
alalazo
previously requested changes
May 25, 2022
lib/spack/spack/solver/concretize.lp
Outdated
| node(Package), | ||
| not derive_target_from_parent(_, Package), | ||
| not package_target_weight(Target, Package, _). | ||
| not package_target_weight(Target, Package, _), | ||
| Offset = #count{W : package_target_weight(_, Package, W)}, |
There was a problem hiding this comment.
I agree with the PR but as discussed we should probably move the math inside asp.py, since clingo isn't particularly good at that. For instance trilinos on develop:
Time:
setup: 7.0143
load: 0.0202
ground: 2.5024
solve: 3.2076
Total: 12.9201
while with this branch:
Time:
setup: 6.9622
load: 0.0203
ground: 2.5175
solve: 15.6529
Total: 25.3843
There was a problem hiding this comment.
I think this needs to be fixed before we can merge.
tgamblin
requested changes
May 25, 2022
lib/spack/spack/solver/concretize.lp
Outdated
| node(Package), | ||
| not derive_target_from_parent(_, Package), | ||
| not package_target_weight(Target, Package, _). | ||
| not package_target_weight(Target, Package, _), | ||
| Offset = #count{W : package_target_weight(_, Package, W)}, |
There was a problem hiding this comment.
I think this needs to be fixed before we can merge.
becker33
force-pushed
the
bugfix/preferred-targets
branch
from
03e7174
to
18c87aa
Compare
tgamblin
approved these changes
May 28, 2022
tgamblin
pushed a commit
that referenced
this pull request
May 28, 2022
…29926) referred targets are currently the only minimization criteria for Spack for which we allow negative values. That means Spack may be incentivized to add nodes to the DAG if they match the preferred target. This PR re-norms the minimization criteria so that preferred targets are weighted from 0, and default target weights are offset by the number of preferred targets per-package to calculate node_target_weight. Also fixes a bug in the test for preferred targets that was making the test easier to pass than it should be.
iarspider
pushed a commit
to iarspider/spack
that referenced
this pull request
May 30, 2022
…pack#29926) referred targets are currently the only minimization criteria for Spack for which we allow negative values. That means Spack may be incentivized to add nodes to the DAG if they match the preferred target. This PR re-norms the minimization criteria so that preferred targets are weighted from 0, and default target weights are offset by the number of preferred targets per-package to calculate node_target_weight. Also fixes a bug in the test for preferred targets that was making the test easier to pass than it should be.
3 tasks
bhatiaharsh
pushed a commit
to bhatiaharsh/spack
that referenced
this pull request
Aug 8, 2022
…pack#29926) referred targets are currently the only minimization criteria for Spack for which we allow negative values. That means Spack may be incentivized to add nodes to the DAG if they match the preferred target. This PR re-norms the minimization criteria so that preferred targets are weighted from 0, and default target weights are offset by the number of preferred targets per-package to calculate node_target_weight. Also fixes a bug in the test for preferred targets that was making the test easier to pass than it should be.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Preferred targets are currently the only minimization criteria for Spack for which we allow negative values. That means Spack may be incentivized to add nodes to the DAG if they match the preferred target.
This PR re-norms the minimization criteria so that preferred targets are weighted from 0, and default target weights are offset by the number of preferred targets per-package to calculate node_target_weight.
Also fixes a bug in the test for preferred targets that was making the test easier to pass than it should be.