Do not try to compute hashes of concrete specs from yaml by scottwittenburg · Pull Request #25708 · spack/spack

scottwittenburg · 2021-08-31T00:59:54Z

Some old concrete spec.yaml files in binary mirrors may appear without full_hash or package_hash, and when spack encounters these (for example, when running spack buildcache update-index on a mirror with old binaries), it tries to compute those hashes, even though it already decided it should not when it set _hashes_final = True. When this happens, spack can easily discover that a patch present at the time the spec.yaml was written is no longer present in the repository, at which point it crashes with an error similar to the following:

spack.patch.NoSuchPatchError: Couldn't find patch for package builtin.mpfr with sha256: 66a5d58364113a21405fc53f4a48f4e8

This PR solves the problem by making sure spack does not attempt to re-compute hashes when the _hashes_final property is set to True. Instead, spack will just take the dag_hash and use it for any of the unknown hashes.

This PR also make sure that going forward, the spack.lock files will contain all the hashes for each spec, as otherwise if we were to reconstitute specs from the spack.lock after this change, those hashes would be lost forever. However, the lockfile is still keyed on build hash.

TODO:

remove build provenance (build_spec = None) after splicing, be sure DAG hash doesn't change
update jobs to read/write json specs instead of yaml
remove old yaml from mirror whenever you update yaml -> json

scottwittenburg · 2021-09-03T17:30:33Z

The gitlab pipeline failed because this PR changed the full hashes of specs with dependencies, which revealed that [email protected] is deprecated and can longer be built from source.

Once #25787 passes and is merged, I'll rebase this on develop and try again.

scottwittenburg · 2021-09-10T17:54:05Z

fyi @eugeneswalker

tgamblin

Some questions

tgamblin · 2021-09-15T17:14:53Z

lib/spack/spack/cmd/ci.py

    tty.debug('job concrete spec path: {0}'.format(job_spec_yaml_path))
    with open(job_spec_yaml_path, 'w') as fd:
-        fd.write(job_spec.to_yaml(hash=ht.build_hash))
+        fd.write(job_spec.to_yaml(hash=ht.full_hash))


Make this write JSON now that #22845 is in.

Yes, will do.

tgamblin · 2021-09-15T17:14:58Z

lib/spack/spack/cmd/ci.py

    root_spec_yaml_path = os.path.join(repro_dir, 'root.yaml')
    with open(root_spec_yaml_path, 'w') as fd:
-        fd.write(spec_map['root'].to_yaml(hash=ht.build_hash))
+        fd.write(spec_map['root'].to_yaml(hash=ht.full_hash))


JSON.

Do we use the contents of this file for anything right now? I am pretty sure it is just for debugging yes?

Yes, just for debugging.

tgamblin · 2021-09-15T17:16:23Z

lib/spack/spack/environment.py

-                    # Assumes no legacy formats, since this was just created.
-                    spec_dict[ht.dag_hash.name] = s.dag_hash()
+                    spec_dict = s.node_dict_with_hashes(
+                        hash=ht.full_hash, dep_hash=ht.build_hash)


why not full hash everywhere?

Yes, maybe we should. But then do we have to change the key used in the spack.lock? And does that mean a new version of the file format we have to support in perpetuity (like when the format changed from dag hash to build hash)?

tgamblin · 2021-09-15T17:19:43Z

lib/spack/spack/spec.py

+            if self._hashes_final and not getattr(self, hash.attr, None):
+                package_hash = getattr(self, ht.dag_hash.attr, None)
+            else:
+                package_hash = self.package_hash()


It's weird to me that we're assigning dag_hash to package_hash. When we discussed this, I said that we should promote dag_hash to build_hash to full_hash, mainly b/c we sometimes need to index things by those and the weaker hash is still the best identifier we have for the spec. package_hash, though, is a content has of the package.py files. We still don't actually know it -- if we can get away with just leaving it None I'd prefer to.

Do we need to set this if we can instead set full_hash and build_hash?

It's the computation of the package_hash that's causing the problems right now, let me see if I can just leave it None.

I think the only place we'd ask for it to be computed is when getting the full hash if the full hash is not set.

Actually, now I'm wondering: Let's say I have an old spec.yaml (now they're spec.json, but this one's old) for pkg a which depends on some dag hash of pkg b. Recently b was rebuilt with a new full hash, but because the dag hash of b didn't change, I need to splice the new b into a. Doesn't it mean I need to recompute the full hash of a now? Because it's the serialized spec dict that's getting hashed, and if the full hash of a dep of a changes, so must the full hash of a itself?

I guess that's where promoting the dag_hash is intended to help.

becker33 · 2021-09-22T21:29:02Z

I think the best way to move forward with this is to drop the splice provenance when pushing new specs to a mirror. I think that can be as simple as setting s.build_spec = s for each spec immediately after the splice operation.

This is lying to the user, but I think it's the least damaging lie we can tell to keep things working until the reusing concretizer works. We should definitely include a TODO comment saying to get rid of this ASAP.

scottwittenburg · 2021-09-23T16:51:07Z

I think the best way to move forward with this is to drop the splice provenance when pushing new specs to a mirror. I think that can be as simple as setting s.build_spec = s for each spec immediately after the splice operation.

Ok, I think this makes sense, thanks. I'll give it a try and just make sure that it has what I think is the desired effect, i.e. we don't get a new DAG hash on the spliced spec as a result.

This is lying to the user, but I think it's the least damaging lie we can tell to keep things working until the reusing concretizer works. We should definitely include a TODO comment saying to get rid of this ASAP.

Yes, I'll definitely do that.

scottwittenburg · 2021-09-30T00:39:41Z

I think the best way to move forward with this is to drop the splice provenance when pushing new specs to a mirror. I think that can be as simple as setting s.build_spec = s for each spec immediately after the splice operation.

@becker33 When I initially read your suggestion, my brain interpreted it as s.build_spec = None. Otherwise, wouldn't the dag hash still change? Anyway, I just tried it this way, and when we subsequently call s.to_dict(), we end up in an infinite recursion. As expected I think. So maybe either setting build_spec = None and hoping it doesn't get serialized in the json, or else popping that key from the dict is maybe what you meant?

scottwittenburg · 2021-10-01T18:29:15Z

So to follow up here, I'm going to try setting s.build_spec = None instead of s.build_spec = s, and mkae sure the DAG hash doesn't change if we do that.

I'm also going to keep the build hash as the key of the spack.lock for now, since we are starting a larger effort to move to full hash everywhere anyway.

This PR also changes the spack.lock file to include all the hashes for each spec. The reason for this is because when specs are created from the lock, they are marked as concrete with _hashes_final = True, which, after this change, prevents hashes from being computed again. Additionally, when we write out the concrete spec yaml in gitlab CI, we now include all the hashes in that file, for the same reason as above: we don't want to lose the hashes spack knew before we wrote this file.

alalazo · 2023-02-20T14:37:00Z

Am I correct that this is outdated since #28504 has been merged?

scottwittenburg · 2023-02-21T18:48:19Z

Am I correct that this is outdated

Yes, it is indeed outdated. Closing.

scottwittenburg force-pushed the hashes-final-means-final branch from 46e2e11 to 4fef6e2 Compare September 1, 2021 22:41

spackbot-app bot added commands environments tests General test capability(ies) labels Sep 1, 2021

scottwittenburg force-pushed the hashes-final-means-final branch 2 times, most recently from de74bee to 77222ac Compare September 2, 2021 20:48

scottwittenburg mentioned this pull request Sep 3, 2021

Update pinned OpenSSL version to 1.1.1l #25787

Merged

scottwittenburg requested a review from tgamblin September 3, 2021 17:43

scottwittenburg force-pushed the hashes-final-means-final branch from 77222ac to 06cfcbd Compare September 9, 2021 00:37

scottwittenburg mentioned this pull request Sep 13, 2021

Fix environment reading from lockfile to trust written hashes #25879

Merged

scottwittenburg force-pushed the hashes-final-means-final branch from 06cfcbd to 973368e Compare September 13, 2021 20:22

tgamblin requested changes Sep 15, 2021

View reviewed changes

scottwittenburg force-pushed the hashes-final-means-final branch from 973368e to f9c9a8d Compare September 22, 2021 15:49

spackbot-app bot added the binary-packages label Sep 22, 2021

scottwittenburg mentioned this pull request Sep 22, 2021

Spack fails to splice old specs with missing patches #26167

Open

3 tasks

scottwittenburg mentioned this pull request Sep 24, 2021

PIpelines: Make quality of life improvements #26241

Open

14 tasks

scottwittenburg force-pushed the hashes-final-means-final branch from f9c9a8d to 1b8dd2d Compare September 30, 2021 16:36

alalazo assigned tgamblin and becker33 Oct 4, 2021

scottwittenburg force-pushed the hashes-final-means-final branch 2 times, most recently from 0cedc7e to ef9166c Compare October 7, 2021 03:00

scottwittenburg force-pushed the hashes-final-means-final branch from ef9166c to 6883473 Compare October 7, 2021 15:49

scottwittenburg closed this Feb 21, 2023

Conversation

scottwittenburg commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scottwittenburg commented Sep 3, 2021

Uh oh!

scottwittenburg commented Sep 10, 2021

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottwittenburg Sep 15, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

becker33 commented Sep 22, 2021

Uh oh!

scottwittenburg commented Sep 23, 2021

Uh oh!

scottwittenburg commented Sep 30, 2021

Uh oh!

scottwittenburg commented Oct 1, 2021

Uh oh!

alalazo commented Feb 20, 2023

Uh oh!

scottwittenburg commented Feb 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scottwittenburg commented Aug 31, 2021 •

edited

Loading

scottwittenburg Sep 15, 2021 •

edited

Loading