Add recursive argument to spack develop #46885

psakievich · 2024-10-09T12:10:45Z

This effort allows for a recursive develop call
which will traverse from the develop spec given back to the root(s) and mark all packages along the path as develop.

If people are doing development across the graph then paying fetch and full rebuild costs every time spack install is called is unnecessary and expensive. Since this requires an already concrete environment the exact versions can be used and there is no need to guess or provide versions for concrete specs.

Additional changes:

Remove the constraint of only one develop spec. Users can specify more than one unless they pass spec specific configurations like --path or --build-directory.
Change the default check for the version to make a non-concrete CLI spec concrete. The order now is:
1. If the environment is concrete take the version in the already concretized environment (highest version, if there happens to be repeats)
2. Next take max(package_class.versions)

This effort allows for a recursive develop call which will traverse from the develop spec given back to the root(s) and mark all packages along the path as develop. If people are doing development across the graph then paying fetch and full rebuild costs every time spack develop is called is unnecessary and expensive. Also remove the constraint for concrete specs and simply take the max(version) if a version is not given. This should default to the highest infinity version which is also the logical best guess for doing development.

tgamblin · 2024-10-10T06:08:34Z

Can you split this into two PRs -- one for the removal of the constraint and the other for the recursive option?

psakievich · 2024-10-10T16:16:23Z

I put them together because the recursive makes sense to default to just the package name since it is already concretized. So we should do remove constraint first and then recursive dependent on that merging IMO.

psakievich · 2024-10-15T14:54:58Z

@tgamblin see #46911. I preemptively added you as a reviewer 🙂

psakievich · 2024-10-18T21:34:06Z

I started a unit-test for this, but it's looking like a big lift at the moment. It needs a concretized environment to work, and so far all I've found is mock concretization of a spec. Seems like a notable gap in our testing infrastructure but would dramatically extend the scope of this PR to implement and should be a PR in and of itself IMHO.

psakievich · 2024-10-18T22:49:09Z

Testing this locally for now:

$ spack env activate --temp 
$ spack add exawind
$ spack concretize
$ spack develop -r trilinos
# trilinos actually has a fetch error that I've documented but never fixed #25370 
$ spack develop -r trilinos
# clones see yaml file below

# This is a Spack Environment file.
#
# It describes a set of packages to be installed, along with
# configuration settings.
spack:
  # add package specs to the `specs` list
  specs:
  - exawind
  view: true
  concretizer:
    unify: true
  develop:
    nalu-wind:
      spec: nalu-wind@=master
    exawind:
      spec: exawind@=master
    trilinos:
      spec: trilinos@=16.0.0

psakievich · 2024-10-18T23:01:30Z

@haampie this should meet your spack develop needs as outlined on slack.

lib/spack/spack/cmd/develop.py

haampie · 2024-10-19T08:31:41Z

@haampie this should meet your spack develop needs as outlined on slack.

I doesn't: what I meant was I want be able to select an individual spec from a concrete env, so that the following:

would take the matching curl@8.4.0 and add dev_path=..., and update the hashes of dependents.

I don't want to run spack -e . concretize -f cause that takes forever, and risks getting a completely different spec.

Also, apparently spack -e . concretize -f is broken if I put curl in the develop section with root spec cmake:

So, in short: I wanna be able to add dev_path=... post-hoc, to a single spec selected from a concrete environment, without re-concretization.

psakievich · 2024-10-19T21:03:50Z

So, in short: I wanna be able to add dev_path=... post-hoc, to a single spec selected from a concrete environment, without re-concretization.

Ah well I can add the single spec logic to this PR (prefer concrete if a version is omitted), but me thinks skipping re-concretization is a more fundamental change for another PR.

psakievich · 2024-10-19T21:42:21Z

Thinking about this more I think to skip concretization we'd likely need to change how develop is recognized (make it an environment property rather than a spec property). It messes with a lot of fundamental stuff though. The other option I can think of is a surgical change to the DAG. Modify specs, change hashes, but don't actually solve.

haampie · 2024-10-20T07:52:04Z

Note that spack spec cmake ^curl dev_path=/tmp works ;) With that in mind the whole develop: ... section is kind of redundant to start with...

The other option I can think of is a surgical change to the DAG. Modify specs, change hashes, but don't actually solve.

Yeah, this is my suggestion too. In my opinion a major flaw in the develop command is that you don't know what to develop until the env is concretized, yet it wants to fetch sources ahead of time (including resources and patches which depend on variants, compiler choices). The order should've been: concretize first, then mark things as dev spec, then fetch sources.

psakievich · 2024-10-20T14:40:34Z

@haampie That order makes sense to me. The only reason I see develop needs to be part of the spec with a variant is to mark for differentiation of the install/future reuse. From an install point dev spec doesn't really matter? I guess it does in that you probably don't want to reuse a develop spec or rather don't want it masquerading as something it is not. Other than that I think it should be an environment property and not a spec property with the order you outlined. Oh and hash computation. New variant ensures new hash.

I do like that develop can happen pre concretization for reuse of developer environments etc, but I don't see why that would have to change.

I suspect that the install situation could be alleviated by not installing develop specs or their dependents into the same install tree. Have it be a special one. That actually aligns well with another request of having develop specs link and possibly rebuild out of a build tree and skip installation all together. Rational being linking to build directly means tighter changes (update a header and not have to reinstall just rebuild) and less file system work.

psakievich · 2024-11-06T01:20:22Z

@spackbot fix style

spackbot-app · 2024-11-06T01:20:24Z

Let me see if I can fix that for you!

spackbot-app · 2024-11-06T01:21:32Z

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/cmd/develop.py
  lib/spack/spack/test/cmd/develop.py
  var/spack/repos/builtin.mock/packages/indirect-mpich/package.py
==> Running isort checks
Fixing /tmp/tmpt0b_m93s/spack/lib/spack/spack/test/cmd/develop.py
  isort checks were clean
==> Running black checks
All done! ✨ 🍰 ✨
3 files left unchanged.
  black checks were clean
==> Running flake8 checks
lib/spack/spack/cmd/develop.py:93: [E501] line too long (100 > 99 characters)
lib/spack/spack/test/cmd/develop.py:247: [F841] local variable 'specs' is assigned to but never used
  flake8 found errors
==> Running mypy checks
lib/spack/spack/version/version_types.py:145: error: Argument 2 to "StandardVersion" has incompatible type "*Tuple[Tuple[Any, ...], Tuple[Any, ...]]"; expected "Tuple[Tuple[Any, ...], Tuple[Any, ...]]"  [arg-type]
lib/spack/spack/version/version_types.py:452: error: Argument 2 to "StandardVersion" has incompatible type "*Tuple[Tuple[Any, ...], Tuple[Any, ...]]"; expected "Tuple[Tuple[Any, ...], Tuple[Any, ...]]"  [arg-type]
lib/spack/spack/version/version_types.py:481: error: Argument 2 to "StandardVersion" has incompatible type "*Tuple[Tuple[Any, ...], Tuple[Any, ...]]"; expected "Tuple[Tuple[Any, ...], Tuple[Any, ...]]"  [arg-type]
lib/spack/spack/variant.py:131: error: Unsupported right operand type for in ("Union[Collection[Any], Callable[..., Any]]")  [operator]
Found 4 errors in 2 files (checked 620 source files)
  mypy found errors

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.

lib/spack/spack/cmd/develop.py

Co-authored-by: Greg Becker <becker33@llnl.gov>

lib/spack/spack/cmd/develop.py

this cached value is not properly invalidated when develop config is written, as in the `spack develop` command. Some accesses are accidentally correct because a read or write transaction on the environment will invalidate the `self._dev_specs` as a side-effect. Considered adding invalidation but determined it was not worth the effort to bridge the gap in interface between `spack.config` and `spack.environment`.

becker33 · 2025-02-07T22:38:43Z

lib/spack/spack/cmd/develop.py

@@ -38,28 +40,22 @@ def setup_parser(subparser):
        "--clone",
        action="store_true",
        dest="clone",
-        default=None,
+        default=True,


I don't understand this change -- this seems to suggest in the default case we clone regardless of whether the path is already cloned?

This is suggesting we call the _clone method and attempt to clone. Additional checks are there to explain why we skip cloning. It's been a while but IIRC the logic was embedded off state of None True or False before. But for a recursive clone we need better inferences that I moved inside a method.

lib/spack/spack/cmd/develop.py

psakievich · 2025-02-07T23:11:02Z

@spackbot fix style

spackbot-app · 2025-02-07T23:11:05Z

Let me see if I can fix that for you!

spackbot-app · 2025-02-07T23:12:09Z

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

spack style --fix

==> Running style checks on spack
  selected: import, isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/cmd/develop.py
  lib/spack/spack/environment/environment.py
  lib/spack/spack/test/cmd/develop.py
  var/spack/repos/builtin.mock/packages/indirect-mpich/package.py
==> Running import checks
import check requires Python 3.9 or later
  import checks were clean
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/cmd/develop.py
All done! ✨ 🍰 ✨
1 file reformatted, 3 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
lib/spack/spack/version/version_types.py:134: error: Incompatible types in assignment (expression has type "Tuple[Any, ...]", variable has type "Tuple[str]")  [assignment]
lib/spack/spack/variant.py:130: error: Unsupported right operand type for in ("Union[Collection[Any], Callable[..., Any]]")  [operator]
lib/spack/spack/build_environment.py:171: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [misc]
lib/spack/spack/build_environment.py:171: error: Overloaded function signatures 1 and 3 overlap with incompatible return types  [misc]
Found 4 errors in 3 files (checked 636 source files)
  mypy found errors

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.

becker33 · 2025-02-07T23:22:22Z

lib/spack/spack/cmd/develop.py

+            version = matching_specs[0].version
+            test_spec = spack.spec.Spec(f"{spec}@{version}")
+            for m_spec in matching_specs:
+                if not test_spec.satisfies(m_spec):


I think this needs to be

Suggested change

if not test_spec.satisfies(m_spec):

if not test_spec.intersects(m_spec):

The order was wrong here. we should be checking that all the matching specs in the environment satisfy the test spec which was constructed from the name and version of the first matching spec.

psakievich · 2025-02-25T22:41:52Z

@spackbot fix style

spackbot-app · 2025-02-25T22:41:55Z

Let me see if I can fix that for you!

spackbot-app · 2025-02-25T22:43:32Z

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

spack style --fix

==> Running style checks on spack
  selected: import, isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/cmd/develop.py
  lib/spack/spack/environment/environment.py
  lib/spack/spack/test/cmd/develop.py
  var/spack/repos/builtin.mock/packages/indirect-mpich/package.py
==> Running import checks
  import checks were clean
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/cmd/develop.py
All done! ✨ 🍰 ✨
1 file reformatted, 3 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 637 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.

spackbot-app bot added commands core PR affects Spack core functionality labels Oct 9, 2024

psakievich force-pushed the psakiev/recursive-develop branch from c7dccf3 to b2ff04b Compare October 9, 2024 12:18

tgamblin requested review from tgamblin and becker33 October 10, 2024 06:07

tgamblin self-assigned this Oct 10, 2024

psakievich mentioned this pull request Oct 11, 2024

remove concrete spec constraint from spack develop #46911

Merged

psakievich added 2 commits October 15, 2024 13:41

Merge branch 'develop' into psakiev/recursive-develop

a893eae

Completion commands

08d3f59

spackbot-app bot added the shell-support label Oct 18, 2024

Add docs

0624e7e

spackbot-app bot added the documentation Improvements or additions to documentation label Oct 18, 2024

psakievich marked this pull request as ready for review October 18, 2024 20:34

Updates after testing

3b1e7e4

haampie reviewed Oct 19, 2024

View reviewed changes

lib/spack/spack/cmd/develop.py Outdated Show resolved Hide resolved

haampie reviewed Oct 19, 2024

View reviewed changes

lib/spack/spack/cmd/develop.py Outdated Show resolved Hide resolved

haampie reviewed Oct 19, 2024

View reviewed changes

lib/spack/spack/cmd/develop.py Outdated Show resolved Hide resolved

psakievich added the snl-core-team Issue for SNL Spack developers label Oct 23, 2024

psakievich requested a review from becker33 November 6, 2024 01:13

[@spackbot] updating style on behalf of psakievich

18f83e5

becker33 requested changes Nov 6, 2024

View reviewed changes

lib/spack/spack/cmd/develop.py Outdated Show resolved Hide resolved

lib/spack/spack/cmd/develop.py Outdated Show resolved Hide resolved

lib/spack/spack/cmd/develop.py Outdated Show resolved Hide resolved

psakievich and others added 2 commits November 5, 2024 20:02

Update develop.py

e1aefb2

Co-authored-by: Greg Becker <becker33@llnl.gov>

Improve error message

09a3fe3

psakievich requested a review from becker33 November 6, 2024 22:27

psakievich mentioned this pull request Dec 19, 2024

Strange unit-test failure inside a write-lock #48179

Open

3 tasks

Merge branch 'develop' into psakiev/recursive-develop

dddf9dc

psakievich commented Feb 4, 2025

View reviewed changes

lib/spack/spack/cmd/develop.py Show resolved Hide resolved

psakievich and others added 4 commits February 4, 2025 09:59

Str for type checking

6b9c014

More str type check conversions

02b8ec3

Use the import over string for type check

e368cae

spackbot-app bot added the environments label Feb 7, 2025

becker33 requested changes Feb 7, 2025

View reviewed changes

Cosmetic updates

be1fe6b

[@spackbot] updating style on behalf of psakievich

e17bbed

becker33 requested changes Feb 7, 2025

View reviewed changes

psakievich added 2 commits February 7, 2025 20:44

Fix logic misses

4b57dad

Merge branch 'develop' into psakiev/recursive-develop

d18aa9c

[@spackbot] updating style on behalf of psakievich

83b248c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add recursive argument to spack develop #46885

Add recursive argument to spack develop #46885

psakievich commented Oct 9, 2024 •

edited

Loading

tgamblin commented Oct 10, 2024

psakievich commented Oct 10, 2024

psakievich commented Oct 15, 2024

psakievich commented Oct 18, 2024

psakievich commented Oct 18, 2024

psakievich commented Oct 18, 2024

haampie commented Oct 19, 2024 •

edited

Loading

psakievich commented Oct 19, 2024

psakievich commented Oct 19, 2024

haampie commented Oct 20, 2024 •

edited

Loading

psakievich commented Oct 20, 2024 •

edited

Loading

psakievich commented Nov 6, 2024

spackbot-app bot commented Nov 6, 2024

spackbot-app bot commented Nov 6, 2024

becker33 Feb 7, 2025

psakievich Feb 7, 2025

psakievich commented Feb 7, 2025

spackbot-app bot commented Feb 7, 2025

spackbot-app bot commented Feb 7, 2025

becker33 Feb 7, 2025

psakievich Feb 8, 2025

psakievich commented Feb 25, 2025

spackbot-app bot commented Feb 25, 2025

spackbot-app bot commented Feb 25, 2025

	if not test_spec.satisfies(m_spec):
	if not test_spec.intersects(m_spec):

Add recursive argument to spack develop #46885

Are you sure you want to change the base?

Add recursive argument to spack develop #46885

Conversation

psakievich commented Oct 9, 2024 • edited Loading

tgamblin commented Oct 10, 2024

psakievich commented Oct 10, 2024

psakievich commented Oct 15, 2024

psakievich commented Oct 18, 2024

psakievich commented Oct 18, 2024

psakievich commented Oct 18, 2024

haampie commented Oct 19, 2024 • edited Loading

psakievich commented Oct 19, 2024

psakievich commented Oct 19, 2024

haampie commented Oct 20, 2024 • edited Loading

psakievich commented Oct 20, 2024 • edited Loading

psakievich commented Nov 6, 2024

spackbot-app bot commented Nov 6, 2024

spackbot-app bot commented Nov 6, 2024

becker33 Feb 7, 2025

Choose a reason for hiding this comment

psakievich Feb 7, 2025

Choose a reason for hiding this comment

psakievich commented Feb 7, 2025

spackbot-app bot commented Feb 7, 2025

spackbot-app bot commented Feb 7, 2025

becker33 Feb 7, 2025

Choose a reason for hiding this comment

psakievich Feb 8, 2025

Choose a reason for hiding this comment

psakievich commented Feb 25, 2025

spackbot-app bot commented Feb 25, 2025

spackbot-app bot commented Feb 25, 2025

psakievich commented Oct 9, 2024 •

edited

Loading

haampie commented Oct 19, 2024 •

edited

Loading

haampie commented Oct 20, 2024 •

edited

Loading

psakievich commented Oct 20, 2024 •

edited

Loading