concretizer: add mode to reuse dependencies only by michaelkuhn · Pull Request #30990 · spack/spack

michaelkuhn · 2022-06-04T10:54:50Z

This adds a new mode for concretizer:reuse called dependencies, which only reuses dependencies. Currently, spack install foo will reuse older versions of foo, which might be surprising to users.

I'm not entirely happy with the argument name (--reuse-deps) yet and would argue for making this the default reuse mode.

michaelkuhn · 2022-10-16T19:35:45Z

@alalazo I have fixed the conflicts and rebased to current develop. Any comments about this change?

haampie · 2022-10-21T12:59:59Z

@becker33 can you coordinate your "reuse from local installs/upstreams" vs "reuse from binary cache" with this PR? Just to avoid that we're ending up with --reuse="locally installed but not the root spec please"

adamjstewart · 2022-11-17T05:25:51Z

@becker33 you mentioned that this PR would need some work to update for changes in develop since it was created. What changes are required? Would love to get this in soon, as I think this is the most logical default concretization strategy.

michaelkuhn · 2022-11-17T07:57:51Z

I rebased this to current develop to fix some conflicts. I also did some tests and it still seems to work.

adamjstewart

I tested this with a few individual packages and a massive environment and it works perfectly, exactly the behavior I would find most intuitive. Great job on this!

michaelkuhn · 2022-12-11T00:56:13Z

@tgamblin @becker33 ping?

alalazo · 2022-12-12T15:40:45Z

lib/spack/spack/solver/asp.py

+        # If we only want to reuse dependencies, remove the root specs
+        if self.reuse == "dependencies":
+            reusable_specs = [
+                spec for spec in reusable_specs if not any(spec.satisfies(root) for root in specs)
+            ]


I think this is redundant, since it includes specs that have a root as a dependency. The solve will rule that out, but potentially we have more hashes than we want as facts. Did you try:

Suggested change

# If we only want to reuse dependencies, remove the root specs

if self.reuse == "dependencies":

reusable_specs = [

spec for spec in reusable_specs if not any(spec.satisfies(root) for root in specs)

]

# If we only want to reuse dependencies, remove the root specs

if self.reuse == "dependencies":

reusable_specs = [

spec for spec in reusable_specs if not any(root in spec for root in specs)

]

?

I might be reading the code wrong but aren't we hitting the anonymous case in __contains__ anyway, which simply calls satisfies: https://github.com/spack/spack/blob/develop/lib/spack/spack/spec.py#L4001-L4002
I would therefore assume that this behaves identically. I also did some tests and couldn't provoke any differences.

Reusable specs are all concrete specs collected either from the DB or from buildcaches, root specs can't be anonymous. I checked the ASP before posting. Result is the same, we just have a lot of input in the ASP which is useless.

Try for instance:

Install hdf5+mpi (fake installs are good for this test)

Create an environment with hdf5+mpi and zlib as root specs + reuse:dependencies

spack solve --show=asp for the environment

I think you'll see all the specs in there, but hdf5 and zlib There are specs though that reference the non-existing zlib and thus will never be selected.

I think there might be other potential pitfalls we might need to check, e.g. if I can reuse the same package as the root but with different variant in an environment. Going back to the example above do we want to reuse hdf5~mpi if it is installed, since the the root says hdf5+mpi? That matters if we have a root spec that depend on hdf5. I didn't test-drive that case yet.

I got what you meant now, thanks! I confused myself with the "root specs" in the snippet above and actually tested it outside of an environment (which hits the anonymous spec path).

I now did some tests inside an environment and could reproduce your results. I guess the interaction can be a bit surprising if a common dependency is a root spec: I added a new cmake version but since zlib is a root spec and thus filtered out, cmake gets rebuilt even though it could theoretically be reused. I guess there is no way around that, though.

If I add enzo (which requires hdf5~mpi) in your example above, everything seems to continue working as expected:

==> Error: 'hdf5' required multiple values for single-valued variant 'mpi' Requested '~mpi' and '+mpi'

I also made the change you suggested and pushed a new version.

adamjstewart · 2023-02-08T04:53:21Z

Any updates on this? I find myself needing this behavior again today. My new use case:

I often fix bugs in packages and want to build them to make sure the fix works. I'm faced with two options:

spack install foo: the package is already installed
spack install --fresh foo: rebuild the entire world

I would love the feature added in this PR (even more so if it becomes the default):

spack install --reuse-deps foo: only build foo

michaelkuhn · 2023-02-08T12:45:22Z

Any updates on this? I find myself needing this behavior again today. My new use case:

I incorporated @alalazo's feedback a while ago, so it should now work better in environments. From my side, this is good to go.

michaelkuhn · 2023-02-27T08:13:02Z

I still run into this every time I update a package:

$ spack install foo
# already installed
$ spack install foo@x.y.z

@alalazo ping 🙂

alalazo · 2023-03-01T10:51:59Z

I'll try to review this one by the end of the week.

alalazo

LGTM. I left a few minor comments. Let's wait a bit to check if people have opinion on the cli flag, otherwise this is ready to be merged.

alalazo · 2023-03-06T15:23:48Z

lib/spack/spack/solver/asp.py

                pass
+
+        # If we only want to reuse dependencies, remove the root specs
+        if self.reuse == "dependencies":


(Comment, not request): If we add more reuse modes, it is worth turning these literal strings into enums.

lib/spack/spack/test/cmd/common/arguments.py

alalazo · 2023-03-06T15:29:59Z

lib/spack/spack/cmd/common/arguments.py

+    subgroup.add_argument(
+        "--reuse-deps",
+        action=ConfigSetAction,
+        dest="concretizer:reuse",
+        const="dependencies",
+        default=None,
+        help="reuse installed dependencies only",


(nitpick) Not sure this is the best way to deal with the command line, but I think this should not be a feature blocker. What I wonder is if:

--reuse=<argument>

is better and more scalable in general, and if we can deprecate the current --fresh argument. @tgamblin @becker33 @haampie Any opinion on this?

Not to say I told you so, but... #28468 (comment)

I haven't touched this one since @adamjstewart mentioned on Slack that supporting both --reuse and --reuse=foo doesn't seem to be possible.

This adds a new mode for `concretizer:reuse` called `dependencies`, which only reuses dependencies. Currently, `spack install foo` will reuse older versions of `foo`, which might be surprising to users.

adamjstewart · 2023-03-13T21:39:40Z

Is there any special handling (in this PR or already in Spack) for handling deprecated versions? Like if one of my packages becomes deprecated, I might not want it to be reused even if it's already installed.

alecbcs · 2023-03-13T21:41:53Z

I don't know of any existing deprecated logic but I too would love this!

adamjstewart · 2023-03-13T21:44:12Z

Either way, it shouldn't hold up this PR since we already reuse everything by default. If there isn't logic to avoid deprecated versions yet, we can add it in a follow-up PR.

alecbcs · 2023-03-13T21:47:31Z

At the moment are there any blockers for this PR? It looks like perhaps we're waiting on final say about --fresh vs --reuse= options? Or is that just a conversation starter for a follow up?

alalazo · 2023-03-14T08:22:12Z

At the moment are there any blockers for this PR? It looks like perhaps we're waiting on final say about --fresh vs --reuse= options? Or is that just a conversation starter for a follow up?

I was waiting for that, but since I got no objections to the current state of the PR nor other comments I'll merge this. If objections come up later we can take care of them in a following PR.

This adds a new mode for `concretizer:reuse` called `dependencies`, which only reuses dependencies. Currently, `spack install foo` will reuse older versions of `foo`, which might be surprising to users.

spackbot-app bot added defaults tests General test capability(ies) labels Jun 4, 2022

michaelkuhn force-pushed the concretizer-reuse-deps branch 2 times, most recently from a0b9ec1 to cb033f8 Compare June 4, 2022 11:36

alalazo self-requested a review June 4, 2022 12:19

alalazo self-assigned this Jun 4, 2022

alalazo added the concretization label Jun 4, 2022

michaelkuhn force-pushed the concretizer-reuse-deps branch from cb033f8 to a514b38 Compare October 16, 2022 19:29

spackbot-app bot added the core PR affects Spack core functionality label Oct 16, 2022

michaelkuhn force-pushed the concretizer-reuse-deps branch from a514b38 to 559e141 Compare October 16, 2022 19:33

michaelkuhn force-pushed the concretizer-reuse-deps branch from 559e141 to e9dbee9 Compare October 21, 2022 05:42

tgamblin added this to the v0.20.0 milestone Nov 7, 2022

michaelkuhn force-pushed the concretizer-reuse-deps branch from e9dbee9 to 5aa6767 Compare November 17, 2022 07:56

adamjstewart requested a review from becker33 November 18, 2022 02:15

adamjstewart previously approved these changes Nov 18, 2022

View reviewed changes

tgamblin self-requested a review December 2, 2022 23:09

alalazo reviewed Dec 12, 2022

View reviewed changes

michaelkuhn dismissed adamjstewart’s stale review via ffe2d9d December 12, 2022 21:58

michaelkuhn force-pushed the concretizer-reuse-deps branch 3 times, most recently from 130768b to 02e7a56 Compare December 13, 2022 08:07

alalazo previously approved these changes Mar 6, 2023

View reviewed changes

michaelkuhn dismissed alalazo’s stale review via c077f63 March 8, 2023 20:39

michaelkuhn force-pushed the concretizer-reuse-deps branch from 02e7a56 to c077f63 Compare March 8, 2023 20:39

concretizer: add mode to reuse dependencies only

ac633d3

This adds a new mode for `concretizer:reuse` called `dependencies`, which only reuses dependencies. Currently, `spack install foo` will reuse older versions of `foo`, which might be surprising to users.

michaelkuhn force-pushed the concretizer-reuse-deps branch from c077f63 to ac633d3 Compare March 8, 2023 20:47

alalazo approved these changes Mar 12, 2023

View reviewed changes

alalazo merged commit 5bae742 into spack:develop Mar 14, 2023

Conversation

michaelkuhn commented Jun 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelkuhn commented Oct 16, 2022

Uh oh!

haampie commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Nov 17, 2022

Uh oh!

michaelkuhn commented Nov 17, 2022

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

michaelkuhn commented Dec 11, 2022

Uh oh!

alalazo Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

michaelkuhn Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelkuhn Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Feb 8, 2023

Uh oh!

michaelkuhn commented Feb 8, 2023

Uh oh!

michaelkuhn commented Feb 27, 2023

Uh oh!

alalazo commented Mar 1, 2023

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alalazo Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

adamjstewart Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

michaelkuhn Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Mar 13, 2023

Uh oh!

alecbcs commented Mar 13, 2023

Uh oh!

adamjstewart commented Mar 13, 2023

Uh oh!

alecbcs commented Mar 13, 2023

Uh oh!

alalazo commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

michaelkuhn commented Jun 4, 2022 •

edited

Loading

haampie commented Oct 21, 2022 •

edited

Loading

alalazo Dec 12, 2022 •

edited

Loading

michaelkuhn Dec 12, 2022 •

edited

Loading