Allow hiding toolchains (REVIEW) #1683

geimer · 2016-03-20T18:05:37Z

Fix for #1682

Unit tests still succeed (since there is no functional change if --hide-deps is not used) and the example mentioned in #1682 works like a charm, both a dry-run and an actual build.

One or the other unit test should probably use the --hide-deps option, though I don't have a clear idea yet where this would be most effective.

Edit: After a discussion during an EB conf call (cfr. https://github.com/hpcugent/easybuild/wiki/Conference-call-notes-20160706), the approach taken has slightly changed. This PR will tackle

Hiding toolchains via --hide-toolchains=foo,bar command-line option (as well as env var and config file parameter)
Extend toolchain easyconfig parameter to handle hidden key (as outlined in Allow hiding toolchains (REVIEW) #1683 (comment))
Unit tests

To be done in a separate PR:

Handle hidden parameter in --try-toolchain option

hpcugentbot · 2016-03-20T18:05:39Z

Automatic reply from Jenkins: Can I test this?

boegel · 2016-03-20T18:11:02Z

Jenkins: ok to test

hpcugentbot · 2016-03-20T19:28:37Z

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2899/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

pforai · 2016-03-25T13:00:18Z

+1 on this!

geimer · 2016-04-04T14:43:24Z

@boegel Could you please take a quick look at this and let me know what is missing? I still don't know where to test this in the unit tests. As far as I can see, there is no test for as_dict yet and the result would only have an impact at a higher level anyway.

boegel · 2016-04-04T20:45:48Z

easybuild/tools/toolchain/toolchain.py

@@ -108,6 +108,7 @@ def __init__(self, name=None, version=None, mns=None, class_constants=None, tcde

        # toolchain instances are created before initiating build options sometimes, e.g. for --list-toolchains
        self.dry_run = build_option('extended_dry_run', default=False)
+        self.hide_deps = build_option('hide_deps', default=None)


I would use an empty list as a fallback here (not as default value for the build option, that's something else)

so:

self.hide_deps = build_option('hide_deps', default=None) or []

boegel · 2016-04-04T20:53:46Z

@geimer: there is a test for --hide-deps already, see test/framework/options.py, look for test_hide_deps; you may want to consider a dedicated new test (test_hide_toolchain or whatever)

However, I'm not sure this is the right approach here...

With the current proposed changes, a hidden toolchain can only be used if the toolchain name is listed in --hide-deps. I'm not sure if that is enough...

Shouldn't there be a more explicit way of using a hidden toolchain as well?
For example, by supporting this:

toolchain = {'name': 'foss', 'version': '2016a', 'hidden': True}

Or even through the module naming scheme somehow (maybe that's already possible though, and it doesn't need any changes in the framework, not sure).

I think we need to double check that we're covering all bases here, since there are also multiple ways in which a dependency can be listed as hidden (--hide-deps, hiddendependencies, eb --hidden), ...

geimer · 2016-04-07T07:16:08Z

@boegel: Thanks, I'll take a look at the tests in test/framework/options.py.

Shouldn't there be a more explicit way of using a hidden toolchain as well?
For example, by supporting this:
toolchain = {'name': 'foss', 'version': '2016a', 'hidden': True}

I'd like to avoid this approach, as it would require users to touch all easyconfigs that use this toolchain. What would be the advantage?

Or even through the module naming scheme somehow (maybe that's already possible though, and it doesn't need any changes in the framework, not sure).

How does the naming scheme fit into the picture???

I think we need to double check that we're covering all bases here, since there are also multiple ways in which a dependency can be listed as hidden (--hide-deps, hiddendependencies, eb --hidden), ...

eb --hidden should be no problem, as it only affects the package that is currently being built, which can't be it's own toolchain. W.r.t. --hide-deps and hiddendependencies: I thought they were affecting the same setting, i.e., hiddendependencies being the easyconfig variant of the --hide-deps command-line parameter...

boegel · 2016-04-11T07:41:27Z

@geimer: you're right about the relation between --hide-deps and hiddendependencies, which is sort of my point...

With your current approach, hiding a toolchain can only be done via --hide-deps; including it in hiddendependencies will not work (the toolchain is not a regular dependency), and there's no equivalent alternative to making a toolchain hidden in an easyconfig file.

I wouldn't like having to touch all the easyconfigs either, but it is a good way of making very sure the toolchain will always be installed hidden, regardless of the EasyBuild configuration that is in place (similar to hiddendependencies).

So, I'm wondering whether supporting with something like --hide-toolchains and hidden_toolchain = True would be better, rather than 'half' mixing dependencies and toolchains...

ocaisa · 2016-04-11T08:39:05Z

Personally, I found it painful to use hiddendependencies and have abandoned it in favour of --hide-deps (or at least the equivalent env-var which I configure for all our installers). Unless it's that something is consistently hidden for all configs coming from the main repo (and there are currently 0 like this), it's very tedious to remember...or more particularly to get everyone else to remember.

geimer · 2016-04-12T09:31:56Z

@boegel: Components such as GCCcore can be both: a toolchain (e.g., for binutils-2.25-GCCcore-4.9.3) and a dependency (e.g., for GCC-4.9.3-2.25 whose toolchain is dummy). Thus, introducing --hide-toolchains would still require to list GCCcore in --hide-deps for a robot build to do the right thing. That is, you would have to list it twice. Another source for errors. Urgh...

The more I think about it, the more I'm inclined to say that the whole hidden business is designed the wrong way around. In the end, being hidden is a property of a package. So why should all packages depending on it (either as a dependency or a toolchain) care whether it is hidden or not? They probably shouldn't -- and leave it up to the dependency resolution mechanism of --robot to select the right module. This would mean to only support the --hidden command-line option as well as a hidden = True easyconfig parameter. An environment variable interface is tricky, though, as it may only be desired to hide a module built with a certain toolchain (e.g., one may want the Bison-3.0.4 module used for bootstrapping to be hidden, whereas the Bison-3.0.4-GCCcore-4.9.3 module should stay visible).

ocaisa · 2016-04-12T09:53:22Z

I agree that supporting hidden = True in the easyconfig might be a simple solution but Kenneth argued against that when we first suggested it. Maybe he can be convinced now...

On a similar note, I ran into an issue yesterday that our back-end has no libuuid but our frontend does. If you're building on the back-end then you need to provide the dep, but then on the front-end this causes problems because basic system libs are expecting the system libuuid which has other symbols inside. The solution was actually simple since the particular builds that needed libuuid had runpath-ed it in, so I could safely leave it as a build dep so that on the front-end it resolves to the system provided version for the other tools. The massive amount of vis libs that we currently installing makes me worried about the environment exported to slurm getting too large, and I see this approach as a potential solution. We could sidestep having to load all the dependencies of (for example) libX11 at runtime by having an rpathed = True in the easyconfig

boegel · 2016-07-12T16:00:32Z

@geimer this is still WIP, right, since you're planning to add support for marking a toolchain as hidden in the easyconfig file too? if so, please update the PR title (and also mention you're adding a hidden easyconfig parameter)

Are you planning to make the robot hidden-aware in this PR too, i.e. make it able to always fall back to checking for a hidden module too? Maybe it's better to do that in a separate PR?

geimer · 2016-08-02T15:11:55Z

@boegel: I believe the parts that are already done in this PR are ready for review. That said, I got lost while investigating how things are passed from the option parsing to the easyconfig tweaking to address --try-toolchain handling... But maybe it would be better to handle this in a separate PR anyhow to keep things in bite-sized chunks? What do you think?

…on_dict + style fixes

boegel · 2016-08-04T14:34:49Z

easybuild/framework/easyconfig/easyconfig.py

@@ -1252,6 +1252,8 @@ def process_easyconfig(path, build_specs=None, validate=True, parse_only=False,
                dep = ec.toolchain.as_dict()
                _log.debug("Adding toolchain %s as dependency for app %s." % (dep, name))
                easyconfig['dependencies'].append(dep)
+                if dep['hidden']:
+                    easyconfig['hiddendependencies'].append(dep)


@geimer: I don't think this is required? I even think we should not do this, since it may have unwanted side-effects...

I'm working on another PR to your branch, where this is removed, but I figured I should double-check with you first why this was added.

@boegel: To be honest, I don't remember. I almost believe that this is a leftover from some hacking and got committed incidentally...

@boegel: Maybe to create a correct module load statement for the hidden module in the modulefile? Cross-checking it right now...

No, I checked where hiddendependencies is used, and it's not used to determine the module name to use.

I dropped this in geimer#9, I'm fairly sure it's not needed at all.

@boegel: I tried various things and couldn't reproduce a situation where is needed. So, yes, let's drop it.

…rameter

boegel · 2016-08-04T15:10:59Z

@geimer some minor style fixes and added tests for --hide-toolchains and hiding aspect of toolchain easyconfig parameter in geimer#9

add tests w.r.t. hiding toolchains (+ minor style fixes)

geimer · 2016-08-05T07:12:16Z

@boegel: Does it make sense to rename the to_name_version_dict function to something like to_toolchain_dict? As far as I can see it is really only used for toolchain specifications and it's now more than just name/version.

boegel · 2016-08-05T08:22:43Z

@geimer: sure, I think it makes sense... Although we should keep to_name_version_dict around for now as a wrapper around to_toolchain_dict, and deprecate it (using a log.deprecated call), just to be safe in case someone uses this somewhere else (e.g. in a script that hooks into the framework).

geimer · 2016-08-05T09:26:33Z

@boegel: WIP, stay tuned... Is there any way to also deprecate the NAME_VERSION_DICT constant?

geimer · 2016-08-05T09:36:51Z

@boegel Do we need to extend test_check_type_of_param_value_toolchain as well?

`NAME_VERSION_DICT` to `TOOLCHAIN_DICT` - Provide `to_name_version_dict` compatibility wrapper printing deprecation warning - Adjusted unit tests to use new function names

geimer · 2016-08-05T12:13:03Z

@boegel Please take another look at my last commit which does the renaming/deprecation. The two questions from the previous comments still hold, though.

boegel · 2016-08-05T12:58:30Z

easybuild/framework/easyconfig/types.py


    return res


+def to_name_version_dict(spec):
+    """Deprecated in favor of to_toolchain_dict."""
+    _log.deprecated("to_name_version_dict; use to_toolchain_dict instead.", '2.9')


the version number indicates when the warning will be transformed into an error, so use 3.0

@boegel: Are you sure? The output I get when modifying one of the unit tests is

DEPRECATED (since v2.9) functionality used: to_name_version_dict; use to_toolchain_dict instead.; see http://easybuild.readthedocs.org/en/latest/Deprecated-functionality.html for more information

This suggests that it is the version since when it is deprecated, no?

as soon as you include a log.deprecated message, you're deprecating something

the version indicates when the deprecation warning becomes a deprecation error, so it should be 3.0

boegel · 2016-08-05T13:05:57Z

@geimer: I wouldn't worry about deprecating NAME_VERSION_DICT, I consider that internal to the framework, and I'd be quite surprised if someone is importing that directly; so, there's no need to retain NAME_VERSION_DICT, just renaming it is fine

also enhancing test_check_type_of_param_value_toolchain is probably a good idea, yes

boegel · 2016-08-09T15:58:25Z

Going in, thanks @geimer!

boegel · 2016-08-09T15:59:24Z

Ah crap, there are conflicts with current develop that will need to be tackled first... :(

@geimer: can you look into that? If not, I'll tackle it myself with a wrapper PR later this week.

sync with develop & resolve conflicts

boegel · 2016-08-10T13:46:25Z

bootstrap test fails due to unrelated issue, cfr. #1869, so good to go

Thanks @geimer!

boegel · 2016-08-10T15:12:34Z

merged via #1871, thanks @geimer!

Allow hiding toolchains

134dee4

boegel added this to the v2.8.0 milestone Mar 20, 2016

boegel reviewed Apr 4, 2016
View reviewed changes

boegel modified the milestones: v2.8.0, v2.x May 10, 2016

geimer added 2 commits July 11, 2016 13:46

Introduce 'hide-toolchains' option rather than abusing 'hide-deps'

70a8d76

Merge branch 'develop' into hide_toolchains

55b176d

boegel modified the milestones: v2.9.0, v2.x Jul 12, 2016

geimer changed the title ~~Allow hiding toolchains~~ Allow hiding toolchains (WIP) Jul 12, 2016

geimer added 3 commits July 26, 2016 09:58

Allow 'hidden' parameter in toolchain specification

a190d99

Extend/fix unit test

903ad9b

Fix YAML unit test

3e5da31

geimer changed the title ~~Allow hiding toolchains (WIP)~~ Allow hiding toolchains (REVIEW) Aug 4, 2016

boegel added 2 commits August 4, 2016 16:26

also test whether setting hidden as False is honored in to_name_versi…

b6d9d59

…on_dict + style fixes

don't add toolchain to hiddendependencies in process_easyconfig

2a3e7e5

boegel reviewed Aug 4, 2016
View reviewed changes

add test for --hide-toolchains and hiding toolchain via easyconfig pa…

5ea09a2

…rameter

boegel mentioned this pull request Aug 4, 2016

add tests w.r.t. hiding toolchains (+ minor style fixes) geimer/easybuild-framework#9

Merged

Merge pull request #9 from boegel/hide_toolchains

9085af1

add tests w.r.t. hiding toolchains (+ minor style fixes)

boegel mentioned this pull request Aug 5, 2016

EasyBuild v2.9.0 (roadmap) easybuilders/easybuild#219

Closed

12 tasks

- Rename to_name_version_dict to to_toolchain_dict and

f5ba8ab

`NAME_VERSION_DICT` to `TOOLCHAIN_DICT` - Provide `to_name_version_dict` compatibility wrapper printing deprecation warning - Adjusted unit tests to use new function names

boegel reviewed Aug 5, 2016
View reviewed changes

geimer added 3 commits August 5, 2016 15:10

Remove NAME_VERSION_DICT

df364fa

Extend test_check_type_of_param_value_toolchain

2ab1bc3

Bump maxver of deprecation warning to '3.0'

680286a

boegel and others added 3 commits August 9, 2016 18:02

Merge branch 'develop' into hide_toolchains

3da5f0b

Merge branch 'develop' into hide_toolchains

568f719

Merge pull request #10 from boegel/hide_toolchains

c7bf20a

sync with develop & resolve conflicts

boegel mentioned this pull request Aug 10, 2016

add support for hiding toolchains #1871

Merged

boegel merged commit c7bf20a into easybuilders:develop Aug 10, 2016

boegel mentioned this pull request Aug 10, 2016

Allow hiding of toolchains #1682

Closed

geimer deleted the hide_toolchains branch August 12, 2016 10:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow hiding toolchains (REVIEW) #1683

Allow hiding toolchains (REVIEW) #1683

geimer commented Mar 20, 2016 •

edited

Loading

hpcugentbot commented Mar 20, 2016

boegel commented Mar 20, 2016

hpcugentbot commented Mar 20, 2016

pforai commented Mar 25, 2016

geimer commented Apr 4, 2016

boegel Apr 4, 2016

boegel commented Apr 4, 2016

geimer commented Apr 7, 2016

boegel commented Apr 11, 2016

ocaisa commented Apr 11, 2016

geimer commented Apr 12, 2016

ocaisa commented Apr 12, 2016

boegel commented Jul 12, 2016

geimer commented Aug 2, 2016

boegel Aug 4, 2016

geimer Aug 4, 2016

geimer Aug 4, 2016

boegel Aug 4, 2016

geimer Aug 5, 2016

boegel commented Aug 4, 2016

geimer commented Aug 5, 2016

boegel commented Aug 5, 2016

geimer commented Aug 5, 2016

geimer commented Aug 5, 2016

geimer commented Aug 5, 2016

boegel Aug 5, 2016

geimer Aug 5, 2016

boegel Aug 5, 2016

boegel commented Aug 5, 2016

boegel commented Aug 9, 2016

boegel commented Aug 9, 2016

boegel commented Aug 10, 2016

boegel commented Aug 10, 2016

Allow hiding toolchains (REVIEW) #1683

Allow hiding toolchains (REVIEW) #1683

Conversation

geimer commented Mar 20, 2016 • edited Loading

hpcugentbot commented Mar 20, 2016

boegel commented Mar 20, 2016

hpcugentbot commented Mar 20, 2016

pforai commented Mar 25, 2016

geimer commented Apr 4, 2016

Choose a reason for hiding this comment

boegel commented Apr 4, 2016

geimer commented Apr 7, 2016

boegel commented Apr 11, 2016

ocaisa commented Apr 11, 2016

geimer commented Apr 12, 2016

ocaisa commented Apr 12, 2016

boegel commented Jul 12, 2016

geimer commented Aug 2, 2016

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

boegel commented Aug 4, 2016

geimer commented Aug 5, 2016

boegel commented Aug 5, 2016

geimer commented Aug 5, 2016

geimer commented Aug 5, 2016

geimer commented Aug 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel commented Aug 5, 2016

boegel commented Aug 9, 2016

boegel commented Aug 9, 2016

boegel commented Aug 10, 2016

boegel commented Aug 10, 2016

geimer commented Mar 20, 2016 •

edited

Loading