Finer selection of virtual providers by alalazo · Pull Request #15569 · spack/spack

alalazo · 2020-03-19T12:18:19Z

fixes #15443
closes #17439

This PR implements what is described in #15443:

Finer selection of virtual packages, with an extension to the spec language
Unit tests to stress the new code
Documentation of the new feature
Rebase after packages.yaml: allow virtuals to specify buildable: false #14934 is merged and check there are no regressions on features
Add attributes on DAG edges to account for virtual dependencies
Handle conditionals in provides correctly
Rebase after Environments: specify packages for developer builds #15256 is merged, revisit extension to the spec language

tldahlgren · 2020-03-24T17:41:37Z

FYI. I provided feedback on #15505

tldahlgren · 2020-03-24T17:48:46Z

I'll start #15570 now.

alalazo · 2020-04-01T13:26:41Z

@becker33 @tgamblin Apparently the features added in #14934 didn't conflict with this PR. I have a question on semantics though. With this configuration:

packages:
  all:
    providers:
      lapack: [openblas]
  lapack:
    buildable: False
    paths:
      [email protected]: /usr

I obtain the following:

$ spack spec netlib-scalapack ^mpi=intel-parallel-studio@cluster+mpi
Input spec
--------------------------------
netlib-scalapack
    ^intel-parallel-studio@cluster+mpi

Concretized
--------------------------------
==> Error: The spec
    'intel-parallel-studio@cluster+mpi'
    is configured as not buildable, and no matching external installs were found

i.e. the configuration in packages.yaml has the meaning of "don't ever build anything that might provide lapack" (even if that lapack is not used in the spec). That seems sensible to me but I want to double check if we are considering a different behavior here.

becker33 · 2020-04-14T17:52:41Z

That is correct about what #14934 did. I'm not sure that we want to keep that behavior, we can update it later to be more expressive

tgamblin

@alalazo: this looks pretty good so far -- two major things:

This needs to handle when conditionals for bound virtuals
The resolved virtuals should be made into edge attributes in the DAG, so that we know what's providing what.

(2) would fix the big TODO for scalapack in intel-parallel-studio's libs method.

lib/spack/docs/basic_usage.rst

lib/spack/external/__init__.py

lib/spack/spack/provider_index.py

lib/spack/spack/spec.py

tgamblin · 2020-04-15T07:10:27Z

lib/spack/spack/spec.py

+            msg = 'virtual dependency "{0}" cannot be specified multiple times'
+            raise ValueError(msg.format(virtual))
+
+        self._explicit_providers[virtual] = spec


This is a fine check, (as it doesn't really know whether anything is virtual) but it may have to go away and be checked later. It is right on the edge of including package/concretization semantics in the spec parser.

I'm on the fence because while I can't think of a reason we would ever want the same virtual twice in a DAG, I don't see why we shouldn't be able to talk about a DAG with that property. A spec DAG can represent that -- it's just not a valid build.

I completely agree with the reasoning above: one thing is construct an abstract spec, another is trying to concretize it using some constraints. I added the check above as I was trying to follow a practice that is already present for non-virtual dependencies:

spack/lib/spack/spack/spec.py

Lines 1113 to 1117 in 04c6a78

def _add_dependency(self, spec, deptypes):

"""Called by the parser to add another spec as a dependency."""

if spec.name in self._dependencies:

raise DuplicateDependencyError(

"Cannot depend on '%s' twice" % spec)

This results in the following behavior:

Spack version 0.14.2 Python 3.7.4, Linux x86_64 >>> import spack.spec >>> s = spack.spec.Spec('hdf5 ^zlib~shared ^zlib+shared') Traceback (most recent call last): File "<console>", line 1, in <module> File "/home/culpo/PycharmProjects/spack/lib/spack/spack/spec.py", line 1035, in __init__ spec_list = SpecParser(self).parse(spec_like) File "/home/culpo/PycharmProjects/spack/lib/spack/spack/parse.py", line 152, in parse return self.do_parse() File "/home/culpo/PycharmProjects/spack/lib/spack/spack/spec.py", line 4113, in do_parse self._parse_dependency(specs) File "/home/culpo/PycharmProjects/spack/lib/spack/spack/spec.py", line 4184, in _parse_dependency specs[-1]._add_dependency(dep, ()) File "/home/culpo/PycharmProjects/spack/lib/spack/spack/spec.py", line 1144, in _add_dependency "Cannot depend on '%s' twice" % spec) spack.spec.DuplicateDependencyError: Cannot depend on 'zlib+shared' twice

Should we move both checks just before concretization and verify there the pre-requisites of an abstract spec?

lib/spack/spack/spec.py

alalazo · 2020-05-08T15:38:01Z

@tgamblin 4181bc7 solves the reindex issue (which doesn't fail) but doesn't go as far as recomputing edge attributes. For specs installed before this PR a reindex will always add an empty list under the provides attribute. I think this is fine, but let me know if some further action needs to be taken.

The root spec byte blob is now generated on the fly for tests.

This should help to better convey the meaning of this attribute i.e. store somewhere the user requested providers so that we can honor those requests when concretizing.

Previously __contains__ was just checking nodes properties i.e. it was checking whether a spec *might* provide a virtual dependency or not. Since this PR annotates virtual dependencies on edges, we should now check that the edge provides the correct virtual.

alalazo · 2020-10-17T08:50:36Z

Converted the PR back to draft since:

The syntax to specify virtual packages from command line needs to be revisited (it's in conflict with Environments: specify packages for developer builds #15256, now merged)
It's highly probable that the new concretizer will be merged before this PR, so support for it needs to be added here.

…e it fixes spack#26866 This semantics fits with the way Spack currently treats providers of virtual dependencies. It needs to be revisited when spack#15569 is reworked with a new syntax.

…e it fixes #26866 This semantics fits with the way Spack currently treats providers of virtual dependencies. It needs to be revisited when #15569 is reworked with a new syntax.

cosmicexplorer · 2022-09-29T23:56:43Z

The changes in #29093 may help to unblock the extension to the spec language mentioned in the final bullet of the OP:

Rebase after Environments: specify packages for developer builds #15256 is merged, revisit extension to the spec language

alalazo · 2023-02-03T13:58:02Z

Superseded by #35322

alalazo added concretization virtual-dependencies labels Mar 19, 2020

alalazo requested review from becker33, scheibelp, tgamblin and tldahlgren March 19, 2020 12:29

alalazo force-pushed the features/finer_virtual_selection branch from 116a236 to ded08d5 Compare March 24, 2020 13:22

alalazo marked this pull request as ready for review March 24, 2020 14:00

tgamblin self-assigned this Mar 24, 2020

alalazo force-pushed the features/finer_virtual_selection branch 2 times, most recently from 867248f to d2b6458 Compare March 25, 2020 17:27

alalazo force-pushed the features/finer_virtual_selection branch from d16e364 to 70c58df Compare April 1, 2020 11:58

tgamblin requested changes Apr 15, 2020

View reviewed changes

alalazo force-pushed the features/finer_virtual_selection branch 3 times, most recently from 1a12699 to b91f20f Compare April 20, 2020 10:42

alalazo commented Apr 20, 2020

View reviewed changes

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

alalazo force-pushed the features/finer_virtual_selection branch from b91f20f to a5bfe28 Compare April 20, 2020 16:27

alalazo added the hash-change label Apr 20, 2020

alalazo commented Apr 21, 2020

View reviewed changes

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

alalazo added the pkg-database label May 4, 2020

This comment has been minimized.

Sign in to view

alalazo force-pushed the features/finer_virtual_selection branch 2 times, most recently from 1dda5a7 to 4181bc7 Compare May 8, 2020 14:26

alalazo force-pushed the features/finer_virtual_selection branch from 4181bc7 to afcfb74 Compare May 9, 2020 10:34

alalazo added 13 commits September 9, 2020 10:39

Renamed "virtuals" to "provides" in spec YAML dictionary

cd54221

Fixed issue with test/ci.py

0e4ee81

The root spec byte blob is now generated on the fly for tests.

Renamed "_explicit_providers" to "_user_requested_providers"

b2e044f

This should help to better convey the meaning of this attribute i.e. store somewhere the user requested providers so that we can honor those requests when concretizing.

Fixed issue with reindex and a failing unit test

5cd7027

Updated documentation according to review comments

ef93be9

Spec._providers is now a private method

e718b3b

flake8: comply to new rules on variable naming

aedaa78

Removed spurious FIXME

9a18d56

Removed ^mpich from test spec

8adea7a

Properly reconstruct virtuals on edges for old DBs

263de40

Preserve monkeypatching of objects if name is virtual

da431ce

Copy specs when parsing user requested providers

22a20f4

alalazo force-pushed the features/finer_virtual_selection branch from 2192088 to 4622489 Compare September 9, 2020 09:23

alalazo marked this pull request as draft October 17, 2020 08:46

scheibelp mentioned this pull request Apr 19, 2021

netlib-lapack install issue #23073

Closed

4 tasks

alalazo mentioned this pull request Sep 20, 2021

Allow for multiple dependencies/dependents from the same package #21683

Closed

3 tasks

alalazo mentioned this pull request Oct 21, 2021

concretizer does not take a unique provider #26866

Closed

alalazo mentioned this pull request Oct 21, 2021

ASP-based solver: a package eligible to provide a virtual must provide it #26875

Merged

2 tasks

cosmicexplorer mentioned this pull request Sep 30, 2022

cleanup spack.parse.Lexer #29093

Closed

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

alalazo closed this Feb 3, 2023

haampie added the hash-change things that will change a lot of hashes label Nov 7, 2024

alalazo deleted the features/finer_virtual_selection branch September 16, 2025 09:40

	def _add_dependency(self, spec, deptypes):
	"""Called by the parser to add another spec as a dependency."""
	if spec.name in self._dependencies:
	raise DuplicateDependencyError(
	"Cannot depend on '%s' twice" % spec)

Conversation

alalazo commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tldahlgren commented Mar 24, 2020

Uh oh!

tldahlgren commented Mar 24, 2020

Uh oh!

alalazo commented Apr 1, 2020

Uh oh!

becker33 commented Apr 14, 2020

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgamblin Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

alalazo Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

alalazo commented May 8, 2020

Uh oh!

alalazo commented Oct 17, 2020

Uh oh!

cosmicexplorer commented Sep 29, 2022

Uh oh!

alalazo commented Feb 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alalazo commented Mar 19, 2020 •

edited

Loading