Skip to content

Avoid verifying variants in default package requirements#35037

Merged
haampie merged 3 commits intospack:developfrom
alalazo:bugfix/requires_all
Feb 16, 2023
Merged

Avoid verifying variants in default package requirements#35037
haampie merged 3 commits intospack:developfrom
alalazo:bugfix/requires_all

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented Jan 19, 2023

fixes #35026

Default package requirements might contain variants that are not defined in each package, so we shouldn't verify them when emitting facts for the ASP solver.

@spackbot-app spackbot-app bot added core PR affects Spack core functionality tests General test capability(ies) labels Jan 19, 2023
alalazo
alalazo commented Jan 19, 2023
View reviewed changes
lib/spack/spack/solver/concretize.lp Outdated
#minimize{ 0@75: #true }.
#minimize {
Weight@75+Priority
Weight@75+Priority,Package
Copy link
Copy Markdown
Member Author

@alalazo alalazo Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate bug fixed here.

@alalazo alalazo added this to the v0.19.1 milestone Jan 19, 2023
@eugeneswalker
Copy link
Copy Markdown
Contributor

eugeneswalker commented Jan 19, 2023

It looks like a lot of deprecated versions are now being selected:

$> spack -e . concretize -f
==> Starting concretization pool with 16 processes
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
==> Warning: using "[email protected]" which is a deprecated version
...

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 19, 2023 •
edited
Loading

@eugeneswalker Can you try:

require:
- any_of: [+mpi, '@:']
- any_of: [cuda_arch=80, '@:']
- any_of: [amdgpu_target=gfx90a, '@:']

instead ?

EDIT: Trying this myself, still getting your results. Will check tomorrow what drives the solutions there.

@alalazo alalazo marked this pull request as draft January 19, 2023 19:23
@alalazo alalazo marked this pull request as ready for review January 19, 2023 19:34
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 19, 2023

@eugeneswalker Working hypothesis, will try to confirm tomorrow:

Requirements are higher priority than any other optimization criteria. To minimize the impact of adding penalties when e.g. a variant doesn't exist under all clingo is minimizing the number of nodes, hence the older versions.

@tgamblin tgamblin self-requested a review January 22, 2023 01:14
@alalazo
Avoid verifying variants in default package requirements
eb2eaef 
fixes spack#35026

Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.
@alalazo
Account for group when enforcing requirements
c0a36f0
@alalazo
packages:all : don't emit facts for requirement conditions that can't…
4e236ac 
… apply to current spec
@alalazo alalazo force-pushed the bugfix/requires_all branch from e18a0bb to 4e236ac Compare January 23, 2023 10:19
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 23, 2023

@eugeneswalker This should be fixed. I still have some issues concretizing rocm stuff on my laptop, but I think it's due to some rocm modeling issues, rather than issues with requirements.

@alalazo alalazo requested a review from scheibelp January 23, 2023 12:25
@haampie haampie modified the milestones: v0.19.1, v0.19.2 Feb 9, 2023
@haampie haampie merged commit 50691cc into spack:develop Feb 16, 2023
@alalazo alalazo deleted the bugfix/requires_all branch February 16, 2023 10:58
@haampie haampie mentioned this pull request Feb 16, 2023
Merged
13 tasks
haampie pushed a commit that referenced this pull request Feb 16, 2023
@alalazo @haampie
Avoid verifying variants in default package requirements (#35037)
0c13b6b 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
haampie pushed a commit that referenced this pull request Feb 16, 2023
@alalazo @haampie
Avoid verifying variants in default package requirements (#35037)
faf0e4f 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
alalazo added a commit that referenced this pull request Feb 16, 2023
@alalazo
Avoid verifying variants in default package requirements (#35037)
959dc4c 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
techxdave pushed a commit to Tech-XCorp/spack that referenced this pull request Feb 17, 2023
@alalazo @techxdave
Avoid verifying variants in default package requirements (spack#35037)
a0de9a1 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
koysean pushed a commit to koysean/spack that referenced this pull request Feb 20, 2023
@alalazo @koysean
Avoid verifying variants in default package requirements (spack#35037)
252aacd 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Mar 27, 2023

develop (ce693ff, blue) vs PR (50691cc, orange)

radiuss_develop.csv
radiuss_35037.csv
radiuss.txt

Oddly enough, this bugfix seem to improve some specs in grounding time. The timing though is already higher than that reported in #31202 1

totals

Footnotes

  1. this seems to suggest some PR not affecting directly the concretizer to be the cause of a general increase in time ↩

haampie pushed a commit that referenced this pull request Apr 3, 2023
@alalazo @haampie
Avoid verifying variants in default package requirements (#35037)
63380ce 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
alalazo added a commit that referenced this pull request Apr 3, 2023
@alalazo
Avoid verifying variants in default package requirements (#35037)
2536dd5 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
jmcarcell pushed a commit to key4hep/spack that referenced this pull request Apr 13, 2023
@alalazo @jmcarcell
Avoid verifying variants in default package requirements (spack#35037)
90131dc 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
iamashwin99 pushed a commit to mpsd-computational-science/spack that referenced this pull request Aug 24, 2023
@alalazo @iamashwin99
Avoid verifying variants in default package requirements (spack#35037)
48e3f0a 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
@alalazo alalazo mentioned this pull request Nov 22, 2023
Merged
vjranagit pushed a commit to vjranagit/spack that referenced this pull request Jan 18, 2026
@alalazo
Avoid verifying variants in default package requirements (spack#35037)
6741255 
Default package requirements might contain
variants that are not defined in each package,
so we shouldn't verify them when emitting facts
for the ASP solver.

Account for group when enforcing requirements

packages:all : don't emit facts for requirement conditions
that can't apply to current spec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haampie haampie haampie approved these changes

@tgamblin tgamblin Awaiting requested review from tgamblin

@scheibelp scheibelp Awaiting requested review from scheibelp

Assignees

No one assigned

Labels

core PR affects Spack core functionality tests General test capability(ies)

Projects

None yet

Milestone

v0.19.2

Development

Successfully merging this pull request may close these issues.

Spec constraints under the "all" attribute in package requirements should not be validated

3 participants

@alalazo @eugeneswalker @haampie