From 5d7901b31252e5828eaa99babb827b5dba4eea8e Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Wed, 23 Aug 2017 23:20:40 +0200 Subject: Improve Spec literals, add Spec.from_dict() (#5151) * Simplified Spec.__init__ signature by removing the *dep_like argument. The `*dep_like` argument of `Spec.__init__` is used only for tests. This PR removes it from the call signature and introduces an equivalent fixture to be used in tests. * Refactored ``spec_from_dict`` to be a static method of ``Spec`` The fixture ``spec_from_dict`` has been refactored to be a static method of ``Spec``. Test code has been updated accordingly. Added tests for exceptional paths. * Renamed argument `unique` to `normal` + added LazySpecCache class As requested in the review the argument `unique` of `Spec.from_literal` has been renamed to `normal`. To avoid eager evaluations of `Spec(spec_like)` expressions a subclass of `collections.defaultdict` has been introduced. * Spec object can be keys of the dictionary for a spec literal. Added back the possibility use a spec directly as a key. This permits to build DAGs that are partially normalized. --- lib/spack/spack/spec.py | 183 +++++++++++++++++++++--- lib/spack/spack/test/concretize.py | 107 +++++++++----- lib/spack/spack/test/optional_deps.py | 119 ++++++++------- lib/spack/spack/test/spec_dag.py | 262 ++++++++++++++++++++++------------ 4 files changed, 472 insertions(+), 199 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index a58e1ceb2f..aa22978217 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -971,7 +971,159 @@ class SpecBuildInterface(ObjectWrapper): @key_ordering class Spec(object): - def __init__(self, spec_like, *dep_like, **kwargs): + @staticmethod + def from_literal(spec_dict, normal=True): + """Builds a Spec from a dictionary containing the spec literal. + + The dictionary must have a single top level key, representing the root, + and as many secondary level keys as needed in the spec. + + The keys can be either a string or a Spec or a tuple containing the + Spec and the dependency types. + + Args: + spec_dict (dict): the dictionary containing the spec literal + normal (bool): if True the same key appearing at different levels + of the ``spec_dict`` will map to the same object in memory. + + Examples: + A simple spec ``foo`` with no dependencies: + + .. code-block:: python + + {'foo': None} + + A spec ``foo`` with a ``(build, link)`` dependency ``bar``: + + .. code-block:: python + + {'foo': + {'bar:build,link': None}} + + A spec with a diamond dependency and various build types: + + .. code-block:: python + + {'dt-diamond': { + 'dt-diamond-left:build,link': { + 'dt-diamond-bottom:build': None + }, + 'dt-diamond-right:build,link': { + 'dt-diamond-bottom:build,link,run': None + } + }} + + The same spec with a double copy of ``dt-diamond-bottom`` and + no diamond structure: + + .. code-block:: python + + {'dt-diamond': { + 'dt-diamond-left:build,link': { + 'dt-diamond-bottom:build': None + }, + 'dt-diamond-right:build,link': { + 'dt-diamond-bottom:build,link,run': None + } + }, normal=False} + + Constructing a spec using a Spec object as key: + + .. code-block:: python + + mpich = Spec('mpich') + libelf = Spec('libelf@1.8.11') + expected_normalized = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'dyninst': { + 'libdwarf': {libelf: None}, + libelf: None + }, + mpich: None + }, + mpich: None + }, + }) + + """ + + # Maps a literal to a Spec, to be sure we are reusing the same object + spec_cache = LazySpecCache() + + def spec_builder(d): + # The invariant is that the top level dictionary must have + # only one key + assert len(d) == 1 + + # Construct the top-level spec + spec_like, dep_like = next(iter(d.items())) + + # If the requirements was for unique nodes (default) + # then re-use keys from the local cache. Otherwise build + # a new node every time. + if not isinstance(spec_like, Spec): + spec = spec_cache[spec_like] if normal else Spec(spec_like) + else: + spec = spec_like + + if dep_like is None: + return spec + + def name_and_dependency_types(s): + """Given a key in the dictionary containing the literal, + extracts the name of the spec and its dependency types. + + Args: + s (str): key in the dictionary containing the literal + + """ + t = s.split(':') + + if len(t) > 2: + msg = 'more than one ":" separator in key "{0}"' + raise KeyError(msg.format(s)) + + n = t[0] + if len(t) == 2: + dtypes = tuple(dt.strip() for dt in t[1].split(',')) + else: + dtypes = () + + return n, dtypes + + def spec_and_dependency_types(s): + """Given a non-string key in the literal, extracts the spec + and its dependency types. + + Args: + s (spec or tuple): either a Spec object or a tuple + composed of a Spec object and a string with the + dependency types + + """ + if isinstance(s, Spec): + return s, () + + spec_obj, dtypes = s + return spec_obj, tuple(dt.strip() for dt in dtypes.split(',')) + + # Recurse on dependencies + for s, s_dependencies in dep_like.items(): + + if isinstance(s, string_types): + dag_node, dependency_types = name_and_dependency_types(s) + else: + dag_node, dependency_types = spec_and_dependency_types(s) + + dependency_spec = spec_builder({dag_node: s_dependencies}) + spec._add_dependency(dependency_spec, dependency_types) + + return spec + + return spec_builder(spec_dict) + + def __init__(self, spec_like, **kwargs): # Copy if spec_like is a Spec. if isinstance(spec_like, Spec): self._dup(spec_like) @@ -1014,22 +1166,6 @@ class Spec(object): self.external_path = kwargs.get('external_path', None) self.external_module = kwargs.get('external_module', None) - # This allows users to construct a spec DAG with literals. - # Note that given two specs a and b, Spec(a) copies a, but - # Spec(a, b) will copy a but just add b as a dep. - deptypes = () - for dep in dep_like: - - if isinstance(dep, (list, tuple)): - # Literals can be deptypes -- if there are tuples in the - # list, they will be used as deptypes for the following Spec. - deptypes = tuple(dep) - continue - - spec = dep if isinstance(dep, Spec) else Spec(dep) - self._add_dependency(spec, deptypes) - deptypes = () - @property def external(self): return bool(self.external_path) or bool(self.external_module) @@ -2944,6 +3080,19 @@ class Spec(object): return str(self) +class LazySpecCache(collections.defaultdict): + """Cache for Specs that uses a spec_like as key, and computes lazily + the corresponding value ``Spec(spec_like``. + """ + def __init__(self): + super(LazySpecCache, self).__init__(Spec) + + def __missing__(self, key): + value = self.default_factory(key) + self[key] = value + return value + + # # These are possible token types in the spec grammar. # diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index 414b0fab84..572436a4b2 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -330,59 +330,94 @@ class TestConcretize(object): def test_find_spec_parents(self): """Tests the spec finding logic used by concretization. """ - s = Spec('a +foo', - Spec('b +foo', - Spec('c'), - Spec('d +foo')), - Spec('e +foo')) + s = Spec.from_literal({ + 'a +foo': { + 'b +foo': { + 'c': None, + 'd+foo': None + }, + 'e +foo': None + } + }) assert 'a' == find_spec(s['b'], lambda s: '+foo' in s).name def test_find_spec_children(self): - s = Spec('a', - Spec('b +foo', - Spec('c'), - Spec('d +foo')), - Spec('e +foo')) + s = Spec.from_literal({ + 'a': { + 'b +foo': { + 'c': None, + 'd+foo': None + }, + 'e +foo': None + } + }) + assert 'd' == find_spec(s['b'], lambda s: '+foo' in s).name - s = Spec('a', - Spec('b +foo', - Spec('c +foo'), - Spec('d')), - Spec('e +foo')) + + s = Spec.from_literal({ + 'a': { + 'b +foo': { + 'c+foo': None, + 'd': None + }, + 'e +foo': None + } + }) + assert 'c' == find_spec(s['b'], lambda s: '+foo' in s).name def test_find_spec_sibling(self): - s = Spec('a', - Spec('b +foo', - Spec('c'), - Spec('d')), - Spec('e +foo')) + + s = Spec.from_literal({ + 'a': { + 'b +foo': { + 'c': None, + 'd': None + }, + 'e +foo': None + } + }) + assert 'e' == find_spec(s['b'], lambda s: '+foo' in s).name assert 'b' == find_spec(s['e'], lambda s: '+foo' in s).name - s = Spec('a', - Spec('b +foo', - Spec('c'), - Spec('d')), - Spec('e', - Spec('f +foo'))) + s = Spec.from_literal({ + 'a': { + 'b +foo': { + 'c': None, + 'd': None + }, + 'e': { + 'f +foo': None + } + } + }) + assert 'f' == find_spec(s['b'], lambda s: '+foo' in s).name def test_find_spec_self(self): - s = Spec('a', - Spec('b +foo', - Spec('c'), - Spec('d')), - Spec('e')) + s = Spec.from_literal({ + 'a': { + 'b +foo': { + 'c': None, + 'd': None + }, + 'e': None + } + }) assert 'b' == find_spec(s['b'], lambda s: '+foo' in s).name def test_find_spec_none(self): - s = Spec('a', - Spec('b', - Spec('c'), - Spec('d')), - Spec('e')) + s = Spec.from_literal({ + 'a': { + 'b': { + 'c': None, + 'd': None + }, + 'e': None + } + }) assert find_spec(s['b'], lambda s: '+foo' in s) is None def test_compiler_child(self): diff --git a/lib/spack/spack/test/optional_deps.py b/lib/spack/spack/test/optional_deps.py index db2267f047..0023fce52d 100644 --- a/lib/spack/spack/test/optional_deps.py +++ b/lib/spack/spack/test/optional_deps.py @@ -29,65 +29,82 @@ from spack.spec import Spec @pytest.fixture( params=[ # Normalize simple conditionals - ('optional-dep-test', Spec('optional-dep-test')), - ('optional-dep-test~a', Spec('optional-dep-test~a')), - ('optional-dep-test+a', Spec('optional-dep-test+a', Spec('a'))), - ('optional-dep-test a=true', Spec( - 'optional-dep-test a=true', Spec('a') - )), - ('optional-dep-test a=true', Spec('optional-dep-test+a', Spec('a'))), - ('optional-dep-test@1.1', Spec('optional-dep-test@1.1', Spec('b'))), - ('optional-dep-test%intel', Spec( - 'optional-dep-test%intel', Spec('c') - )), - ('optional-dep-test%intel@64.1', Spec( - 'optional-dep-test%intel@64.1', Spec('c'), Spec('d') - )), - ('optional-dep-test%intel@64.1.2', Spec( - 'optional-dep-test%intel@64.1.2', Spec('c'), Spec('d') - )), - ('optional-dep-test%clang@35', Spec( - 'optional-dep-test%clang@35', Spec('e') - )), + ('optional-dep-test', {'optional-dep-test': None}), + ('optional-dep-test~a', {'optional-dep-test~a': None}), + ('optional-dep-test+a', {'optional-dep-test+a': {'a': None}}), + ('optional-dep-test a=true', { + 'optional-dep-test a=true': { + 'a': None + }}), + ('optional-dep-test a=true', { + 'optional-dep-test+a': { + 'a': None + }}), + ('optional-dep-test@1.1', {'optional-dep-test@1.1': {'b': None}}), + ('optional-dep-test%intel', {'optional-dep-test%intel': {'c': None}}), + ('optional-dep-test%intel@64.1', { + 'optional-dep-test%intel@64.1': { + 'c': None, + 'd': None + }}), + ('optional-dep-test%intel@64.1.2', { + 'optional-dep-test%intel@64.1.2': { + 'c': None, + 'd': None + }}), + ('optional-dep-test%clang@35', { + 'optional-dep-test%clang@35': { + 'e': None + }}), # Normalize multiple conditionals - ('optional-dep-test+a@1.1', Spec( - 'optional-dep-test+a@1.1', Spec('a'), Spec('b') - )), - ('optional-dep-test+a%intel', Spec( - 'optional-dep-test+a%intel', Spec('a'), Spec('c') - )), - ('optional-dep-test@1.1%intel', Spec( - 'optional-dep-test@1.1%intel', Spec('b'), Spec('c') - )), - ('optional-dep-test@1.1%intel@64.1.2+a', Spec( - 'optional-dep-test@1.1%intel@64.1.2+a', - Spec('b'), - Spec('a'), - Spec('c'), - Spec('d') - )), - ('optional-dep-test@1.1%clang@36.5+a', Spec( - 'optional-dep-test@1.1%clang@36.5+a', - Spec('b'), - Spec('a'), - Spec('e') - )), + ('optional-dep-test+a@1.1', { + 'optional-dep-test+a@1.1': { + 'a': None, + 'b': None + }}), + ('optional-dep-test+a%intel', { + 'optional-dep-test+a%intel': { + 'a': None, + 'c': None + }}), + ('optional-dep-test@1.1%intel', { + 'optional-dep-test@1.1%intel': { + 'b': None, + 'c': None + }}), + ('optional-dep-test@1.1%intel@64.1.2+a', { + 'optional-dep-test@1.1%intel@64.1.2+a': { + 'a': None, + 'b': None, + 'c': None, + 'd': None + }}), + ('optional-dep-test@1.1%clang@36.5+a', { + 'optional-dep-test@1.1%clang@36.5+a': { + 'b': None, + 'a': None, + 'e': None + }}), # Chained MPI - ('optional-dep-test-2+mpi', Spec( - 'optional-dep-test-2+mpi', - Spec('optional-dep-test+mpi', Spec('mpi')) - )), + ('optional-dep-test-2+mpi', { + 'optional-dep-test-2+mpi': { + 'optional-dep-test+mpi': {'mpi': None} + }}), # Each of these dependencies comes from a conditional # dependency on another. This requires iterating to evaluate # the whole chain. - ('optional-dep-test+f', Spec( - 'optional-dep-test+f', Spec('f'), Spec('g'), Spec('mpi') - )) + ('optional-dep-test+f', { + 'optional-dep-test+f': { + 'f': None, + 'g': None, + 'mpi': None + }}) ] ) def spec_and_expected(request): - """Parameters for te normalization test.""" - return request.param + """Parameters for the normalization test.""" + spec, d = request.param + return spec, Spec.from_literal(d) def test_normalize(spec_and_expected, config, builtin_mock): diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py index c82365ad11..0d04cb5b0c 100644 --- a/lib/spack/spack/test/spec_dag.py +++ b/lib/spack/spack/test/spec_dag.py @@ -197,15 +197,19 @@ class TestSpecDag(object): spec.normalize() spec.normalize() - def test_normalize_with_virtual_spec(self): - dag = Spec('mpileaks', - Spec('callpath', - Spec('dyninst', - Spec('libdwarf', - Spec('libelf')), - Spec('libelf')), - Spec('mpi')), - Spec('mpi')) + def test_normalize_with_virtual_spec(self, ): + dag = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'dyninst': { + 'libdwarf': {'libelf': None}, + 'libelf': None + }, + 'mpi': None + }, + 'mpi': None + } + }) dag.normalize() # make sure nothing with the same name occurs twice @@ -219,14 +223,18 @@ class TestSpecDag(object): assert counts[name] == 1 def test_dependents_and_dependencies_are_correct(self): - spec = Spec('mpileaks', - Spec('callpath', - Spec('dyninst', - Spec('libdwarf', - Spec('libelf')), - Spec('libelf')), - Spec('mpi')), - Spec('mpi')) + spec = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'dyninst': { + 'libdwarf': {'libelf': None}, + 'libelf': None + }, + 'mpi': None + }, + 'mpi': None + } + }) check_links(spec) spec.normalize() @@ -274,21 +282,45 @@ class TestSpecDag(object): def test_equal(self): # Different spec structures to test for equality - flat = Spec('mpileaks ^callpath ^libelf ^libdwarf') - - flat_init = Spec( - 'mpileaks', Spec('callpath'), Spec('libdwarf'), Spec('libelf')) - - flip_flat = Spec( - 'mpileaks', Spec('libelf'), Spec('libdwarf'), Spec('callpath')) - - dag = Spec('mpileaks', Spec('callpath', - Spec('libdwarf', - Spec('libelf')))) - - flip_dag = Spec('mpileaks', Spec('callpath', - Spec('libelf', - Spec('libdwarf')))) + flat = Spec.from_literal( + {'mpileaks ^callpath ^libelf ^libdwarf': None} + ) + + flat_init = Spec.from_literal({ + 'mpileaks': { + 'callpath': None, + 'libdwarf': None, + 'libelf': None + } + }) + + flip_flat = Spec.from_literal({ + 'mpileaks': { + 'libelf': None, + 'libdwarf': None, + 'callpath': None + } + }) + + dag = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'libdwarf': { + 'libelf': None + } + } + } + }) + + flip_dag = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'libelf': { + 'libdwarf': None + } + } + } + }) # All these are equal to each other with regular == specs = (flat, flat_init, flip_flat, dag, flip_dag) @@ -311,39 +343,52 @@ class TestSpecDag(object): def test_normalize_mpileaks(self): # Spec parsed in from a string - spec = Spec('mpileaks ^mpich ^callpath ^dyninst ^libelf@1.8.11' - ' ^libdwarf') + spec = Spec.from_literal({ + 'mpileaks ^mpich ^callpath ^dyninst ^libelf@1.8.11 ^libdwarf': None + }) # What that spec should look like after parsing - expected_flat = Spec( - 'mpileaks', Spec('mpich'), Spec('callpath'), Spec('dyninst'), - Spec('libelf@1.8.11'), Spec('libdwarf')) + expected_flat = Spec.from_literal({ + 'mpileaks': { + 'mpich': None, + 'callpath': None, + 'dyninst': None, + 'libelf@1.8.11': None, + 'libdwarf': None + } + }) # What it should look like after normalization mpich = Spec('mpich') libelf = Spec('libelf@1.8.11') - expected_normalized = Spec( - 'mpileaks', - Spec('callpath', - Spec('dyninst', - Spec('libdwarf', - libelf), - libelf), - mpich), - mpich) + expected_normalized = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'dyninst': { + 'libdwarf': {libelf: None}, + libelf: None + }, + mpich: None + }, + mpich: None + }, + }) # Similar to normalized spec, but now with copies of the same # libelf node. Normalization should result in a single unique # node for each package, so this is the wrong DAG. - non_unique_nodes = Spec( - 'mpileaks', - Spec('callpath', - Spec('dyninst', - Spec('libdwarf', - Spec('libelf@1.8.11')), - Spec('libelf@1.8.11')), - mpich), - Spec('mpich')) + non_unique_nodes = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'dyninst': { + 'libdwarf': {'libelf@1.8.11': None}, + 'libelf@1.8.11': None + }, + mpich: None + }, + mpich: None + } + }, normal=False) # All specs here should be equal under regular equality specs = (spec, expected_flat, expected_normalized, non_unique_nodes) @@ -380,14 +425,18 @@ class TestSpecDag(object): spec = Spec('mpileaks ^mpi ^libelf@1.8.11 ^libdwarf') spec.normalize() - expected_normalized = Spec( - 'mpileaks', - Spec('callpath', - Spec('dyninst', - Spec('libdwarf', - Spec('libelf@1.8.11')), - Spec('libelf@1.8.11')), - Spec('mpi')), Spec('mpi')) + expected_normalized = Spec.from_literal({ + 'mpileaks': { + 'callpath': { + 'dyninst': { + 'libdwarf': {'libelf@1.8.11': None}, + 'libelf@1.8.11': None + }, + 'mpi': None + }, + 'mpi': None + } + }) assert str(spec) == str(expected_normalized) @@ -552,16 +601,18 @@ class TestSpecDag(object): def test_traversal_directions(self): """Make sure child and parent traversals of specs work.""" - # We'll use d for a diamond dependency - d = Spec('d') - - # Mock spec. - spec = Spec('a', - Spec('b', - Spec('c', d), - Spec('e')), - Spec('f', - Spec('g', d))) + # Mock spec - d is used for a diamond dependency + spec = Spec.from_literal({ + 'a': { + 'b': { + 'c': {'d': None}, + 'e': None + }, + 'f': { + 'g': {'d': None} + } + } + }) assert ( ['a', 'b', 'c', 'd', 'e', 'f', 'g'] == @@ -577,16 +628,18 @@ class TestSpecDag(object): def test_edge_traversals(self): """Make sure child and parent traversals of specs work.""" - # We'll use d for a diamond dependency - d = Spec('d') - - # Mock spec. - spec = Spec('a', - Spec('b', - Spec('c', d), - Spec('e')), - Spec('f', - Spec('g', d))) + # Mock spec - d is used for a diamond dependency + spec = Spec.from_literal({ + 'a': { + 'b': { + 'c': {'d': None}, + 'e': None + }, + 'f': { + 'g': {'d': None} + } + } + }) assert ( ['a', 'b', 'c', 'd', 'e', 'f', 'g'] == @@ -610,12 +663,14 @@ class TestSpecDag(object): def test_construct_spec_with_deptypes(self): """Ensure that it is possible to construct a spec with explicit dependency types.""" - s = Spec('a', - Spec('b', - ['build'], Spec('c')), - Spec('d', - ['build', 'link'], Spec('e', - ['run'], Spec('f')))) + s = Spec.from_literal({ + 'a': { + 'b': {'c:build': None}, + 'd': { + 'e:build,link': {'f:run': None} + } + } + }) assert s['b']._dependencies['c'].deptypes == ('build',) assert s['d']._dependencies['e'].deptypes == ('build', 'link') @@ -653,12 +708,19 @@ class TestSpecDag(object): 'dt-diamond-bottom'].deptypes == ('build', 'link', 'run') def check_diamond_normalized_dag(self, spec): - bottom = Spec('dt-diamond-bottom') - dag = Spec('dt-diamond', - ['build', 'link'], Spec('dt-diamond-left', - ['build'], bottom), - ['build', 'link'], Spec('dt-diamond-right', - ['build', 'link', 'run'], bottom)) + + dag = Spec.from_literal({ + 'dt-diamond': { + 'dt-diamond-left:build,link': { + 'dt-diamond-bottom:build': None + }, + 'dt-diamond-right:build,link': { + 'dt-diamond-bottom:build,link,run': None + }, + + } + }) + assert spec.eq_dag(dag) def test_normalize_diamond_deptypes(self): @@ -788,3 +850,13 @@ class TestSpecDag(object): canonical_deptype(('foo', 'bar')) with pytest.raises(ValueError): canonical_deptype(('foo',)) + + def test_invalid_literal_spec(self): + + # Can't give type 'build' to a top-level spec + with pytest.raises(spack.spec.SpecParseError): + Spec.from_literal({'foo:build': None}) + + # Can't use more than one ':' separator + with pytest.raises(KeyError): + Spec.from_literal({'foo': {'bar:build:link': None}}) -- cgit v1.2.3-60-g2f50