Allow different instances of build deps in environment by scheibelp · Pull Request #10792 · spack/spack

scheibelp · 2019-03-05T03:14:42Z

Environments currently stores the full spec dag (including build dependencies) but indexes by dag hash, which excludes build dependencies. This can lead to reference errors for example given:

X-[L]->Y
X-[B]->Z1
Y-[B]->Z2

This PR

Updates specs to track dag hashes both with and without build dependencies
Updates the lockfile format to index based on the full dag hash (including build deps)
Automatically updates older lockfiles to newer ones (where the indices are hashes based on the full dag)

becker33

I would consider changing the all_deps booleans in to_node_dict and dag_hash to be named include_build_deps for clarity.

Similarly, I would consider changing _build_hash and _hash to something that's more descriptive of their differences. I think it might be hard for others to pick up this code, but I'm not sure what a better name for these two is so I'm not requesting changes since I don't have a good answer.

Overall LGTM.

becker33

@scheibelp Does this mean we can get rid of the yaml round-trip at line 746 in environment.py?

tgamblin

@scheibelp @becker33: this looks mostly awesome -- I definitely think environments should be keyed by full hash. But this duplicates some work by @scottwittenburg on the release pipeline.

Requests:

There is already a full_hash() function on Spec, and we should consolidate build_hash into that.
I have a clarifying question about whether the lockfile already includes the build dependencies -- see the comments.
After reading the whole thing, I think it would probably be good to make the deptype arguments (deptypes, all_deps) consistent and use the convention that we use when we have deptypes as a keyword argument. I think it would improve the PR.

tgamblin · 2019-07-11T16:51:20Z

lib/spack/spack/environment.py


+                if read_lock_version == 1:
+                    shutil.copy(self.lock_path, self._lock_backup_v1_path)
+                    tty.debug("Storing backup of old lockfile format")


Add the path to the message here.

tgamblin · 2019-07-11T17:07:53Z

lib/spack/spack/environment.py

+        self.specs_by_hash = {}
+        for _, spec in specs_by_hash.items():
+            old_hash = spec.dag_hash()
+            full_hash = spec.dag_hash(all_deps=True)


There is already a full_hash() method on Spec. Can we use this here? It's used for the release pipeline work by Kitware -- I think it's more consistent to just use the method since it's there, but it may not actually be what you want.

I'm looking at the full_hash() implementation and it looks like it mostly copies the dag_hash() code -- can it just be implemented as spec.dag_hash(all_deps=True)?

In this case full_hash is just the dag hash computed with all dependencies (including build deps). Spec.full_hash includes Package.content_hash, which is more-specific than we have wanted so far and would be going a bit beyond the original fix.

Given that, I will rename this variable in this context to build_hash to avoid confusion.

@scheibelp: ok thanks -- that is very clear and makes sense. Can we:

Have 3 methods: dag_hash(), full_hash(), and build_hash(), with comments making them clear, and

Implement full_hash and build_hash() in terms of dag_hash()? I think it would be clearer if dag_hash() looked something like this:
def dag_hash(self, length=none, deptypes=default_deptype, include_package=False): deptypes = canonical_deptype(deptypes) ...

Then implement full_hash and build_hash in terms of that. Does that seem clearer?

lib/spack/spack/environment.py

tgamblin · 2019-07-11T19:10:31Z

lib/spack/spack/spec.py

        if deps:
            if hash_function is None:
-                hash_function = lambda s: s.dag_hash()
+                hash_function = lambda s: s.dag_hash(all_deps=all_deps)


I found this hard to read because all_deps is not nearly as clear as the deptype argument that other functions have. I think it would be clearer that this is NOT the default case if we refactored to use deptype=....

tgamblin · 2019-07-11T19:11:25Z

lib/spack/spack/spec.py

            node = s.to_node_dict(all_deps=all_deps)
            node[s.name]['hash'] = s.dag_hash()
+            if all_deps:
+                node[s.name]['build_hash'] = s.dag_hash(all_deps=True)


I think this would be clearer with a deptype argument too.

lib/spack/spack/spec.py

scheibelp · 2019-07-12T20:18:36Z

@becker33 Regarding #10792 (review):

Does this mean we can get rid of the yaml round-trip at line 746 in environment.py?

No: views still don't store build deps

scheibelp · 2019-07-18T21:30:31Z

This could likely be compressed into two commits: everything up to and including a9a9466, and then everything after (refactoring the hash calculations).

tgamblin · 2019-07-20T01:22:12Z

@becker33: this is ready for re-review. There is at least one failing test, but I think it has to do with the rebase I did on top of develop, which now has stacks.

I think that, barring any changes you want to the code, I can hand this off to you now.

There are a few things I noticed here while working on this PR and the old develop branch -- it's very easy to create an environment that doesn't validate with an old Spack instance. We should think about how we want to handle that. It would probably help if we fixed the issue where envs create empty default values for all keys. I think we should think about that fairly soon in a follow-on PR.

tgamblin · 2019-07-20T01:24:47Z

@becker33: last point here. When merging this, we should probably squash everything before
35befe8 (experimental refactoring of hash functions) into one commit authored by me and @scheibelp, and everything before it into one commit authored by @scheibelp.

tgamblin · 2019-07-19T04:02:11Z

lib/spack/spack/spec.py


        # TODO: restore build dependencies here once we have less picky
        # TODO: concretization.
-        if all_deps:


The comment above here should probably be removed now.

tgamblin · 2019-07-20T07:24:29Z

@becker33: make sure my bugfix commit here is correct. I believe it fixes an issue in stacks.

- ensure that Spec._build_hash attr is defined - add logic to compute _build_hash when it is not already computed (e.g. for specs prior to this PR) - add test to ensure that different instance of a build dep are preserved - test conversion of old env lockfile format to new format - tests: avoid view creation, DAG display in tests using MockPackage - add regression test for more-general bug also fixed by this PR - update lockfile version since the way we are maintaining hashes has changed - write out backup for version-1 lockfiles and test that

Spack has evolved to have three types of hash functions, and it's becoming hard to tell when each one is called. Whlie we aren't yet ready to get rid of them, we can refactor them so that the code is clearer and easier to track. - Add a `hash_types` module with concise descriptors for hashes. - Consolidate hashing logic in a private `Spec._spec_hash()` function. - `dag_hash()`, `build_hash()`, and `full_hash()` all call `_spec_hash()` - `to_node_dict()`, `to_dict()`, `to_yaml()` and `to_json()` now take a `hash` parameter consistent with the one that `_spec_hash()` requires. Co-authored-by: Todd Gamblin <[email protected]>

- Setting specs from lockfiles was not correctly stringifying concretized user specs. - Fix `_set_user_specs_from_lockfile` - Add some validation code to `SpecList` constructor

This fixes a regression introduced in #10792. `spack uninstall` in an environment would not match concrete query specs properly after the index hash of enviroments changed. - [x] Search by DAG hash for specs to remove instead of by build hash

becker33 approved these changes Mar 6, 2019

View reviewed changes

scheibelp mentioned this pull request Mar 7, 2019

Remove libtool version dependency #10810

Closed

adamjstewart added concretization environments labels Mar 13, 2019

tgamblin self-requested a review April 3, 2019 23:02

scheibelp added the WIP label Jul 9, 2019

becker33 approved these changes Jul 9, 2019

View reviewed changes

becker33 approved these changes Jul 11, 2019

View reviewed changes

tgamblin requested changes Jul 11, 2019

View reviewed changes

tgamblin added the impact-high label Jul 18, 2019

tgamblin self-assigned this Jul 18, 2019

tgamblin requested a review from becker33 July 20, 2019 00:36

tgamblin assigned scheibelp and unassigned scheibelp Jul 20, 2019

tgamblin force-pushed the bugfix/build-deps-in-envs branch from 007201b to 30ca77c Compare July 20, 2019 01:05

tgamblin assigned becker33 and unassigned tgamblin Jul 20, 2019

tgamblin approved these changes Jul 20, 2019

View reviewed changes

tgamblin force-pushed the bugfix/build-deps-in-envs branch from 30ca77c to 672b600 Compare July 20, 2019 07:23

tgamblin force-pushed the bugfix/build-deps-in-envs branch from 672b600 to a6becb2 Compare July 20, 2019 16:50

becker33 force-pushed the bugfix/build-deps-in-envs branch from a6becb2 to 0843b13 Compare July 22, 2019 14:27

scheibelp and others added 3 commits July 22, 2019 12:02

bugfix: env.write() should stringify Spec lists.

0bb00e3

- Setting specs from lockfiles was not correctly stringifying concretized user specs. - Fix `_set_user_specs_from_lockfile` - Add some validation code to `SpecList` constructor

becker33 force-pushed the bugfix/build-deps-in-envs branch from 0843b13 to 0bb00e3 Compare July 22, 2019 17:03

becker33 merged commit cc4094b into spack:develop Jul 22, 2019

glennpj mentioned this pull request Jul 23, 2019

spack commands hanging #12091

Closed

tgamblin mentioned this pull request Nov 5, 2019

bugfix: uninstall should find concrete specs by DAG hash #13598

Merged

1 task

Conversation

scheibelp commented Mar 5, 2019

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

becker33 left a comment • 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!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

tgamblin Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

scheibelp Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

tgamblin Jul 12, 2019 • 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!

Uh oh!

Uh oh!

Uh oh!

tgamblin Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

tgamblin Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scheibelp commented Jul 12, 2019

Uh oh!

scheibelp commented Jul 18, 2019

Uh oh!

tgamblin commented Jul 20, 2019

Uh oh!

tgamblin commented Jul 20, 2019

Uh oh!

tgamblin Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Jul 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

becker33 left a comment •

edited

Loading

tgamblin Jul 12, 2019 •

edited

Loading