fix module load behaviour [WIP] [donotmerge] by healther · Pull Request #3827 · spack/spack

healther · 2017-04-13T18:10:37Z

Partially due to my tests for #3761, but also regularly when interacting with spack I become annoyed, that it doesn't complain enough. If I ask it to load a package which is installed in multiple versions. spack's behaviour is to just load ALL of them. Even worse, in the case of 0 matching packages it silently does nothing.

This PR adds a check in a module loads call whether exactly one spec is found in the db. I'm sure that it is not the most elegant way, but it works for me . The error message (well the dying message) should distinguish between having found n>1 or n=0 matching packages (which it doesn't do at the moment).
This here is mostly to ask the questions, whether a) this feature is wanted, b) this is the correct wau of introducing the logic and c) what the scope of the error message should be.

I know that I broke two tests in lib/spack/spack/test/cmd/module.py, but I don't know how I have to read the output of spack test. So any help/feedback is appreciated.

check in a module loads call whether exactly one spec is found in the db

healther · 2017-04-13T20:15:08Z

After some more digging I found Database.query() and Database.query_one() in spack/database.py. So at some point someone basically put everything needed for sane loading behaviour there. Leading to the question, is there a reason for not making .query_one() a parameter in .query()

See the latest commit for my latest attempt. I'm not too happy with the error messages, ideally spack would return the available options (i.e. installed packages) or in case of missing packages with similar (installed) package names, but I don't know how I would find the information on this point (or propagate the information back out)

edit: I'm not sure about the failing travis job for python2.7, locally spack test passes for both 2.7.12 and 3.5.2, although I'm not sure what exactly happens when calling spack test as the output suggest the use of 2.7.12 even if the local python is supposed to be 3.5.2...

$ python --version
Python 3.5.2
$ spack test
============================================================================ test session starts =============================================================================
platform darwin -- Python 2.7.12, pytest-3.0.5, py-1.4.32, pluggy-0.4.0

adamjstewart · 2017-04-13T21:01:45Z

lib/spack/spack/database.py

            if unique:
                if len(results) > 1:
-                    tty.die('Found more than one spec for {}'.format(query_spec))
+                    tty.die('Found more than one spec for ' + str(query_spec))


'{}'.format() only works for Python 2.7+. You have to use '{0}'.format() in Python 2.6+.

healther · 2017-04-13T21:49:23Z

I'd propose to get rid of .query_one() completely, but that broke some tests. I'll take another shot at it, if it is wanted.

healther · 2017-04-18T17:51:55Z

Ping

adamjstewart · 2017-04-18T17:53:18Z

@alalazo Can you take a look at this? You know a lot more about our module support than I do.

healther · 2017-05-02T10:35:47Z

@alalazo did you find time to take a look at this?

alalazo · 2017-05-02T11:11:19Z

@healther No sorry, but will try to have a look later in the day.

alalazo · 2017-05-02T14:20:39Z

lib/spack/spack/database.py

        """
-        concrete_specs = self.query(query_spec, known, installed)
+        concrete_specs = self.query(query_spec, known, installed, unique=True)
        assert len(concrete_specs) <= 1


Do you agree that we will never hit this line:

assert len(concrete_specs) <= 1

because if unique=True and len(results) == 0 the program will tty.die?

I think we shouldn't change anything in the DB API, but rather act in cmd/module.py

I'm not particularly attached to this solution, it's only the first one that did what I wanted. That being said, I find the concept of query_one strange, imho it should just be a parameter.

Nevertheless, if we can accomplish the load-fix in module.py I'm all for it. Shall I try there again? Discussions here suggested that, a check of specs = args.specs(**query_args) might be sufficient.
I'll try that later (or tomorrow). I haven't done it so far, as I wanted some Feedback on how you want this PR done first.

edit: Yes, that line cannot be reached

I put @tgamblin as a reviewer because skimming the code query_one seems to be used only in tests. In that case it may be fine to erase it completely, and have a unique argument to query.

The only comment that would remain from my side is that I wouldn't use tty directly in the database module, but raise an appropriate error. The command should then catch it and react with tty.die.

alalazo

The two tests LGTM, but I would rather modify the code in cmd/module.py to use the existing DB API. Overall I think the behavior is the one the users will expect if they load modules using spack load.

healther · 2017-05-02T18:21:30Z

Turns out there is a much shorter solution on the level of cmd/module.py. All tests pass locally, let's see what Travis has to say.

I still think it would be worthwhile removing the query_one-function, but it's not needed here.

healther · 2017-05-03T09:08:47Z

Close and reopen to trigger Travis

edit: Wasn't there the possibility to look at the individual Travis tests? Did you remove that or am I just failing to find it?

alalazo · 2017-05-03T09:19:55Z

Wasn't there the possibility to look at the individual Travis tests? Did you remove that or am I just failing to find it?

You can click on "Details" next to the Travis icon and then check the logs there.

healther · 2017-05-03T19:26:26Z

thanks, somehow I failed to find that. All checks passed -> please let me know if you have improvements, otherwise this is ready from my side

alalazo

I think this change will break spack module refresh and spack module rm. On the OP side the fact that we don't have unit tests signaling the issue.

alalazo · 2017-05-04T06:13:42Z

lib/spack/spack/cmd/module.py

    }
    query_args = constraint_qualifiers.get(args.subparser_name, {})
    specs = args.specs(**query_args)
+    if len(specs) > 1:


@healther This is in the module command. I didn't try it, but it seems it will prevent the regeneration of multiple modules at once. What happens if you try:

$ spack module refresh -m tcl

using this branch?

healther · 2017-05-04T09:21:49Z

You are right, that must have been the reason for the convoluted solution I had...
I now just filter the results if the command is loads, but I'm not sure if this a valid way. I'll try to add a test for the refresh and rm command later today, but I'm not too sure, that I know the intended behaviour well enough.

citibeth · 2017-05-04T17:36:41Z

How is spack find broken?

Because it is non-determinstic. Same with spack load. You install package a and spack load a works fine. Later on, you install another version of a and spack load a breaks. The ways you have to be more specific depend on what else was installed, in an uncontrolled environment, which is fundamentally broken.

Also, I don't know if I've heard anything about "Spack Environments". What is it?

See (or better yet, attend) today's Telcon.

healther · 2017-05-04T17:38:38Z

It breaks exactly because it just loads everything that matches, rather than complain when it finds not exactly one match.

adamjstewart · 2017-05-04T17:39:30Z

@citibeth I wouldn't describe that as "broken". spack find a returns all installed packages. If you install a second copy of a, spack find a will now display both installations. Seems like the correct behavior to me.

I can't speak for spack load as I don't use it. But much like spack uninstall asks you to be more specific if there are multiple installations present, I imagine that spack load should too. Or just use the default module like Lmod does.

citibeth · 2017-05-04T17:59:17Z

It breaks exactly because it just loads everything that matches, rather than complain when it finds not exactly one match.

I agree, complaining instead of just loading everything (this PR) is an improvement and should be merged. But it doesn't fix the fundamental problem that the command is not scriptable.

adamjstewart · 2017-05-04T18:43:33Z

But it doesn't fix the fundamental problem that the command is not scriptable.

It's scriptable as long as you provide a hash. I'm not sure if there will ever be an alternative to that and I don't know enough about "Spack Environments" to tell if that will help. Personally I would use module load instead of spack load as it is scriptable and will load the highest numerically sorted module file only.

healther · 2017-05-04T22:00:19Z

Okay, so from my side this is good to go. @alalazo you still have a change request outstanding.

healther · 2017-05-06T15:29:23Z

@tgamblin @alalazo can we merge this?

alalazo

The logic now LGTM. I suggested a few improvements below, but I'll leave to you if you want to implement them. If not I can also submit a PR after this is merged.

alalazo · 2017-05-06T16:20:47Z

lib/spack/spack/test/cmd/module.py


+def test_load_multi_install_package(database, parser, capfd):
+    args = parser.parse_args(['loads', 'mpileaks'])
+    # with pytest.raises(SystemExit):


Why don't you use pytest.raises? Personally I don't see the need to check what is in the error message (which may be changed later).

alalazo · 2017-05-06T16:21:16Z

lib/spack/spack/test/cmd/module.py

+def test_load_non_existing_package(database, parser, capfd):
+    args = parser.parse_args(['loads', 'py-pudb'])
+    try:
+        module.module(parser, args)


Same as above.

alalazo · 2017-05-06T16:26:46Z

lib/spack/spack/cmd/module.py

+        if len(specs) > 1:
+            message = ("Trying to load multiple modules, please be more "
+                       "specific. Currently we would load: \n\t" +
+                       "\n\t".join(str(s) for s in specs) + "\n")


An improvement here would be to use spack.cmd.display_specs to display specs, as it will be consistent with all the other commands. I don't think we say 'Aborting.' anywhere else, so I would also remove that.

I added it specifically, so that the user (i.e. me) can be sure, that spack didn't actually change anything in the environment, I can remove it if you want, but since spack, at least right now, has the reputation of being overeager (at least at our lab), I'd prefer to be specific.

adamjstewart · 2017-05-06T17:00:33Z

I haven't been closely following this PR, but I wonder whether we should make spack load behave more like Module commands. I've only looked at Lmod, but when you module load something, if there are multiple modules that match, it loads the newest version. I think we all agree that we shouldn't load all modules that match, but instead of failing with an error message it might be better to load what Lmod would load. That would make this command more reliable in scripts.

P.S. I don't use spack load so I don't care that much. Just a suggestion.

alalazo · 2017-05-06T17:06:53Z

@adamjstewart I agree with you, here though we are modifying the behavior of spack module loads not of spack load.

adamjstewart · 2017-05-06T17:10:32Z

Oh boy, there are too many commands... What's the difference?

healther · 2017-05-06T17:13:50Z

The problem with spack load is that it is actually only a bash function defined in setup-env.sh which parses the output of
spack $_sp_flags module loads --input-only $_sp_subcommand_args --module-type dotkit
this also prevents the use of spack.cmd.display_specs as it directly writes to stdout instead of returning the string with the specs.

I'd prefer to leave the tests as is, as any change should be intentional, in which case it is trivial to adapt the test(s). I ran into this myself, because I had forgotten how I had written the test after less than 3 weeks.

healther · 2017-05-10T08:04:43Z

WIP pending decision regarding the intended behaviour of spack load see #4189.

citibeth · 2017-05-10T11:56:35Z

Mark down one more fundamentally broken thing that Spack environment work will likely improve,

…

On May 10, 2017 4:04 AM, "healther" ***@***.***> wrote: WIP pending decision regarding the intended behaviour of spack load see #4189 <#4189>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3827 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd5ySHx9gxuvYyD5NASd0GALPy5Koks5r4W-dgaJpZM4M9BtP> .

ifelsefi · 2017-05-24T15:44:01Z

We use environment modules so I am very excited to see this fixed. "spack load r" should not load all three versions of R which I have installed.

I don't know if this happens to be related but when users do "spack load blah," if blah happens to not be installed, the user gets no message stating that's the case.

It would also be nice if "spack info" would report if whether the package happens to be installed already.

fixes spack#2215 fixes spack#2570 fixes spack#6676 fixes spack#7281 closes spack#3827 This PR reverts the use of `spack module loads` in favor of `spack module find` when loading module files via Spack. After this PR `spack load` will accept a single spec at a time, and will be able to interpret correctly the `--dependencies` option.

fixes #2215 fixes #2570 fixes #6676 fixes #7281 closes #3827 This PR reverts the use of `spack module loads` in favor of `spack module find` when loading module files via Spack. After this PR `spack load` will accept a single spec at a time, and will be able to interpret correctly the `--dependencies` option.

healther added 3 commits April 13, 2017 19:56

fix module load behaviour

395852f

check in a module loads call whether exactly one spec is found in the db

fix failing tests

da3ead6

implement unique on db-query level

0ce6afb

healther added 2 commits April 13, 2017 22:44

fix formatting error

53b3aa8

flake 8 fixes

5a3c5cc

adamjstewart reviewed Apr 13, 2017

View reviewed changes

add test cases

c11cbae

adamjstewart added the modules label Apr 14, 2017

adamjstewart assigned alalazo Apr 18, 2017

alalazo reviewed May 2, 2017

View reviewed changes

alalazo requested changes May 2, 2017

View reviewed changes

alalazo requested a review from tgamblin May 2, 2017 14:24

moved number check to module.py

b1677ea

healther closed this May 3, 2017

healther reopened this May 3, 2017

alalazo requested changes May 4, 2017

View reviewed changes

alalazo reviewed May 4, 2017

View reviewed changes

restrict check to module loads command

8858b00

fix test for loads

3a6c6d1

alalazo approved these changes May 6, 2017

View reviewed changes

remove commented line

6d543d5

healther mentioned this pull request May 10, 2017

Question: Dependency resolution #4189

Closed

healther changed the title ~~fix module load behaviour~~ fix module load behaviour [WIP] [donotmerge] May 10, 2017

adamjstewart mentioned this pull request May 24, 2017

Set Default Package Version To Be Loaded #4336

Closed

healther mentioned this pull request Dec 13, 2017

'spack load' loads more than one module for the same package #6676

Closed

healther mentioned this pull request Feb 19, 2018

spack load return code #7281

Closed

tgamblin added the WIP label Mar 10, 2018

healther mentioned this pull request Jul 9, 2018

spack load should throw when input is wrong #2570

Closed

alalazo mentioned this pull request Jul 10, 2018

Split 'module' into multiple commands #7098

Merged

9 tasks

tgamblin closed this in #7098 Jul 24, 2018

Conversation

healther commented Apr 13, 2017 • edited by adamjstewart Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

healther commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

healther commented Apr 13, 2017

Uh oh!

healther commented Apr 18, 2017

Uh oh!

adamjstewart commented Apr 18, 2017

Uh oh!

healther commented May 2, 2017

Uh oh!

alalazo commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

healther May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

healther commented May 2, 2017

Uh oh!

healther commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo commented May 3, 2017

Uh oh!

healther commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

healther commented May 4, 2017

Uh oh!

citibeth commented May 4, 2017

Uh oh!

healther commented May 4, 2017

Uh oh!

adamjstewart commented May 4, 2017

Uh oh!

citibeth commented May 4, 2017

Uh oh!

adamjstewart commented May 4, 2017

Uh oh!

healther commented May 4, 2017

Uh oh!

healther commented May 6, 2017

Uh oh!

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

adamjstewart commented May 6, 2017

Uh oh!

alalazo commented May 6, 2017

Uh oh!

adamjstewart commented May 6, 2017

Uh oh!

healther commented May 6, 2017

healther commented Apr 13, 2017 •

edited by adamjstewart

Loading

healther commented Apr 13, 2017 •

edited

Loading

alalazo commented May 2, 2017 •

edited

Loading

healther May 2, 2017 •

edited

Loading

healther commented May 3, 2017 •

edited

Loading

healther commented May 3, 2017 •

edited

Loading

ifelsefi commented May 24, 2017 •

edited

Loading