New `spack.environment.active_environment` api, and make spack.environment not depend on spack.cmd. #25439

haampie · 2021-08-16T10:13:32Z

Rework spack.environment.get_env(...) to spack.environment.active_environment()
which is now a trivial getter for the active environment.
Introduce spack.cmd.require_active_env(cmd_name=...) which is a wrapper
around spack.environment.active_environment() but errors with a helpful
error message when used from the CLI.
Clean up instances where former spack.environment.get_env was abused
outside the context of a command (fake command name, fake -e flag,
bunch of None, None arguments)
Remove the -e parsing from former get_env, because main.py is
responsible for processing -e and already activates the environment.
In fact this logic was entirely broken, because it did not respect -D at all
for loading an env from a directory, and it did not respect -E (i.e. it would
load an environment even when explicitly disabled).
Move spack.environment.find_environment to
spack.cmd.find_environment, to avoid that spack.environment is aware
of argparse.
Move command line argument parsing of environment installs_specs from
spack.environment to spack.cmd, to avoid that spack.environment is aware
of argparse.
Remove some unused imports, and work around circular imports.
Use ev alias for spack.environment consistently where it works Python 2.7
permitting

TL;DR: better interface for getting the currently active environment, and refactor
things so that spack.environment does not depend on spack.cmd.

haampie · 2021-08-16T12:12:58Z

Added the breaking-change label for posterity because spack.environment.get_env got removed.

haampie · 2021-08-16T14:36:36Z

@gartung any idea why test_default_rpaths_create_install_default_layout is failing? restarted the tests a couple times but it keeps failing.

gartung · 2021-08-16T14:42:12Z

Stage directory was not removed

spack/lib/spack/spack/test/conftest.py

Line 291 in 3773185

assert not files_in_stage

haampie · 2021-08-16T15:14:40Z

Sure, but it's failing spuriously, any idea why?

gartung · 2021-08-16T15:18:36Z

It's a test package so it creates it's own files on the fly. Maybe there is a timing issue with the cleanup of the stage directory.

becker33

I don't love the arrangement of methods in the cmd vs env modules, but I didn't love the old arrangement either so I'm not going to complain too much about that.

A couple minor issues.

lib/spack/spack/cmd/__init__.py

lib/spack/spack/cmd/add.py

becker33 · 2021-08-16T18:59:18Z

lib/spack/spack/cmd/verify.py

@@ -74,7 +73,7 @@ def verify(parser, args):

    elif args.specs_or_files:
        # construct disambiguated spec list
-        env = ev.get_env(args, 'verify')
+        env = spack.environment.get_active_env()


You need to import spack.environment at the top here, and this goes for most of the files where you deleted it.

The only reason it's working without is because the name gets bound from importing spack.store, but you shouldn't rely on that because it's undefined python behavior.

@haampie I think this might be related to the test failure -- you can get some really inscrutable errors with improper imports.

Hm okay, let me try to fix the circular dependencies. Note that def matching_spec_from_env in __init__.py already used spack.environment.<stuff> without importing before this pr.

But I mean, this PR is intended to separate core domain stuff (spack.environment) from the interface layer (spack.cmd), so optimally there would not be any circular deps at all...

Couple of commits were necessary to fix circular imports, but then it's OK to add those spack.environment imports in spack.cmd.*. I've done it over the whole code base in 1ac0eb3, and made it consistently use import spack.environment as ev since this was already done in 55 out of 62 cases.

- Make `spack.environment.get_env` a trivial getter for the active environment. - Introduce `spack.cmd.get_env_for_command(cmd_name, env)` which is a wrapper around spack.env.get_env(env)` but adds support for an error message for the particular command. - Clean up instances where `spack.environment.get_env` was abused outside the context of a command (fake command name, bunch of None arguments which are hard to follow) - Remove the `-e` parsing from `get_env`, because `main.py` is responsible for processing `-e` and already activates the environment. - Move `spack.environment.find_environment` to `spack.cmd.find_environment`, to avoid that spack.environment is aware of argparse.

The way I see it update_kwargs_from_args could have been a constructor for a PackageInstallArgs object, and they could have a named constructor like PackageInstallArgs::from_cli_args(args). So it makes sense to move this function to spack.installer, and with that the last dependency of spack.environment on spack.cmd is gone, as it should be.

haampie · 2021-08-17T21:02:47Z

@spackbot run pipeline

spackbot-app · 2021-08-17T21:02:49Z

I've started that pipeline for you!

alalazo

I left a few comments. Overall I think this is an improvement over current state, and it catches a few places where we were missing imports. There's a function that I think is being moved to the wrong module and a few local imports that I would avoid if possible.

Style question to everyone. Do we want to consistently:

import spack.environment as ev

everywhere? I have a slight preference for fully qualified names, but if we go with an alias everywhere I'm fine with it. It would be good if we can decide on that and have the decision checked automatically by style tests, if possible and easy to do.

lib/spack/spack/environment.py

alalazo · 2021-08-18T07:24:25Z

lib/spack/spack/installer.py

@@ -613,6 +613,42 @@ def package_id(pkg):
    return "{0}-{1}-{2}".format(pkg.name, pkg.version, pkg.spec.dag_hash())


+def update_kwargs_from_args(args, kwargs):


I feel like this is the wrong place for this function if the aim was to have the spack core module not dependent on the cli. Here we assume knowledge on which arguments are passed in an argparse namespace.

Yeah, I tried to explain this in the commit message 8917731:

The way I see it update_kwargs_from_args could have been a constructor
for a PackageInstallArgs object, and they could have a named constructor
like PackageInstallArgs::from_cli_args(args). So it makes sense to move
this function to spack.installer, and with that the last dependency of
spack.environment on spack.cmd is gone, as it should be.

I could move it out of installer.py, but I don't know where it should go instead 🤔 installer_args.py?

Ok, I see that commit. My opinion is that the right solution is to have:

spack/lib/spack/spack/environment.py

Line 1575 in b22728d

def install_specs(self, specs=None, args=None, **install_args):

and similar functions not having the args argument. It should be on the caller of the interface to prepare the install_args appropriately.

Looking at:

spack/lib/spack/spack/cmd/install.py

Line 197 in b22728d

def install_specs(cli_args, kwargs, specs):

it seems this is doable. Besides, update_kwargs_from_args is called internally from Environment.install_specs which receives directly the cli_args and externally in the function above. So why not setting everything in cmd/install.py and just pass a dictionary after a single call to the function?

I've removed args from spack.environment.install_spec(...) and spack.environment.install_all(...) and combined the update_kwargs_from_args in spack.cmd.install. That seems like a better solution indeed.

That's exactly what I had in mind. Thanks!

alalazo · 2021-08-18T07:25:46Z

lib/spack/spack/test/cmd/bootstrap.py

@@ -7,7 +7,7 @@
 import pytest

 import spack.config
-import spack.environment
+import spack.environment as ev


Why this change?

See #25439 (comment)

I think we can take care of understanding these two failures in a following PR.

lib/spack/spack/architecture.py

alalazo · 2021-08-18T07:34:01Z

lib/spack/spack/concretize.py

@@ -63,10 +63,11 @@ def __init__(self, abstract_spec=None):
        self._adjust_target_answer_generator = None

    def concretize_develop(self, spec):
+        import spack.environment as ev


Why doing a local import rather than one at the top of the module?

Because of Python 2:

$ SPACK_PYTHON=python2 spack --version Traceback (most recent call last): File "/home/harmen/spack/bin/spack", line 96, in <module> import spack.main # noqa File "/home/harmen/spack/lib/spack/spack/main.py", line 36, in <module> import spack.cmd File "/home/harmen/spack/lib/spack/spack/cmd/__init__.py", line 24, in <module> import spack.environment as ev File "/home/harmen/spack/lib/spack/spack/environment.py", line 20, in <module> import spack.concretize File "/home/harmen/spack/lib/spack/spack/concretize.py", line 36, in <module> import spack.environment as ev AttributeError: 'module' object has no attribute 'environment'

There's a circularity between environment & concretize, but it's only an error on Python 2.7, not Python 3.x.

Actually... when I drop as ev here, Python 2 does not complain 😕.

Can you open an issue as soon as we merge this, to understand what's going on?

lib/spack/spack/package.py

haampie · 2021-08-18T08:03:12Z

Do we want to consistently:
import spack.environment as ev
everywhere?

My reason for it was that we're doing it already in 88% of the cases:

$ git checkout develop
$ grep '--include=*.py' 'import spack.environment as ev' -r lib/ | wc -l
55
$ grep '--include=*.py' 'import spack.environment' -r lib/ | wc -l
62

Edit: turns out python 2.7 struggles with alias'ed imports 6b8e412, so as ev is used everywhere except in two cases now.

Apparently no longer necessary to have local imports because of later changes

The error message "'module' object has no attribute 'environment'" is gone after using `import spack.environment` without an alias on Python 2.7, so these are the only exceptions to `as ev` everywhere...

…nment()

This reverts commit 8917731.

haampie · 2021-08-18T10:26:16Z

I think I've incorporated all the review comments now, and I've updated the PR description at the top with a summary of the changes.

alalazo

LGTM but let's wait for @becker33 review. Should we wait on #22845 and #22720 to be merged before merging this PR, given the number of files it touches?

alalazo · 2021-08-18T15:37:28Z

lib/spack/spack/cmd/install.py

@@ -324,10 +324,14 @@ def get_tests(specs):
        else:
            return False

+    # Parse cli arguments and construct a dictionary
+    # that will be passed to the package installer
+    update_kwargs_from_args(args, kwargs)


This looks much better to me now!

alalazo · 2021-08-18T15:40:54Z

lib/spack/spack/environment.py


+    def install_specs(self, specs=None, **install_args):


Definitely a better API imo without a dependency on an argparse namespace. Thanks for taking care of that!

alalazo · 2021-08-18T15:42:25Z

lib/spack/spack/test/cmd/bootstrap.py

@@ -7,7 +7,7 @@
 import pytest

 import spack.config
-import spack.environment
+import spack.environment as ev


I think we can take care of understanding these two failures in a following PR.

becker33

Mostly looks good, I agree the changes you made after Max's review are great improvements. only minor stuff left.

becker33 · 2021-08-17T20:49:43Z

lib/spack/spack/util/__init__.py

@@ -2,3 +2,19 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+
+def elide_list(line_list, max_num=10):


This probably belongs in llnl.util.lang (the same module as dedupe and star)

becker33 · 2021-08-17T20:52:25Z

lib/spack/spack/cmd/__init__.py

@@ -501,3 +486,71 @@ def extant_file(f):
    if not os.path.isfile(f):
        raise argparse.ArgumentTypeError('%s does not exist' % f)
    return f
+
+
+def require_active_env(cmd_name):


I don't fully understand why this method would be in spack.cmd... does that help with the circular import problem if you put it in spack.environment (and leave find_environment here because it uses argparse)?

It's in spack.cmd because its only purpose is to create an error that shows you how to use the command if you didn't activate an env.

becker33

I'm still not convinced on whether the methods are all in the perfect modules, but I don't think we should hold this up over it.

haampie · 2021-08-20T06:50:01Z

Thanks!

…after spack#25470

* CI for #25439 was not run on the latest merge commit, and fails after #25470 * Make it consistent

spackbot-app bot added commands environments modules tests General test capability(ies) utilities labels Aug 16, 2021

haampie force-pushed the fix/refactor-get_env branch 2 times, most recently from 16f38aa to bea4caf Compare August 16, 2021 11:56

haampie added the breaking-change label Aug 16, 2021

haampie force-pushed the fix/refactor-get_env branch from bea4caf to 57ea171 Compare August 16, 2021 12:57

haampie closed this Aug 16, 2021

haampie reopened this Aug 16, 2021

haampie closed this Aug 16, 2021

haampie reopened this Aug 16, 2021

becker33 requested changes Aug 16, 2021

View reviewed changes

haampie force-pushed the fix/refactor-get_env branch from 48f1d83 to 117579a Compare August 17, 2021 10:45

spackbot-app bot added the architecture label Aug 17, 2021

haampie force-pushed the fix/refactor-get_env branch 2 times, most recently from 45699cb to 84f57de Compare August 17, 2021 10:55

spackbot-app bot added fetching stage labels Aug 17, 2021

haampie force-pushed the fix/refactor-get_env branch 4 times, most recently from bec2625 to f9d0218 Compare August 17, 2021 15:22

haampie force-pushed the fix/refactor-get_env branch from f9d0218 to 8917731 Compare August 17, 2021 16:08

haampie requested a review from becker33 August 17, 2021 21:29

alalazo reviewed Aug 18, 2021

View reviewed changes

haampie added 5 commits August 18, 2021 10:35

Make more imports top-level (review comments)

c3f1566

Apparently no longer necessary to have local imports because of later changes

Python 2: toplevel import errors only with 'as ev'

6b8e412

The error message "'module' object has no attribute 'environment'" is gone after using `import spack.environment` without an alias on Python 2.7, so these are the only exceptions to `as ev` everywhere...

spack.environment.get_active_env() -> spack.environment.active_enviro…

b02ea7c

…nment()

Revert "Remove last dep of spack.environment on spack.cmd"

adedfec

This reverts commit 8917731.

Handle cli args -> package installer args in cmd, not in environment

8a4d6e7

haampie force-pushed the fix/refactor-get_env branch from c9f4ad2 to 8a4d6e7 Compare August 18, 2021 10:12

haampie changed the title ~~Refactor active environment getter~~ New spack.environment.active_environment api, and make spack.environment not depend on spack.cmd. Aug 18, 2021

haampie requested a review from alalazo August 18, 2021 12:41

alalazo previously approved these changes Aug 18, 2021

View reviewed changes

becker33 requested changes Aug 19, 2021

View reviewed changes

spack.util.elide_list -> llnl.util.lang.elide_list

c501dc3

haampie dismissed alalazo’s stale review via c501dc3 August 19, 2021 08:38

haampie requested review from alalazo and becker33 August 19, 2021 08:38

becker33 approved these changes Aug 20, 2021

View reviewed changes

becker33 merged commit 220a878 into spack:develop Aug 20, 2021

haampie deleted the fix/refactor-get_env branch August 20, 2021 06:50

haampie added a commit to haampie/spack that referenced this pull request Aug 20, 2021

CI for spack#25439 was not run on the latest merge commit, and fails …

b501cea

…after spack#25470

haampie mentioned this pull request Aug 20, 2021

Fix broken develop as CI didn't run on latest merge commit #25531

Merged

tgamblin pushed a commit that referenced this pull request Aug 20, 2021

Fix broken develop as CI didn't run on latest merge commit (#25531)

d52a1b8

* CI for #25439 was not run on the latest merge commit, and fails after #25470 * Make it consistent

haampie mentioned this pull request Aug 24, 2021

Fix: --overwrite backs up old install dir, but it gets destroyed anyways #25583

Merged

		@@ -613,6 +613,42 @@ def package_id(pkg):
		return "{0}-{1}-{2}".format(pkg.name, pkg.version, pkg.spec.dag_hash())


		def update_kwargs_from_args(args, kwargs):

New spack.environment.active_environment api, and make spack.environment not depend on spack.cmd. #25439

New spack.environment.active_environment api, and make spack.environment not depend on spack.cmd. #25439

Conversation

haampie commented Aug 16, 2021 • edited

haampie commented Aug 16, 2021

haampie commented Aug 16, 2021

gartung commented Aug 16, 2021 • edited

haampie commented Aug 16, 2021

gartung commented Aug 16, 2021

becker33 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie Aug 17, 2021 • edited

Choose a reason for hiding this comment

haampie Aug 17, 2021 • edited

Choose a reason for hiding this comment

haampie commented Aug 17, 2021

spackbot-app bot commented Aug 17, 2021

alalazo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie Aug 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie Aug 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie commented Aug 18, 2021 • edited

haampie commented Aug 18, 2021 • edited

alalazo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becker33 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haampie Aug 19, 2021 • edited

Choose a reason for hiding this comment

becker33 left a comment

Choose a reason for hiding this comment

haampie commented Aug 20, 2021

New `spack.environment.active_environment` api, and make spack.environment not depend on spack.cmd. #25439

New `spack.environment.active_environment` api, and make spack.environment not depend on spack.cmd. #25439

haampie commented Aug 16, 2021 •

edited

gartung commented Aug 16, 2021 •

edited

haampie Aug 17, 2021 •

edited

haampie Aug 17, 2021 •

edited

haampie Aug 18, 2021 •

edited

haampie Aug 18, 2021 •

edited

haampie commented Aug 18, 2021 •

edited

haampie commented Aug 18, 2021 •

edited

haampie Aug 19, 2021 •

edited