Make "minimal" the default duplicate strategy#39621
Merged
alalazo merged 6 commits intospack:developfrom Oct 6, 2023
Merged
Conversation
Member
Author
|
This is the current situation when trying to compare "no duplicates" with "minimal duplicates" on the current e4s stack: e4s-develop-serial-minimal.csv
Table results
Concretization is 10%-30% slower when using that mode of operation. I'll check if I can improve it in some way with heuristics. |
Member
|
Would be good to add Edit: this was done. |
00944c2 to
5e2d0e2
Compare
Member
|
@alalazo I've reduced it to: # example-one/package.py
from spack.package import *
class ExampleOne(Package):
version("1.0")
depends_on("example-provider", type="build")
# example-two/package.py
from spack.package import *
class ExampleTwo(Package):
version("1.0")
provides("example-provider")Fails to concretize, but it does concretize when: a) you replace the virtual with the provider: Finally, this is not caused by 5e2d0e2, but the bug happens on develop too 👍 |
Member
Author
I'll have a look at this today. |
fd7bf11 to
7ec19db
Compare
5367ea5 to
0cf2122
Compare
Member
Author
|
Tested on:
radiuss_none.csv
Table results
The slow-down when compared to no duplicates is in the range 5-15% |
Member
Author
|
@spackbot rebuild everything |
|
I've started that pipeline for you! |
Member
Author
|
So, after a too long debugging, it seems clingo is behaving as it should in https://gitlab.spack.io/spack/spack/-/pipelines/506271 The fact that a
I'll be updating the pipelines to use |
77f14af to
c2d9598
Compare
haampie
reviewed
Sep 28, 2023
haampie
reviewed
Sep 28, 2023
haampie
reviewed
Sep 28, 2023
Member
Author
|
@spackbot run pipeline |
|
I've started that pipeline for you! |
520452d to
44e8112
Compare
* Allow branching out of the "generic build" unification set For cases like the one in spack#39661 we need to relax rules on unification sets. The issue is that, right now, nodes in the "generic build" unification set are unified together with their build dependencies. This was done out of caution to avoid the risk of circular dependencies, which would ultimately cause a very slow solve. For build-tools like Cython, however, the build dependencies is masked by a long chain of "build, run" dependencies that belong in the "generic build" unification space. To allow splitting on cases like this, we relax the rule disallowing branching out of the "generic build" unification set. * Fix issue with pure build virtual dependencies Pure build virtual dependencies were not accounted properly in the list of possible virtuals. This caused some facts connecting virtuals to the corresponding providers to not be emitted, and in the end lead to unsat problems.
44e8112 to
911624e
Compare
haampie
reviewed
Oct 6, 2023
911624e to
5e1ee12
Compare
haampie
approved these changes
Oct 6, 2023
RikkiButler20
pushed a commit
to RikkiButler20/spack
that referenced
this pull request
Nov 2, 2023
* Allow branching out of the "generic build" unification set For cases like the one in spack#39661 we need to relax rules on unification sets. The issue is that, right now, nodes in the "generic build" unification set are unified together with their build dependencies. This was done out of caution to avoid the risk of circular dependencies, which would ultimately cause a very slow solve. For build-tools like Cython, however, the build dependencies is masked by a long chain of "build, run" dependencies that belong in the "generic build" unification space. To allow splitting on cases like this, we relax the rule disallowing branching out of the "generic build" unification set. * Fix issue with pure build virtual dependencies Pure build virtual dependencies were not accounted properly in the list of possible virtuals. This caused some facts connecting virtuals to the corresponding providers to not be emitted, and in the end lead to unsat problems. * Fixed a few issues in packages py-gevent: restore dependency on py-cython@3 jsoncpp: fix typo in build dependency ecp-data-vis-sdk: update spack.yaml and cmake recipe py-statsmodels: add v0.13.5 * Make dependency on "blt" of type "build"
RikkiButler20
pushed a commit
to RikkiButler20/spack
that referenced
this pull request
Jan 11, 2024
* Allow branching out of the "generic build" unification set For cases like the one in spack#39661 we need to relax rules on unification sets. The issue is that, right now, nodes in the "generic build" unification set are unified together with their build dependencies. This was done out of caution to avoid the risk of circular dependencies, which would ultimately cause a very slow solve. For build-tools like Cython, however, the build dependencies is masked by a long chain of "build, run" dependencies that belong in the "generic build" unification space. To allow splitting on cases like this, we relax the rule disallowing branching out of the "generic build" unification set. * Fix issue with pure build virtual dependencies Pure build virtual dependencies were not accounted properly in the list of possible virtuals. This caused some facts connecting virtuals to the corresponding providers to not be emitted, and in the end lead to unsat problems. * Fixed a few issues in packages py-gevent: restore dependency on py-cython@3 jsoncpp: fix typo in build dependency ecp-data-vis-sdk: update spack.yaml and cmake recipe py-statsmodels: add v0.13.5 * Make dependency on "blt" of type "build"
RikkiButler20
pushed a commit
to RikkiButler20/spack
that referenced
this pull request
Jan 31, 2024
* Allow branching out of the "generic build" unification set For cases like the one in spack#39661 we need to relax rules on unification sets. The issue is that, right now, nodes in the "generic build" unification set are unified together with their build dependencies. This was done out of caution to avoid the risk of circular dependencies, which would ultimately cause a very slow solve. For build-tools like Cython, however, the build dependencies is masked by a long chain of "build, run" dependencies that belong in the "generic build" unification space. To allow splitting on cases like this, we relax the rule disallowing branching out of the "generic build" unification set. * Fix issue with pure build virtual dependencies Pure build virtual dependencies were not accounted properly in the list of possible virtuals. This caused some facts connecting virtuals to the corresponding providers to not be emitted, and in the end lead to unsat problems. * Fixed a few issues in packages py-gevent: restore dependency on py-cython@3 jsoncpp: fix typo in build dependency ecp-data-vis-sdk: update spack.yaml and cmake recipe py-statsmodels: add v0.13.5 * Make dependency on "blt" of type "build"
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 #38226
The current strategy is "none", meaning no duplicates. Marking as a draft since we need to decide whether or not to keep the current configuration schema.
We also need to improve speed for the
minimalcase, when no duplicate is actually required.Modifications:
concretizer:duplicates:strategy:minimalis now the defaultbuild/rundependencyImplementing #31511 is probably better deferred to another PR.