reset keep toolchain component class 'constants' every time #1294

boegel · 2015-06-05T07:12:27Z

This fixes a problem that often occurs when testing large easyconfig PRs that contain easyconfigs with a large variety on toolchains using --from-pr, for example easybuilders/easybuild-easyconfigs#1337 (comment) .

If old and new version of Intel compilers are used in the same session, things break because constants aren't exactly constant.

This fixes the problem; I took care of checking the other toolchain components too.

@wpoely86, @pforai, @stdweird: please review?

stdweird · 2015-06-05T07:16:08Z

easybuild/toolchains/compiler/inteliccifort.py

@@ -104,6 +104,9 @@ def _set_compiler_vars(self):
            raise EasyBuildError("_set_compiler_vars: mismatch between icc version %s and ifort version %s",
                                 icc_version, ifort_version)

+        # reset LIB_MULTITHREAD every time,
+        # to avoid problems when multiple Intel compilers versions are used in a single session
+        self.LIB_MULTITHREAD = ['iomp5', 'pthread']  # iomp5 is OpenMP related


hmmmm, this is not inheritance-safe. need to think about this one

maybe

if self._init_LIB_MULTITHREAD is None: self._init_LIB_MULTITHREAD = self.LIB_MULTITHREAD[:] else: self.LIB_MULTITHREAD = self._init_LIB_MULTITHREAD[:]

and add self._init_... = None to the __init__? (or use a dict to store all such non-constant class constants)

On another note, why do we manually add libiomp5? Using -fopenmp should take care of this?

should, we had a very good reason at some point for this, and I'm not keen on breaking stuff that works just to remove a handful of characters ;)

these are libraries you can pass via LDFLAGS, doesn't always work with compiler options. there's probably some crappy code out there that wants library names.

ideally, EB should figure out what libraries -fopenmp links in and use those instead of hardcoding.

@stdweird: good point on inheritance

How about simply this in __init__.py:

self._init_LIB_MULTITHREAD = self.LIB_MULTITHREAD[:]

and this here:

self.LIB_MULTITHREAD = self._init_LIB_MULTITHREAD[:]

That should work, no?

Toolchain instances are recreated for different versions.

then maybe also make 2 private methods _copy_some_class_constants and _restore_some_class_constants and call/extend those.

I wouldn't generalise this, it should be considered an exception, not encouraged.

hpcugentbot · 2015-06-05T07:33:42Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1732/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1732/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.

hpcugentbot · 2015-06-05T07:53:40Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1733/
EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1733/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

boegel · 2015-06-05T09:18:44Z

@stdweird: fixed the inheritance issue you pointed out, please rereview?

hpcugentbot · 2015-06-05T09:39:29Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1734/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1734/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.

stdweird · 2015-06-05T13:41:39Z

easybuild/toolchains/compiler/inteliccifort.py

@@ -87,7 +87,9 @@ class IntelIccIfort(Compiler):
        'dynamic':'-Bdynamic',
    }

-    LIB_MULTITHREAD = ['iomp5', 'pthread']  ## iomp5 is OpenMP related
+    LIB_MULTITHREAD = ['iomp5', 'pthread']  # iomp5 is OpenMP related
+    # keep track of original value, needs to be restored every time since we append to class 'constant' LIB_MULTITHREAD


no, please hide this in the __init__, people will start to manipultae this

hpcugentbot · 2015-06-05T14:59:53Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1737/
EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1737/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

hpcugentbot · 2015-06-05T15:04:42Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1738/
EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1738/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

…safe

hpcugentbot · 2015-06-05T16:22:57Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1739/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1739/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.

boegel · 2015-06-05T16:25:04Z

@stdweird: central solution implemented, unit test enhanced to make sure things work as expected when multiple toolchains and toolchain versions are in play

stdweird · 2015-06-05T16:43:40Z

easybuild/toolchains/linalg/intelmkl.py

@@ -28,7 +28,7 @@
 @author: Stijn De Weirdt (Ghent University)
 @author: Kenneth Hoste (Ghent University)
 """
-
+import copy


not needed?

will remove, thx

hpcugentbot · 2015-06-05T17:11:54Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1740/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1740/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.

stdweird · 2015-06-06T10:55:32Z

easybuild/tools/toolchain/toolchain.py

@@ -95,6 +99,10 @@ def __init__(self, name=None, version=None, mns=None):

        self.vars = None

+        self.add_class_constants_to_restore([])  # make sure self.CLASS_CONSTANTS_TO_RESTORE is initialised
+        self._copy_class_constants()
+        self._restore_class_constants()


make a new method

def _init_class_constants(self, names=None): self.add_class_constants_to_restore(names or []) self._copy_class_constants() self._restore_class_constants()

because this looks so strange otherwise

stdweird · 2015-06-06T11:00:59Z

although the code looks good, it just feel weird looking at it. but my alternative with the __init__ isn't much better 😄
so feel free to modify or merge

hpcugentbot · 2015-06-08T09:27:54Z

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1743/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1743/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.

boegel · 2015-06-15T15:17:18Z

The last test report for easybuilders/easybuild-easyconfigs#1337, available at easybuilders/easybuild-easyconfigs#1337 (comment), still shows problems, so I need to look at it more closely, and see why this change isn't fixing the issues there...

boegel · 2015-07-07T06:22:28Z

retested with easybuilders/easybuild-easyconfigs#1337, works as intended and fixes the problem it intends to fix (previous failing test was due to issues with Intel license server, not due to EB itself)

Going in, thanks for the review @stdweird!

reset keep toolchain component class 'constants' every time

reset keep toolchain component class 'constants' every time

02e85d9

stdweird reviewed Jun 5, 2015
View reviewed changes

add unit test to verify fix w.r.t. toolchain constants

f532b8d

boegel added 3 commits June 5, 2015 10:28

fix modules count in modules.py test module

2cee2ad

make fix inheritance-safe

5c94dd1

fix resetting of self.SCALAPACK_LIB_MAP, enhance test

8d1cdf4

stdweird reviewed Jun 5, 2015
View reviewed changes

boegel added 2 commits June 5, 2015 16:54

implement central solution for restoring Toolchain class 'constants'

391cc62

cleanup

0671a43

don't define Toolchain class constant as empty list, not inheritance-…

d4c5fc6

…safe

stdweird reviewed Jun 5, 2015
View reviewed changes

drop unused import

77cf7f6

stdweird reviewed Jun 6, 2015
View reviewed changes

fix remarks

cf16ee2

boegel mentioned this pull request Jun 8, 2015

{bio} [intel-2014b] SOBAcl (REVIEW) easybuilders/easybuild-easyconfigs#1658

Merged

boegel mentioned this pull request Jun 23, 2015

matplotlib strange sanity check fail easybuilders/easybuild-easyconfigs#1414

Closed

boegel modified the milestone: v2.2 Jun 24, 2015

boegel added a commit that referenced this pull request Jul 7, 2015

Merge pull request #1294 from boegel/keep_class_constants_constant

f1841ab

reset keep toolchain component class 'constants' every time

boegel merged commit f1841ab into easybuilders:develop Jul 7, 2015

boegel deleted the keep_class_constants_constant branch July 7, 2015 06:22

boegel mentioned this pull request Jul 7, 2015

{devel} [all of them, yes, ALL of them] enable ncursesw in all ncurses easyconfigs (REVIEW) easybuilders/easybuild-easyconfigs#1337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reset keep toolchain component class 'constants' every time #1294

reset keep toolchain component class 'constants' every time #1294

boegel commented Jun 5, 2015

stdweird Jun 5, 2015

stdweird Jun 5, 2015

stdweird Jun 5, 2015

wpoely86 Jun 5, 2015

boegel Jun 5, 2015

stdweird Jun 5, 2015

boegel Jun 5, 2015

stdweird Jun 5, 2015

boegel Jun 5, 2015

hpcugentbot commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

boegel commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

stdweird Jun 5, 2015

hpcugentbot commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

boegel commented Jun 5, 2015

stdweird Jun 5, 2015

boegel Jun 5, 2015

hpcugentbot commented Jun 5, 2015

stdweird Jun 6, 2015

stdweird commented Jun 6, 2015

hpcugentbot commented Jun 8, 2015

boegel commented Jun 15, 2015

boegel commented Jul 7, 2015

reset keep toolchain component class 'constants' every time #1294

reset keep toolchain component class 'constants' every time #1294

Conversation

boegel commented Jun 5, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpcugentbot commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

boegel commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

Choose a reason for hiding this comment

hpcugentbot commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

hpcugentbot commented Jun 5, 2015

boegel commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpcugentbot commented Jun 5, 2015

Choose a reason for hiding this comment

stdweird commented Jun 6, 2015

hpcugentbot commented Jun 8, 2015

boegel commented Jun 15, 2015

boegel commented Jul 7, 2015