Allow users to specify root env dir by psakievich · Pull Request #32836 · spack/spack

psakievich · 2022-09-27T04:38:59Z

Environments managed by spack have some advantages over anonymous Environments but they are tucked away inside spack's directory tree. This PR gives users the ability to specify where the environments should live.

See #32823

Environments managed by spack have some advantages over anonymous Environments but they are tucked away inside spack's directory tree. This PR gives users the ability to specify where the environments should live. See #32823

lib/spack/spack/environment/environment.py

becker33

This is a great idea

lib/spack/spack/environment/environment.py

Co-authored-by: Greg Becker <[email protected]> Add back path canonicalization

psakievich · 2022-09-27T18:11:28Z

Closes #32823

tldahlgren

I suggest a few minor typo corrections and have a few questions.

@tgamblin @becker33 What would (or should) happen if a user already has environments defined in the default path then updates their configuration to point elsewhere?

Is it worth considering supporting a list of environment paths?

lib/spack/docs/environments.rst

lib/spack/spack/test/config.py

share/spack/qa/setup.sh

lib/spack/spack/environment/environment.py

lib/spack/spack/util/path.py

tldahlgren

Switching from comment to request changes due to the typos needing fixed.

Co-authored-by: Tamara Dahlgren <[email protected]>

psakievich · 2022-09-28T04:02:33Z

@tldahlgren I thought about having a list of directories. This is a good question. It complicated things a bit which is why I didn't pursue it.

One thing I'm not sure how to handle is where we add the new environments to. Thinking further, if we treat this like a stack then we could just add environments to the directory at the top of the stack. Another option could be to only allow two entries: the default directory and allow for a single option from the config. I think I like the second option more. I'm not sure the utility of expanding beyond a single user location and the default location.

In the current form the already created environments in the default directory would become anonymous environments. Speaking for myself I typically only use one or the other (registered vs anonymous) in a given spack instance but that may not be reflective of the broader community.

psakievich · 2022-09-28T04:21:15Z

Hmm another potential issue for this is if the environments have the same name in both locations

psakievich · 2023-02-21T21:21:42Z

@tldahlgren and @becker33 this is ready finally. After #34758 some of the logic regarding environment paths and avoiding circular imports was cleaned up. Most of the hang up on this was due to tests using the same mutable_mock_env_path in parallel. I addressed this by making the fixture use a function scope and then cleaning up two more sets of tests.

becker33

Besides the one inline comment, everything looks good to me. I agree with your assessment on slack that it's confusing that this is coming up for this PR and not previously. My best guess without fully diving in to debug it is that previously things "just happened" to work because environments would coincidentally wind up in the same place, even though they weren't being mocked in a way that was thread safe.

becker33 · 2023-02-21T21:36:09Z

bin/spack-tmpconfig

    root: $TMP_DIR/install
  misc_cache: $$user_cache_path/cache
  source_cache: $$user_cache_path/source
+  environments_root: $TMP_DIR/envs


I'm having trouble tracking why this change is necessary.

This was a suggestion from @trws to sandbox parallel execution. I don't fully understand it either. @trws perhaps you could comment?

That script is used to separate the unit tests when run in parallel. It ensures that each test runner gets its own separate spack environment so they don't tromp each other. If the environments_root defaults to being under user_cache_path or something then it's unnecessary, but if it's in the spack_root by default or something like that then the script needs to put it somewhere else.

Yeah @psakievich and I came to the conclusion that this probably isn't technically necessary since all of the environment tests use a mock environment root anyway, but that we should keep it to ensure future tests are separated for parallel execution.

Sounds right to me. The script is meant to be a safety net and a workaround for all the tests that haven't been converted yet. It's also oddly useful as a way to do one-off testing without messing up a current environment just on the command line, so having it there helps with that kind of use.

becker33

I'm sorry this became such an unholy pain to get ready to merge

tldahlgren

LGTM. Just a couple of questions.

lib/spack/docs/environments.rst

lib/spack/spack/environment/environment.py

lib/spack/spack/test/cmd/concretize.py

tldahlgren

Would prefer the 'named' -> 'managed' comment change but won't require the change.

lib/spack/docs/environments.rst

tldahlgren

LGTM

docs: consistently use 'managed environment' over 'named ...'

tldahlgren

LGTM. Thanks.

* Allow users to specify root env dir Environments managed by spack have some advantages over anonymous Environments but they are tucked away inside spack's directory tree. This PR gives users the ability to specify where the environments should live. See spack#32823 This is also taken as an opportunity to ensure that all references are to "managed environments", rather than "named environments". Prior to this PR some references to the latter persisted. Co-authored-by: Tom Scogland <[email protected]> Co-authored-by: Tamara Dahlgren <[email protected]> Co-authored-by: Gregory Becker <[email protected]> Conflicts: lib/spack/spack/test/cmd/env.py lib/spack/spack/test/env.py

* Allow users to specify root env dir Environments managed by spack have some advantages over anonymous Environments but they are tucked away inside spack's directory tree. This PR gives users the ability to specify where the environments should live. See spack#32823 This is also taken as an opportunity to ensure that all references are to "managed environments", rather than "named environments". Prior to this PR some references to the latter persisted. Co-authored-by: Tom Scogland <[email protected]> Co-authored-by: Tamara Dahlgren <[email protected]> Co-authored-by: Gregory Becker <[email protected]>

Allow users to specify root env dir

3f2e77e

Environments managed by spack have some advantages over anonymous Environments but they are tucked away inside spack's directory tree. This PR gives users the ability to specify where the environments should live. See #32823

spackbot-app bot added core PR affects Spack core functionality defaults environments tests General test capability(ies) utilities labels Sep 27, 2022

psakievich commented Sep 27, 2022

View reviewed changes

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

Rework path access strategy

490b5ee

becker33 requested changes Sep 27, 2022

View reviewed changes

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

becker33 reviewed Sep 27, 2022

View reviewed changes

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

psakievich and others added 2 commits September 27, 2022 11:17

Update lib/spack/spack/environment/environment.py

76fde63

Co-authored-by: Greg Becker <[email protected]> Add back path canonicalization

Add some documentation

9ecdafd

spackbot-app bot added the documentation Improvements or additions to documentation label Sep 27, 2022

Add check that env_root dir exists

076d60c

psakievich requested a review from becker33 September 27, 2022 18:05

psakievich marked this pull request as ready for review September 27, 2022 18:05

avoid using realpath, readlink -f is more portable

2cae953

spackbot-app bot added the shell-support label Sep 27, 2022

Merge branch 'develop' into f/env-location

bce2d38

tldahlgren reviewed Sep 28, 2022

View reviewed changes

tldahlgren assigned becker33 and tldahlgren Sep 28, 2022

tldahlgren requested changes Sep 28, 2022

View reviewed changes

psakievich and others added 2 commits September 27, 2022 21:08

Update lib/spack/docs/environments.rst

addb891

Co-authored-by: Tamara Dahlgren <[email protected]>

Update lib/spack/spack/test/config.py

da7294c

Co-authored-by: Tamara Dahlgren <[email protected]>

Change name of variable

ef43044

Merge branch 'develop' into f/env-location

c144558

psakievich added 6 commits February 21, 2023 09:18

Undo hoops for avoiding env include

29fbad2

Add unit-tests back

36bf6c9

Style

2acc356

Try setting mutable_mock_env in each func

47cdefc

Try to resolve uninstall failure

9debeaf

remove env's

4ae9730

becker33 reviewed Feb 21, 2023

View reviewed changes

becker33 previously approved these changes Feb 21, 2023

View reviewed changes

tldahlgren reviewed Feb 21, 2023

View reviewed changes

lib/spack/docs/environments.rst Outdated Show resolved Hide resolved

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

lib/spack/spack/test/cmd/concretize.py Show resolved Hide resolved

tldahlgren previously approved these changes Feb 21, 2023

View reviewed changes

psakievich commented Feb 21, 2023

View reviewed changes

lib/spack/docs/environments.rst Outdated Show resolved Hide resolved

Update lib/spack/docs/environments.rst

c16f166

psakievich dismissed stale reviews from tldahlgren and becker33 via c16f166 February 21, 2023 23:33

psakievich requested review from becker33 and tldahlgren February 21, 2023 23:34

tldahlgren previously approved these changes Feb 21, 2023

View reviewed changes

becker33 and others added 2 commits February 21, 2023 15:53

docs: consistently use 'managed environment' over 'named ...'

79e821a

Merge pull request #53 from spack/f/env-location

603ec8f

docs: consistently use 'managed environment' over 'named ...'

psakievich dismissed tldahlgren’s stale review via 603ec8f February 21, 2023 23:57

becker33 approved these changes Feb 21, 2023

View reviewed changes

tldahlgren approved these changes Feb 22, 2023

View reviewed changes

becker33 enabled auto-merge (squash) February 22, 2023 00:04

becker33 merged commit b8d15e8 into spack:develop Feb 22, 2023

psakievich deleted the f/env-location branch November 8, 2023 04:21

This was referenced Sep 13, 2024

spack system isolation with out-of-spack databases #31154

Closed

feature: sharing a spack instance #11871

Open

Conversation

psakievich commented Sep 27, 2022

Uh oh!

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

psakievich commented Sep 27, 2022

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

psakievich commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psakievich commented Sep 28, 2022

Uh oh!

psakievich commented Feb 21, 2023

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

becker33 Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

psakievich Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

trws Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

becker33 Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

trws Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

psakievich commented Sep 28, 2022 •

edited

Loading