Introduce a class to represent a "closed set of concrete specs" by alalazo · Pull Request #40636 · spack/spack

alalazo · 2023-10-20T15:35:41Z

There are many places in Spack where we need to operate with a set of consistent spec objects, i.e. a set of specs where:

A single DAG hash refers to a single object in memory
Each spec has no references to other specs outside the set

This is, for instance, the case when we need to reconstruct a concrete spec during concretization, in particular if that spec is provided by more than one source of "reusable" specs (see #39590).

Another case is when we concretize or reconstruct environments. In those cases we have custom ad-hoc code to ensure consistency in memory. For concretization that would be:

spack/lib/spack/spack/environment/environment.py

Lines 1601 to 1624 in 2f24aeb

    
           # Unify the specs objects, so we get correct references to all parents 
        
           self._read_lockfile_dict(self._to_lockfile_dict()) 
        
           # Re-attach information on test dependencies 
        
           if tests: 
        
               # This is slow, but the information on test dependency is lost 
        
               # after unification or when reading from a lockfile. 
        
               for h in self.specs_by_hash: 
        
                   current_spec, computed_spec = self.specs_by_hash[h], by_hash[h] 
        
                   for node in computed_spec.traverse(): 
        
                       test_edges = node.edges_to_dependencies(depflag=dt.TEST) 
        
                       for current_edge in test_edges: 
        
                           test_dependency = current_edge.spec 
        
                           if test_dependency in current_spec[node.name]: 
        
                               continue 
        
                           current_spec[node.name].add_dependency_edge( 
        
                               test_dependency.copy(), depflag=dt.TEST, virtuals=current_edge.virtuals 
        
                           ) 
        
           results = [ 
        
               (abstract, self.specs_by_hash[h]) 
        
               for abstract, h in zip(self.concretized_user_specs, self.concretized_order) 
        
           ] 
        
           return results

while when reading an environment from a lockfile:

spack/lib/spack/spack/environment/environment.py

Lines 2239 to 2257 in 2f24aeb

    
           # Traverse the root specs one at a time in the order they appear. 
        
           # The first time we see each DAG hash, that's the one we want to 
        
           # keep.  This is only required as long as we support older lockfile 
        
           # formats where the mapping from DAG hash to lockfile key is possibly 
        
           # one-to-many. 
        
           for lockfile_key in self.concretized_order: 
        
               for s in specs_by_hash[lockfile_key].traverse(): 
        
                   if s.dag_hash() not in first_seen: 
        
                       first_seen[s.dag_hash()] = s 
        
           # Now make sure concretized_order and our internal specs dict 
        
           # contains the keys used by modern spack (i.e. the dag_hash 
        
           # that includes build deps and package hash). 
        
           self.concretized_order = [ 
        
               specs_by_hash[h_key].dag_hash() for h_key in self.concretized_order 
        
           ] 
        
           for spec_dag_hash in self.concretized_order: 
        
               self.specs_by_hash[spec_dag_hash] = first_seen[spec_dag_hash]

This PR is an attempt at collecting these similar operations together into a class, which can be reused in different places within the codebase. This class is a mapping from hashes to concrete specs that:

Ensures consistency of the specs being managed
Provides methods to add, delete, query specs

The idea is to refine an API for the class in this PR, and use it for Environments, and apply it in later PRs also to Databases and Indexes.

lib/spack/spack/environment/environment.py

This method receives a list of concrete specs to keep, and removes everything else.

haampie · 2024-01-31T14:32:48Z

I'm not immediately a fan of the fact that this always duplicates.

It's redundant in many cases (and then it's just a performance penalty)
I don't know if we store any properties on Spec object like we do on PackageBase instances, like installed_from_binary_cache, that would get lost after _dup.

If you wanna go this way in environments, you should use ConcreteSpecsByHash as a spec builder, instead of first building the Spec objects graph, only to tear them apart and reconstruct them in ConcreteSpecsByHash again.

The overhead of deserializing an environment is really felt in CI with e.g. the e4s env, any command spack -e <env> ... is ultra slow already.

spackbot-app bot added core PR affects Spack core functionality environments labels Oct 20, 2023

haampie reviewed Oct 21, 2023

View reviewed changes

lib/spack/spack/environment/environment.py Outdated Show resolved Hide resolved

alalazo marked this pull request as draft October 30, 2023 11:20

alalazo changed the title ~~Use ConcreteSpecsByHash to unify environments~~ Use ConcreteSpecsByHash in environments Nov 2, 2023

alalazo force-pushed the maintainers/environment-simplify-unification branch from 6796aef to 74c35d0 Compare November 2, 2023 23:09

spackbot-app bot added commands stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) labels Nov 2, 2023

alalazo commented Nov 2, 2023

View reviewed changes

lib/spack/spack/environment/environment.py Show resolved Hide resolved

alalazo force-pushed the maintainers/environment-simplify-unification branch from 60aa55c to 8016344 Compare November 15, 2023 15:38

alalazo changed the title ~~Use ConcreteSpecsByHash in environments~~ Introduce a class to represent a "closed set of concrete specs" Nov 21, 2023

alalazo marked this pull request as ready for review November 21, 2023 09:13

alalazo added the refactoring label Nov 24, 2023

alalazo force-pushed the maintainers/environment-simplify-unification branch from 8016344 to 4b4ea7d Compare January 5, 2024 16:34

alalazo mentioned this pull request Jan 8, 2024

bugfix: original concretizer is sensitive to dependency order #41975

Merged

5 tasks

alalazo added 14 commits January 30, 2024 13:13

Use ConcreteSpecsByHash to unify environments

dd1db29

Add query and delete operations to ConcreteSpecsByHash

c1cc07c

Extract a method to initialize spec related state in environments

9b34b59

Introduce Environment.all_specs_by_hash

d34516c

environment: rename "specs_by_hash" to "root_specs_by_hash"

a639d84

environment: reuse code to build the env from lockfile

c833b85

environment: fix deconcretize method

4b73b00

environment: remove dead code

0e3333a

environment: simplify all_hashes

7823709

environment: remove a traverse in get_by_hash

c2d5768

environment: remove a traverse in matching_spec

7126d0e

environment: remove unneeded local variable

2e4cafa

Add docstrings for query and delete methods

038dd39

Move class to its own Python module

367102e

ConcreteSpecsByHash: add a garbage_collection method

f5687db

This method receives a list of concrete specs to keep, and removes everything else.

alalazo force-pushed the maintainers/environment-simplify-unification branch from 4b4ea7d to f5687db Compare January 30, 2024 12:23

alalazo closed this Mar 2, 2026

alalazo deleted the maintainers/environment-simplify-unification branch March 2, 2026 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a class to represent a "closed set of concrete specs"#40636

Introduce a class to represent a "closed set of concrete specs"#40636
alalazo wants to merge 15 commits intospack:developfrom
alalazo:maintainers/environment-simplify-unification

alalazo commented Oct 20, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

haampie commented Jan 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	# Unify the specs objects, so we get correct references to all parents
	self._read_lockfile_dict(self._to_lockfile_dict())

	# Re-attach information on test dependencies
	if tests:
	# This is slow, but the information on test dependency is lost
	# after unification or when reading from a lockfile.
	for h in self.specs_by_hash:
	current_spec, computed_spec = self.specs_by_hash[h], by_hash[h]
	for node in computed_spec.traverse():
	test_edges = node.edges_to_dependencies(depflag=dt.TEST)
	for current_edge in test_edges:
	test_dependency = current_edge.spec
	if test_dependency in current_spec[node.name]:
	continue
	current_spec[node.name].add_dependency_edge(
	test_dependency.copy(), depflag=dt.TEST, virtuals=current_edge.virtuals
	)

	results = [
	(abstract, self.specs_by_hash[h])
	for abstract, h in zip(self.concretized_user_specs, self.concretized_order)
	]
	return results

	# Traverse the root specs one at a time in the order they appear.
	# The first time we see each DAG hash, that's the one we want to
	# keep. This is only required as long as we support older lockfile
	# formats where the mapping from DAG hash to lockfile key is possibly
	# one-to-many.
	for lockfile_key in self.concretized_order:
	for s in specs_by_hash[lockfile_key].traverse():
	if s.dag_hash() not in first_seen:
	first_seen[s.dag_hash()] = s

	# Now make sure concretized_order and our internal specs dict
	# contains the keys used by modern spack (i.e. the dag_hash
	# that includes build deps and package hash).
	self.concretized_order = [
	specs_by_hash[h_key].dag_hash() for h_key in self.concretized_order
	]

	for spec_dag_hash in self.concretized_order:
	self.specs_by_hash[spec_dag_hash] = first_seen[spec_dag_hash]

Conversation

alalazo commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haampie commented Jan 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alalazo commented Oct 20, 2023 •

edited

Loading