Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Concretizer: fix duplicate count when strategy=full #46169

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Concretizer: fix duplicate count when strategy=full #46169

wants to merge 6 commits into from

Conversation

elenimath
Copy link
Contributor

Depends on #46106

This PR resolves the error reported by @muffgaga in #46106 (comment) when using the "full" concretization strategy.

The previous implementation did not always correctly handle the selection of packages that were allowed to have multiple nodes in the DAG, because self._total_build is a set, so packages that appeared multiple times in the build subgraph were not counted correctly.

With this PR, the FullDuplicatesCounter instead:

  • counts the number of dependents (incoming edges) of each package in the DAG
  • selects all the packages tagged build-tool that have more than 1 incoming edge
  • the subGraphs of those packages are allowed to have multiple possible nodes

Additionally, this adds 3 tests (that would fail before the PR):

alalazo and others added 6 commits August 29, 2024 18:36
@alalazo @muffgaga
Concretizer: build-tools with build->run dep chain in unification set (…
a4794c9 
…fixes spack#46034)

* Each parent gets its own "build environment" where the build deps are
  unified.
* Things that don't allow multiple nodes are also thrown into the
  "generic_build" unification

In spack#46034, a package X build-depends on a `build-tools`-tagged package,
e.g., `py-setuptools`, as well as another package Y that itself depends
(build+run) on the same `build-tools` tagged package. In this case,
the concretizer puts the possibly two versions of the build tool into
separate unification sets, but the run-type dependency requires the
build environment of X to have both versions in its environment — which
is illegal.
With this fix, the concretizer puts the two build tool nodes into a new
common unification set if there is a build→run-type dependency chain on
the build tool ⇒ spack now reports the expected error about not being
able to find a common concretization of the build tool.

Change-Id: I99257b225602c21d4f270fe16e68c1f8bd4b1eba
@muffgaga
fix: TestConcretizeSeparately tests involving multiple unification sets
7c352e4 
This introduces `-fixed` packages for py-shapely and py-floating which
depend on a `-fixed` py-numpy that does NOT run-depend on py-setuptools.
If we allow run deps of build deps, it causes the tool to appear multiple
times in the build environment ⇒ fail.

Change-Id: I1ef1fa0b8636c23626c7e2ea521d3b5438f97d0a
@elenimath @muffgaga
test: add build->run dep regression test in TestConcretizeSeparately
9ac60b3 
This introduces a regression test to ensure that the concretizer fails
when trying to solve a spec where we have a build tool in two versions and
the "lower" (in the DAG) version being also a run-dependency. If this
would be allowed, we could end up having multiple paths to different tool
versions/variants in the build env.

Change-Id: I050efc9471d89580f5546a1c56ab419891bcc792
@muffgaga
test: add build-tool->build-tool dep regression test
16c9e96 
Existing tests use py-setuptools for multiple-build-tool testing;
here we add a case where a build-tool ("cmake") depends on another one
("gmake").

Change-Id: I08c3144b30258a43eda152aa72e1379472660432
@elenimath
test: add build-tool->(build-tool->)run-dep regression tests for stra…
0cff98f 
…tegy=full

add tests where two different versions of a build-tool are needed, but have
conflicting dependencies (either direct or deeper in the build stack DAG)
@elenimath
Fix duplicate handling in the "full" concretization strategy
05ce02a 
Build-tools with multiple dependents and all packages in their subDAGs are allowed to have multiple nodes in the DAG.
@spackbot-app spackbot-app bot added core PR affects Spack core functionality dependencies new-variant new-version stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) labels Sep 2, 2024
elenimath
elenimath commented Sep 2, 2024
View reviewed changes
lib/spack/spack/solver/counter.py
Comment on lines +155 to +166
dep_tree = spack.package_base.possible_dependencies(
*self.specs, virtuals=self._possible_virtuals, depflag=self.all_types
)
gen.h2("Maximum number of nodes")
for pkg, count in sorted(counter.items(), key=lambda x: (x[1], x[0])):
count = min(count, 2)
gen.fact(fn.max_dupes(pkg, count))
dep_counter = collections.Counter(
[dep for pkg_deps in dep_tree.values() for dep in pkg_deps]
)
dupe_build_tools = set([n for n in build_tools if dep_counter[n] > 1])
possible_dupes = set(
spack.package_base.possible_dependencies(
*dupe_build_tools, virtuals=self._possible_virtuals, depflag=self.all_types
)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: The dependent-counting is just an optimisation, I just tried to follow the logic of the previous implementation.
Having just:

        build_tools = spack.repo.PATH.packages_with_tags("build-tools")
        possible_dupes = set(
            spack.package_base.possible_dependencies(
                *build_tools, virtuals=self._possible_virtuals, depflag=self.all_types
            )
        )

instead would also work and have the same results, I believe, but would create more duplicates/unification sets (than strictly needed).

@elenimath
Copy link
Contributor Author

When testing this with real packages, I noticed that concretization seems to hang indefinitely sometimes, and traced this to packages that depend on py-sphinx. I think the reason somehow is a cyclic dependency between an old version of py-sphinx and py-sphinx-rtd-theme (as everything worked fine when I removed it), however I am unsure why this results in concretization getting stuck.

@muffgaga muffgaga mentioned this pull request Sep 2, 2024
Open
@muffgaga
Copy link
Contributor

muffgaga commented Sep 2, 2024 •
edited
Loading

When testing this with real packages, I noticed that concretization seems to hang indefinitely sometimes, and traced this to packages that depend on py-sphinx. I think the reason somehow is a cyclic dependency between an old version of py-sphinx and py-sphinx-rtd-theme (as everything worked fine when I removed it), however I am unsure why this results in concretization getting stuck.

Maybe open an issue for this :) ? (And it's unrelated to this PR (even if it might have been hidden before)… i.e. should not affect the review :) )

@muffgaga muffgaga mentioned this pull request Sep 2, 2024
Open
3 tasks
@alalazo alalazo self-assigned this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alalazo alalazo

Labels
core PR affects Spack core functionality dependencies new-variant new-version stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elenimath @muffgaga @alalazo