Support deps with constraints based on package versions by greenc-FNAL · Pull Request #37418 · spack/spack

greenc-FNAL · 2023-05-03T20:54:33Z

Add optional version_translator function argument to depends_on()

This allows for packages to require—with a single directive—a dependency to have version requirements that are related to the selected version of the parent. This may be an exact match to the parent's version, a major-minor version match, or any other reasonable relation.

For example: the geant4-data package nominally defines a set of versioned dependencies on data packages which are required by the geant4 package whose version matches that of geant4-data. Prior to this PR, the following code is necessary in the geant4 recipe:

    for _vers in [
        "10.3.3",
        "10.4.0",
        "10.4.3",
        "10.5.1",
        "10.6.0",
        "10.6.1",
        "10.6.2",
        "10.6.3",
        "10.7.0",
        "10.7.1",
        "10.7.2",
        "10.7.3",
        "10.7.4",
        "11.0.0:11.0",
        "11.1:",
    ]:
        depends_on("geant4-data@" + _vers, type="run", when="@" + _vers)

With this PR, this could be replaced by the single line:

    depends_on("geant4-data", spack.version.Version)

If one looks more closely at geant4-data, one might discover that the datasets are generally the same within a minor version. This could be handled by using:

    depends_on("geant4-data", lambda v: v.up_to(2))

with mostly only minor versions (10.1, 10.2, etc.) defined for the geant4-data package and special cases handled by patch versions and conflicts() directives in the geant4-data recipe.

greenc-FNAL · 2023-05-03T20:57:00Z

Submitted as draft for early comments. Tests, etc., incoming.

h/t @obreitwi for ideas from #14002.

adamjstewart · 2023-05-03T22:58:48Z

Can you add a section to the docs explaining how this would work, for example if you want to keep versions the same? Also, are there any potential use cases aside from version matching? Just want to keep things simple if possible.

adamjstewart · 2023-05-04T01:02:59Z

lib/spack/spack/directives.py



-def _depends_on(pkg, spec, when=None, type=default_deptype, patches=None):
+def pkg_depends_on(pkg, spec, when=None, type=default_deptype, patches=None):


Is this stuff related to one of your other PRs?

Yes, that's #37380

greenc-FNAL · 2023-05-04T13:02:35Z

Can you add a section to the docs explaining how this would work, for example if you want to keep versions the same?

NP.

Also, are there any potential use cases aside from version matching? Just want to keep things simple if possible.

One might want to do something similar with variants, but version_translator would become constraint_specs, accepting pkg and returning a list of anonymous specs (or strings convertible to same). In the original use case of the version, the function would need to do its own loop over pkg.versions and convert each Version to a string with prepended "@."

Let me know if you'd rather I do that.

adamjstewart · 2023-05-04T15:52:39Z

If we also want to handle variants, should we change the name to something more generic?

greenc-FNAL · 2023-05-04T19:43:28Z

If we also want to handle variants, should we change the name to something more generic?

How about, constraint_generator?

Do bear in mind that it would become less simple to write a same-version constraint function, but we could provide a bridge function.

adamjstewart · 2023-05-04T20:19:44Z

Honestly the only thing I really need this for at the moment is for packages that require matching versions, so I would be fine with something even simpler. Curious what @alalazo thinks, this is right up his alley.

greenc-FNAL · 2023-05-04T22:29:39Z

@adamjstewart Please find an example implementation of the general case at https://github.com/greenc-FNAL/spack/tree/feature/depends_on-constraint-generator.

An example depends_on() call for the same-version case would look like:

depends_on("my_dep", ..., constraint_generator=version_constraint_generator())

For a minor-version match:
An example depends_on() call for the same-version case would look like:

depends_on("my_dep", ..., constraint_generator=version_constraint_generator(lambda v: v.up_to(2)))

A more general constraint generator would have to look something like:

https://github.com/greenc-FNAL/spack/blob/3ad662dc29db03c054f2410ebb345ce9719ff9be/lib/spack/spack/directives.py#L512-L534

That is just a first pass. If you want something this general I can submit a draft PR to replace this one and we can iterate.

Otherwise, we can go with this PR as-is and I can put tests and docs together ready for review. Please let me know.

greenc-FNAL · 2023-06-30T21:08:07Z

@adamjstewart Bump?

adamjstewart · 2023-06-30T21:39:06Z

Curious what @alalazo thinks

greenc-FNAL · 2023-07-03T17:01:33Z

@spackbot fix style

spackbot-app · 2023-07-03T17:01:35Z

Let me see if I can fix that for you!

spackbot-app · 2023-07-03T17:02:29Z

I was able to run spack style --fix for you!

spack style --fix

==> Running style checks on spack
  selected: isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/directives.py
  lib/spack/spack/test/directives.py
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/directives.py
All done! ✨ 🍰 ✨
1 file reformatted, 1 file left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 577 source files
  mypy checks were clean
==> spack style checks were clean

Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

greenc-FNAL · 2023-08-23T12:58:55Z

@adamjstewart @scheibelp Bump?

scheibelp

I have some preliminary remarks.

Also broadly speaking of the intent, this appears to generalize #14002, but my thought is: if the version_translator function becomes complex, it is not much different than defining a for-loop in the parent package, so I'm wondering what cases you expect it to be useful in.

scheibelp · 2023-09-05T22:40:31Z

lib/spack/spack/directives.py

+        # If we haven't already produced one or more version-specific
+        # dependencies, do the basic dependency for spec.
+        if not executed:
+            pkg_depends_on(pkg, spec, when=when, type=type, patches=patches)


I don't see why depends_on was renamed to _depends_on (this confusion persists for me after reading through #37380).

depends_on wasn't renamed to _depends_on by me: _depends_on() is a private implementation function called by the depends_on directive. #37380 proposed renaming _depends_on() to pkg_depends_on() to make it public and accessible directly from recipes.

scheibelp · 2023-09-05T22:41:08Z

lib/spack/spack/directives.py

+
+        # If we haven't already produced one or more version-specific
+        # dependencies, do the basic dependency for spec.
+        if not executed:


Remove executed variable: make this an else:

That's not the logic of this function—if version_translator is defined, then a single depends_on() directive produces up to length(pkg.versions) distinct dependency relationships, depending on how many non-vacuous calls to version_translator(pkg_version) produce a constraint that actually alters the spec.

This number may be zero, in which case a "basic" dependency must be generated as it would be in the case where no version_translator is defined.

scheibelp · 2023-09-05T22:42:55Z

lib/spack/spack/directives.py

+                version_ish = version_translator(pkg_version)
+                if version_ish is not None:
+                    constrained_spec = spack.spec.Spec(spec)
+                    if constrained_spec.constrain(f"@{version_ish}"):


This method always returns something or raises an exception, so if will not be the way to handle this.

Also, what is a version-ish? I'm thinking version_translator ought to be returning a Spec or a Version; the former if we want to attach a variant specification like +cuda to the dependencies that we define - although I'm not sure if that's necessary since we can separately say

# 1.0 needs [email protected], etc.. depends_on(child, version_translator=...) # When we enable +variant, child should be built that way too depends_on(child+variant, when=+variant)

Adding examples to the PR description of what you want to be able to write in a package.py would be useful, and we'd also want tests to enable this.

I don't understand what you mean, here: "This method always returns ..."

The depends_on() directive here always returns the _execute_depends_on function. _execute_depends_on() always results in at least one execution of the _depends_on() function, just as did the pre-PR implementation.

version_translator()—if defined by the user—may return anything, but the intention is for it to return either None or something that converts to a string which is a valid version part of a version constraint (hence version_ish). This could be a valid Version instance (which could be a standard version, range, list, or any other valid version type), or it could be a string.

scheibelp · 2023-09-05T22:45:41Z

lib/spack/spack/directives.py

+                        pkg_depends_on(
+                            pkg,
+                            constrained_spec,
+                            when=f"@{pkg_version} {when}" if when else f"@{pkg_version}",


This will need to constrain to @={pkg_version} rather than @{pkg_version}

See #37418 (comment): version_translator() may not return a single, exact, version constraint.

scheibelp · 2023-09-05T22:49:24Z

lib/spack/spack/directives.py

            ``str`` to be passed to ``patch``, or a list of these
+        version_translator (function): function to generate a version-ish for spec
+            from a parent version. If provided, this function will be called for
+            each version defined for pkg, and any non-vacuous result will be used to


this function will be called for each version defined for pkg

Imprecise wording, I suggest: the when will be applied for each parent version which yields a result when passed to version_translator.

adamjstewart · 2023-09-06T00:08:42Z

I agree with @scheibelp, I think this could be made simpler unless there is a need to make it more generic.

Tweak test for function name change Test directive enhancement

greenc-FNAL · 2023-12-12T00:04:09Z

@spackbot help

spackbot-app · 2023-12-12T00:04:10Z

You can interact with me in many ways!

@spackbot hello: say hello and get a friendly response back!
@spackbot help or @spackbot commands: see this message
@spackbot run pipeline or @spackbot re-run pipeline: to request a new run of the GitLab CI pipeline
@spackbot rebuild everything: to run a pipeline rebuilding all specs from source.
@spackbot fix style if you have write and would like me to run spack style --fix for you.
@spackbot maintainers or @spackbot request review: to look for and assign reviewers for the pull request.

I'll also help to label your pull request and assign reviewers!
If you need help or see there might be an issue with me, open an issue here

greenc-FNAL · 2023-12-12T00:04:23Z

@spackbot run pipeline

spackbot-app · 2023-12-12T00:04:25Z

I'm sorry, gitlab does not have your latest revision yet, I can't run that pipeline for you right now.

One likely possibility is that your PR pipeline has been temporarily deferred, in which case, it is awaiting a develop pipeline, and will be run when that finishes.

Please check the gitlab commit status message to see if more information is available.

Details

Unexpected response from gitlab: {'message': '404 Commit Not Found'}

tgamblin · 2023-12-13T09:20:54Z

So not to jump in too late here, but what do people think of this syntax?

depends_on("geant4-data@{my.version}", when="@10:")

I am trying to think through the details, but in this model, {my.version} would basically be an expression template that expands out the depends_on() to all possible values of some spec attribute. my here would be used like a proxy for this package's spec, with a limited set of allowed attributes: basically the ones we know how to expand all values of. I think this makes it intuitive to filter by the when= clause (to be consistent with other directives). It would also enable you to, e.g., only do version matching only on certain versions (10 or higher here).

Expressions could be more complex as @greenc-FNAL has demonstrated above:

depends_on("geant4-data@{my.version.up_to(2)}", when="@10:")

And you could use this on variants as well:

depends_on("hdf5 mpi={my.variants['mpi'].value}")

Or maybe more like our format strings:

depends_on("hdf5 mpi={my.variants.mpi.value}")

The final advantage I see here is that for certain spec attributes (e.g. variants) we may not know all possible values until we set up the solve (integer valued variants are an example). Using a syntax like this (vs. an imperative callback) would allow us to push the implementation down to ASP where needed, though we would probably start with something package-level.

I think this could be made to work -- thinking through how best to explode the expression for different values.

Thoughts?

greenc-FNAL · 2023-12-13T14:31:17Z

@tgamblin This seems like a great idea in principle. I'm a little worried about the possible difficulties of referring to variants (say) in the expression that may only exist conditionally. I also don't have any feel for exactly where this would be implemented and how it would reasonably integrate with the existing directive-handling logic or the ASP.

adamjstewart · 2024-05-08T08:03:01Z

Related to this, JAX has a slightly different constraint that I haven't seen before in other packages. Basically, every version of py-jax requires a version of py-jaxlib equal to or older than py-jax. So with @tgamblin's syntax, I believe this would look like:

depends_on("py-jaxlib@:{my.version}", when="@{my.version}")

May be a good test case for whichever solution we end up going with.

greenc-FNAL requested a review from adamjstewart May 3, 2023 20:54

spackbot-app bot added core PR affects Spack core functionality directives tests General test capability(ies) labels May 3, 2023

greenc-FNAL mentioned this pull request May 3, 2023

[geant4] Restore single point of maintenance for versions #37381

Closed

adamjstewart reviewed May 4, 2023

View reviewed changes

greenc-FNAL force-pushed the feature/depends_on-version-translator branch from aade3c8 to 1b73817 Compare June 30, 2023 21:09

greenc-FNAL force-pushed the feature/depends_on-version-translator branch from 888e9a8 to 537c939 Compare July 7, 2023 14:37

adamjstewart mentioned this pull request Jul 17, 2023

Unable to extend virtual dependencies #17475

Open

scheibelp self-assigned this Aug 23, 2023

greenc-FNAL force-pushed the feature/depends_on-version-translator branch from 537c939 to 1fe636e Compare August 23, 2023 17:35

scheibelp reviewed Sep 5, 2023

View reviewed changes

greenc-FNAL force-pushed the feature/depends_on-version-translator branch from 1fe636e to d51e1dc Compare September 22, 2023 20:00

greenc-FNAL added 2 commits December 11, 2023 17:02

Enable package-specific dependency directives

1d10d5f

Tweak test for function name change Test directive enhancement

Clarify argument documentation per spack#37418 (comment)

face9b1

greenc-FNAL force-pushed the feature/depends_on-version-translator branch from d51e1dc to face9b1 Compare December 11, 2023 23:03

Fix argument documentation per CI complaint

5dacba4

greenc-FNAL marked this pull request as ready for review December 12, 2023 00:03

greenc-FNAL requested a review from scheibelp December 12, 2023 14:27

tgamblin self-requested a review December 13, 2023 07:22

tgamblin mentioned this pull request Dec 14, 2023

Allow packages to depend on my.version, variant=my.value, etc. #41700

Draft



		def _depends_on(pkg, spec, when=None, type=default_deptype, patches=None):
		def pkg_depends_on(pkg, spec, when=None, type=default_deptype, patches=None):

Conversation

greenc-FNAL commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greenc-FNAL commented May 3, 2023

Uh oh!

adamjstewart commented May 3, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greenc-FNAL commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented May 4, 2023 • edited by greenc-FNAL Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greenc-FNAL commented May 4, 2023

Uh oh!

adamjstewart commented May 4, 2023

Uh oh!

greenc-FNAL commented May 4, 2023

Uh oh!

greenc-FNAL commented Jun 30, 2023

Uh oh!

adamjstewart commented Jun 30, 2023

Uh oh!

greenc-FNAL commented Jul 3, 2023

Uh oh!

spackbot-app bot commented Jul 3, 2023

Uh oh!

spackbot-app bot commented Jul 3, 2023

Uh oh!

greenc-FNAL commented Aug 23, 2023

Uh oh!

scheibelp 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!

adamjstewart commented Sep 6, 2023

Uh oh!

greenc-FNAL commented Dec 12, 2023

Uh oh!

spackbot-app bot commented Dec 12, 2023

Uh oh!

greenc-FNAL commented Dec 12, 2023

Uh oh!

spackbot-app bot commented Dec 12, 2023

Uh oh!

tgamblin commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greenc-FNAL commented Dec 13, 2023

Uh oh!

adamjstewart commented May 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

greenc-FNAL commented May 3, 2023 •

edited

Loading

greenc-FNAL commented May 4, 2023 •

edited

Loading

adamjstewart commented May 4, 2023 •

edited by greenc-FNAL

Loading

tgamblin commented Dec 13, 2023 •

edited

Loading