Skip to content

Conversation

@alalazo
Copy link
Member

@alalazo alalazo commented Jan 31, 2023

see #34821

fixes #34886

Currently virtual dependencies are used only during concretization to select a provider. No annotation is kept on edges to reflect that a leaf node is providing a virtual dependency. This PR adds virtual information on edges, so we can keep track of which spec is providing what virtual both in memory and in the JSON / YAML representation.

The way edge attributes are added is generic, so to reuse the same representation later when the use_variant will be implemented, see spack/seps#2. A sample representation is:

        "dependencies": [
          {
            "name": "cmake",
            "hash": "iqpoju67adln3yqzvqzccrtkjmpc666m",
            "parameters": {
              "deptypes": [
                "build"
              ],
              "virtuals": []
            }
          },
          {
            "name": "openmpi",
            "hash": "p23v372ij7bomrarjrhcv4uw6ieahjt3",
            "parameters": {
              "deptypes": [
                "build",
                "link"
              ],
              "virtuals": [
                "mpi"
              ]
            }
          },
          {
            "name": "pkgconf",
            "hash": "i4avrindvhcamhurzbfdaggbj2zgsrrh",
            "parameters": {
              "deptypes": [
                "run"
              ],
              "virtuals": [
                "pkgconfig"
              ]
            }
          },
          {
            "name": "zlib",
            "hash": "nizxi5u5bbrzhzwfy2qb7hatlhuswlrz",
            "parameters": {
              "deptypes": [
                "build",
                "link"
              ],
              "virtuals": []
            }
          }
        ],

Modifications:

  • Add a parameters attribute to DependencySpec, which can hold arbitrary attributes
  • Modify Spec.to_node_dict to output information on virtuals in the `"dependencies" section
  • Add unit tests (round-trip specs / check annotation)
  • Modified DependencySpec to hold an arbitrary number of attributes (98b9806)
  • Reconstruct virtuals from old specfile formats (0b2db30)

@spackbot-app spackbot-app bot added build-systems core PR affects Spack core functionality environments tests General test capability(ies) workflow labels Jan 31, 2023
@alalazo alalazo force-pushed the features/casd/keep_track_of_virtuals_on_edges branch 2 times, most recently from b88123f to 562afeb Compare January 31, 2023 12:09
@tgamblin tgamblin requested review from becker33 and tgamblin January 31, 2023 18:33
@alalazo alalazo force-pushed the features/compilers-as-deps branch from 9875a5c to 84d1d48 Compare February 2, 2023 09:07
@alalazo alalazo linked an issue Feb 2, 2023 that may be closed by this pull request
2 tasks
@alalazo alalazo force-pushed the features/compilers-as-deps branch from 84d1d48 to 8471718 Compare February 3, 2023 13:51
@alalazo alalazo mentioned this pull request Feb 3, 2023
4 tasks
@alalazo alalazo marked this pull request as ready for review February 3, 2023 13:59
@alalazo alalazo force-pushed the features/compilers-as-deps branch from 8471718 to d9bf846 Compare February 3, 2023 18:37
@alalazo alalazo force-pushed the features/compilers-as-deps branch from d9bf846 to d8ae02c Compare February 22, 2023 09:39
self.parent = parent
self.spec = spec
self.deptypes = dp.canonical_deptype(deptypes)
self.parameters = {
Copy link
Member

@haampie haampie Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary to have a dict here? Personally I would keep redundancy & the number of indirections small so that we don't hit even more Python slowness. (One of the bottlenecks is creating the edges when deserializing the db, so if a normal prop works that's likely better than a string and hashmap lookup -- ok, to be fair, we do go from json string -> dict -> props, so in a sense it could also be faster)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using properties is a step towards adding custom attributes on edges. This is needed for "use variants" as proposed in spack/seps#2 and can be implemented on top of #35322, so 2 PRs from here.

"""
possible_virtuals = [x for x in spec.package.dependencies if Spec(x).virtual]
for vspec in possible_virtuals:
if vspec in spec:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to restrict to direct deps here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be done already, since the loop below selects only outgoing edges from the root.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but in case vspec is not a direct dep you walk the whole dag? And what if it is a dependency of a dependency?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turned out the reconstruction was incomplete. See 3f1205a

@alalazo alalazo force-pushed the features/compilers-as-deps branch from d8ae02c to 6ad4286 Compare February 28, 2023 15:38
@alalazo alalazo force-pushed the features/casd/keep_track_of_virtuals_on_edges branch 2 times, most recently from 826f5ac to a8521cd Compare February 28, 2023 15:59
@alalazo alalazo force-pushed the features/compilers-as-deps branch from 6ad4286 to f5eb6ac Compare March 30, 2023 09:04
alalazo added 2 commits March 30, 2023 15:32
This works for both the new and the old concretizer. Also,
added type hints to involved functions.
@alalazo alalazo force-pushed the features/casd/keep_track_of_virtuals_on_edges branch from 3f1205a to 83e5c45 Compare March 30, 2023 13:45
tgamblin added a commit that referenced this pull request Apr 17, 2023
`spec_clauses()` attempts to look up package information for concrete specs in order to
determine which virtuals they may provide. This fails for renamed/deleted dependencies
of buildcaches and installed packages.

This will eventually be fixed by #35258, which adds virtual information on edges, but we
need a workaround to make older buildcaches usable.

- [x] make an exception for renamed packages and omit their virtual constraints
- [x] add a note that this will be solved by adding virtuals to edges
tgamblin added a commit that referenced this pull request Apr 18, 2023
`spec_clauses()` attempts to look up package information for concrete specs in order to
determine which virtuals they may provide. This fails for renamed/deleted dependencies
of buildcaches and installed packages.

This will eventually be fixed by #35258, which adds virtual information on edges, but we
need a workaround to make older buildcaches usable.

- [x] make an exception for renamed packages and omit their virtual constraints
- [x] add a note that this will be solved by adding virtuals to edges
@alalazo alalazo marked this pull request as draft May 4, 2023 17:57
tgamblin added a commit that referenced this pull request May 15, 2023
`spec_clauses()` attempts to look up package information for concrete specs in order to
determine which virtuals they may provide. This fails for renamed/deleted dependencies
of buildcaches and installed packages.

This will eventually be fixed by #35258, which adds virtual information on edges, but we
need a workaround to make older buildcaches usable.

- [x] make an exception for renamed packages and omit their virtual constraints
- [x] add a note that this will be solved by adding virtuals to edges
tgamblin added a commit that referenced this pull request May 15, 2023
`spec_clauses()` attempts to look up package information for concrete specs in order to
determine which virtuals they may provide. This fails for renamed/deleted dependencies
of buildcaches and installed packages.

This will eventually be fixed by #35258, which adds virtual information on edges, but we
need a workaround to make older buildcaches usable.

- [x] make an exception for renamed packages and omit their virtual constraints
- [x] add a note that this will be solved by adding virtuals to edges
alalazo pushed a commit to haampie/spack that referenced this pull request May 15, 2023
`spec_clauses()` attempts to look up package information for concrete specs in order to
determine which virtuals they may provide. This fails for renamed/deleted dependencies
of buildcaches and installed packages.

This will eventually be fixed by spack#35258, which adds virtual information on edges, but we
need a workaround to make older buildcaches usable.

- [x] make an exception for renamed packages and omit their virtual constraints
- [x] add a note that this will be solved by adding virtuals to edges
haampie pushed a commit that referenced this pull request May 15, 2023
`spec_clauses()` attempts to look up package information for concrete specs in order to
determine which virtuals they may provide. This fails for renamed/deleted dependencies
of buildcaches and installed packages.

This will eventually be fixed by #35258, which adds virtual information on edges, but we
need a workaround to make older buildcaches usable.

- [x] make an exception for renamed packages and omit their virtual constraints
- [x] add a note that this will be solved by adding virtuals to edges
@alalazo alalazo self-assigned this Jun 7, 2023
@alalazo alalazo deleted the features/casd/keep_track_of_virtuals_on_edges branch June 15, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-systems core PR affects Spack core functionality environments tests General test capability(ies) workflow

Projects

Development

Successfully merging this pull request may close these issues.

Annotate virtual dependencies on DAG edges

2 participants