consider potential of multiple subtoolchains in get_toolchain_hierarchy #2466

bartoldeman · 2018-04-04T18:23:50Z

This PR replaces #2234, and depends on ~~#2464~~ and ~~#2465~~.
subtoolchains are now searched using a breadth-first-search, which
allows multiple subtoolchains per toolchain.

Some subtoolchains such as golf and golfc are now marked OPTIONAL
since they do not have to exist as modules, and can be skipped.

Since those optional subtoolchains cannot usually be found via a
dependency search (as toolchain easyconfigs do not typically have
subtoolchains as dependencies)
try to search for subtoolchain/version with the same version as
the parent. E.g. goolfc/2.6.10 searches for golfc/2.6.10, and
golfc/2.6.10 searches for golf/2.6.10 and gcccuda/2.6.10.

This PR replaces easybuilders#2234. subtoolchains are now searched using a breadth-first-search, which allows multiple subtoolchains per toolchain. Some subtoolchains such as golf and golfc are now marked OPTIONAL since they do not have to exist as modules, and can be skipped. Since those optional subtoolchains cannot usually be found via a dependency search (as toolchain easyconfigs do not typically have subtoolchains as dependencies) try to search for subtoolchain/version with the same version as the parent. E.g. goolfc/2.6.10 searches for golfc/2.6.10, and golfc/2.6.10 searches for golf/2.6.10 and gcccuda/2.6.10.

houndci-bot · 2018-04-04T18:24:00Z

easybuild/toolchains/golfc.py

+from easybuild.toolchains.fft.fftw import Fftw
+from easybuild.toolchains.linalg.openblas import OpenBLAS
+
+class Golfc(GccCUDA, Golf, OpenBLAS, Fftw):


expected 2 blank lines, found 1

houndci-bot · 2018-04-04T18:24:00Z

easybuild/toolchains/golfc.py

+from easybuild.toolchains.fft.fftw import Fftw
+from easybuild.toolchains.linalg.openblas import OpenBLAS
+
+class Golfc(GccCUDA, Golf, OpenBLAS, Fftw):


expected 2 blank lines, found 1

akesandgren · 2018-08-28T16:04:36Z

Now a bit of merging of conflicts is in order here.

…subtoolchains

bartoldeman · 2018-08-28T16:36:39Z

Just a merge, no conflicts.

bartoldeman · 2018-08-29T11:58:46Z

Reopening since travis still has not reported after 19 hours.

akesandgren · 2018-08-29T13:12:55Z

This looks sane to me. How much testing have you done?

bartoldeman · 2018-08-29T14:00:53Z

I've had a simpler variant (from #2234) of this in Compute Canada for ages (since May 2017), but not this exact code yet. Will do a few tests here.

bartoldeman · 2018-08-29T14:44:25Z

I did a test with this code on CP2K-iomkl now which in our setup depends on libxsmm-iimkl using this subtoolchain mechanism. It works.

I did promise in last week's conf call however to add a comment with the dependency tree for the most complex case (goolfc). So I will do that.

bartoldeman · 2018-08-29T15:03:20Z

One more thing, this approach should be applied to many other toolchains to be complete, at least the following:
foss.py
gmkl.py
gmklc.py
iimkl.py
iimklc.py
intel.py
iomkl.py
pmkl.py

should I submit a seperate PR for this fairly boring but noisy diff?

akesandgren · 2018-08-29T15:09:01Z

I think a separate PR for that would be easier to handle? @boegel ??

bartoldeman · 2018-08-29T15:51:38Z

travis now fails with File "/tmp/422121558/lib/python2.6/site-packages/easybuild_framework-3.6.3.dev0-py2.6.egg/easybuild/tools/job/gc3pie.py", line 94, in _check_version version_str = gc3libs.core.__version__ AttributeError: 'module' object has no attribute '__version__'
which looks unrelated.

bartoldeman · 2018-08-29T16:11:00Z

Related to Travis issue:
#2204

boegel · 2018-08-29T18:46:18Z

travis now fails with File "/tmp/422121558/lib/python2.6/site-packages/easybuild_framework-3.6.3.dev0-py2.6.egg/easybuild/tools/job/gc3pie.py", line 94, in _check_version version_str = gc3libs.core.__version__ AttributeError: 'module' object has no attribute '__version__'
which looks unrelated.

@riccardomurri Can this somehow be caused by the GC3Pie release? The latest version of GC3Pie gets installed automagically, it seems like 2.5.0 includes a backwards incompatible change?!

riccardomurri · 2018-08-29T19:42:15Z

The __version__ fields were removed from the GC3Pie source files, to keep the release version in one place only (setup.py).

The pkg_resources API should be used to check the package version -- I'll add to the GC3Pie 2.5.0 compatibility PR.

boegel · 2018-08-29T19:55:36Z

@riccardomurri Well, ok, I can see the motivation, but this is a backward-incompatible change, so anyone who will be trying to use an existing EasyBuild release with GC3Pie 2.5.0 will run into this... :)

Very good reason to get #2474 ready & merged to be included in EasyBuild v3.7.0 ASAP... :)

boegel

@bartoldeman Do you mind adding a dedicated test for det_subtoolchain_version? In test/framework/easyconfig.py would make sense, but put it wherever it's most convenient (get_toolchain_hierarchy is tested in test/framework/robot.py)

boegel · 2018-08-31T19:29:50Z

easybuild/framework/easyconfig/easyconfig.py

+        # skip dummy if add_dummy_to_minimal_toolchains is not set
+        return None
+
+    return {'name': subtoolchain_name, 'version': subtoolchain_version}


@bartoldeman I would just return subtoolchain_version here? I don't really see the point in returning a dict value here...

I can't because of the change of name from "GCCcore" to "dummy" for legacy toolchains. I did not want to remove that code.

I suspect the idea is that older toolchains had GCC directly above dummy, but the gcc toolchain has GCCcore as the subtoolchain which does not exist in those situations.

Ah, OK, I figured there must have been a good reason.

Then: i) mention that in a comment, explain why you need to return the subtoolchain name too, ii) just make this a tuple (subtoolchain_name, subtoolchain_version).

There are some ways around this of course, but e.g. moving the GCCcore logic back to the main function needs another check for dummy there too then, which is awkward...

No, it's fine like this, just needs a comment since it doesn't make sense unless you take the whole function into account.

In the end I found a more elegant way by declaring the gcccore toolchain as optional. It still needs some special logic in the main loop, but det_subtoolchain_version does exactly that now and nothing more.

Next will be tests for that but probably on Monday :)

Only read this after your earlier comment, looks good to me in its current state, it doesn't hurt that the fallback to dummy for GCCcore stands out a bit.

boegel · 2018-08-31T19:31:37Z

easybuild/framework/easyconfig/easyconfig.py

-        subtoolchain_name, subtoolchain_version = subtoolchains[current_tc_name], None
-        toolchain_hierarchy.insert(0, {'name': current_tc_name, 'version': current_tc_version})
+        for subtoolchain_name in subtoolchain_names:
+            tc = det_subtoolchain_version(current_tc, subtoolchain_name, optional_toolchains, cands)


If you change the return type of det_subtoolchain_version, I'd go with subtc_ver (but I guess subtoolchain_version is better for consistency with subtoolchain_name)

With the above in mind:

subtc_name, subtc_ver = det_subtoolchain_version(current_tc, subtoolchain_name, optional_toolchains, cands)

That won't work since the function can also return None.
I tried to implement your suggestion but I don't think it looks any better:

@@ -185,7 +185,7 @@ def det_subtoolchain_version(current_tc, subtoolchain_name, optional_toolchains, # skip dummy if add_dummy_to_minimal_toolchains is not set return None - return {'name': subtoolchain_name, 'version': subtoolchain_version} + return subtoolchain_name, subtoolchain_version @toolchain_hierarchy_cache @@ -278,12 +278,15 @@ def get_toolchain_hierarchy(parent_toolchain): cands = [c for c in cands if c['name'] in subtoolchain_names] for subtoolchain_name in subtoolchain_names: - tc = det_subtoolchain_version(current_tc, subtoolchain_name, optional_toolchains, cands) + subtc = det_subtoolchain_version(current_tc, subtoolchain_name, optional_toolchains, cands) # add to hierarchy and move to next - if tc is not None and tc['name'] not in visited: - toolchain_hierarchy.insert(0, tc) - bfs_queue.insert(0, tc) - visited.add(tc['name']) + if subtc is not None: + subtc_name, subtc_ver = subtc + if subtc_name not in visited: + tc = {'name': subtc_name, 'version': subtc_ver} + toolchain_hierarchy.insert(0, tc) + bfs_queue.insert(0, tc) + visited.add(subtc_name) _log.info("Found toolchain hierarchy for toolchain %s: %s", parent_toolchain, toolchain_hierarchy) return toolchain_hierarchy

unless I am missing something?

Ah, I overlooked the fact that you need tc as a dict anyway... You're right, keep it as is. :)

boegel · 2018-08-31T19:32:13Z

easybuild/framework/easyconfig/easyconfig.py

+        for subtoolchain_name in subtoolchain_names:
+            tc = det_subtoolchain_version(current_tc, subtoolchain_name, optional_toolchains, cands)
+            # add to hierarchy and move to next
+            if tc is not None and tc['name'] not in visited:


It's the tc['name'] that makes this a bit awkward, just use subtoolchain_name?

Legacy where gcc/iccifort/etc have dummy directly as subtoolchain, and GCCcore does not exist.

boegel · 2018-09-01T07:24:27Z

@bartoldeman Gave this another review, looks good, just missing the test for det_subtoolchain_version...

akesandgren · 2018-09-03T10:36:34Z

The OPTIONAL flag in the toolchain definitions, that's used in a toolchain that is optional, not in toolchains that have optional sub-toolchains?
Was trying to look at the toolchain changes and want to make sure...

bartoldeman · 2018-09-03T14:32:01Z

It is the toolchain itself that is optional. However I hit an issue by marking gcccore optional it marked almost all others optional too through subclassing...will correct tomorrow. Today we are all off in Canada (labour day)

By allowing both GCCcore and dummy as allowed subtoolchains for compilers we no longer need the special logic in easyconfig.py. However I did need to unconditionally return '' as version for the dummy toolchain to avoid inconsistencies with 'dummy','dummy', which resolves to the same dependency anyway.

…None.

This is now done in the candidate search instead of det_toolchain_version, and applies to all subtoolchains that are composite, so they can have the same version as the parent.

houndci-bot · 2018-09-04T14:59:51Z

test/framework/easyconfig.py

@@ -2078,6 +2080,52 @@ def test_resolve_template(self):
        # '%(name)' is not a correct template spec (missing trailing 's')
        self.assertEqual(resolve_template('%(name)', tmpl_dict), '%(name)')

+
+    def test_det_subtoolchain_version(self):


too many blank lines (2)

bartoldeman · 2018-09-04T16:19:35Z

ok, I have been able to simplify the code quite a bit by giving the gcc, iccifort and pgi toolchains multiple subtoolchains (GCCcore and dummy). This way the data can do the talking which looks much more elegant than special case code.

Test case added for det_subtoolchain_version too.

boegel · 2018-09-04T17:45:40Z

easybuild/toolchains/gcc.py


 class GccToolchain(GCCcore):
    """Simple toolchain with just the GCC compilers."""
    NAME = 'GCC'
    COMPILER_MODULE_NAME = [NAME]
-    SUBTOOLCHAIN = GCCcore.NAME
+    SUBTOOLCHAIN = [GCCcore.NAME, DUMMY_TOOLCHAIN_NAME]


@bartoldeman How does this relate to --add-dummy-to-minimal-toolchains?

dummy should only be considered a subtoolchain when --add-dummy-to-minimal-toolchains is enabled?

Actually, it's probably OK, since dummy was already a subtoolchain for GCCcore (see below)..

exactly. the bfs tree has dummy twice here:

gcc / \ gcccore dummy | dummy

which actually corresponds to pre-existing behaviour since the legacy gcc toolchains do not have gcccore. det_toolchain_version then explicitly filters out dummy if --add-dummy-to-minimal-toolchains is not set.

hmm, should adjust the ascii-art diagram comment then!

Yes, indeed :)

It's basically this:

gcc / \ / \ gcccore / \ / \ / dummy

(I need more practice in this ASCII art thing, ugh...)

boegel · 2018-09-04T17:56:38Z

easybuild/toolchains/gcccore.py

@@ -37,3 +37,4 @@ class GCCcore(Gcc):
    # Replace the default compiler module name with our own
    COMPILER_MODULE_NAME = [NAME]
    SUBTOOLCHAIN = DUMMY_TOOLCHAIN_NAME
+    OPTIONAL = True


@bartoldeman Can you clarify why GCCcore is optional?

It's the base of every toolchain, or am I overlooking something?

it's not the base of the older toolchains with GCC 4.8 / intel 2015.2 and older.

Right, I overlooked that...

Please clarify with a comment above:

# GCCcore is only guaranteed to be present in recent toolchains # for old versions of some toolchains (GCC, intel) it is not part of the hierarchy and hence optional

akesandgren · 2018-09-04T18:55:31Z

Apart from the missing toolchains (esp intelcuda and friends), this looks good.

@akesandgren

Missed those, thanks @akesandgren.

boegel · 2018-09-04T19:14:54Z

@akesandgren Isn't intelcuda handled via iimklc.py which is updated here?

boegel · 2018-09-04T19:16:45Z

nvm, changes in 345c515 make sense...

Anything else @akesandgren?

bartoldeman · 2018-09-04T19:19:07Z

No @boegel, @akesandgren is right. I missed intelcuda and intel-para as well.

Some others could have multiple subtoolchains except they do not exist yet. I skipped them for now:
cgmpolf/cgmvolf/cgoolf => cgolf (all Clang based toolchain easyconfigs are archived)
gmacml => gacml (not done yet, also ACML looks no longer current)
goalf => galf (not done yet, atlas is also mostly archived)
goblf => gblf (not done yet, on my plate for later)
iqacml => iacml (see above; QLogic MPI is also pretty dead)

akesandgren · 2018-09-04T19:33:27Z

I'm a bit tired so this is probably wrong^3, but why doesn't iccifort have both icc and ifort listed as subtoolchains

boegel · 2018-09-04T19:50:26Z

@akesandgren Neither icc nor ifort are toolchains by themselves currently. We can consider refactoring that, but that should be done in a separate PR.

akesandgren · 2018-09-05T08:31:19Z

Going in, thanks @bartoldeman

ocaisa · 2018-09-21T12:46:31Z

easybuild/framework/easyconfig/easyconfig.py

+    elif len(uniq_subtc_versions) == 0:
+        if subtoolchain_name not in optional_toolchains:
+            # raise error if the subtoolchain considered now is not optional
+            raise EasyBuildError("No version found for subtoolchain %s in dependencies of %s",


@bartoldeman I'm hitting this error with our intel-para toolchain but I don't understand why since Iimkl is an optional toolchain

== 2018-09-21 14:42:03,912 build_log.py:158 ERROR EasyBuild crashed with an error (at ?:124 in __init__): No version found for subtoolchain <class 'easybuild.toolchains.iimkl.Iimkl'> in dependencies of intel-para (at easybuild/framework/easyconfig/easyconfig.py:162 in det_subtoolchain_version)

Never mind PR on the way

ocaisa · 2018-09-21T13:00:51Z

easybuild/framework/easyconfig/easyconfig.py

+    elif len(uniq_subtc_versions) == 0:
+        if subtoolchain_name not in optional_toolchains:
+            # raise error if the subtoolchain considered now is not optional
+            raise EasyBuildError("No version found for subtoolchain %s in dependencies of %s",


Never mind PR on the way

easybuild/toolchains/intel-para.py

bartoldeman added 2 commits April 4, 2018 18:02

Merge branch 'goolf-1.4.10-openblas-gcc' into multiple_subtoolchains

905862e

houndci-bot reviewed Apr 4, 2018

View reviewed changes

bartoldeman added 5 commits April 4, 2018 18:25

Add blank line to make houndci-bot happy.

8504be1

Merge branch 'goolfc-2.6.10' into multiple_subtoolchains

ef57c52

Adjust options test to OpenBLAS gompi to GCC change.

631574c

Adjust OpenBLAS in v2.0 and yeb test goolf toolchain easyconfigs.

a585b76

Fix test.framework.easyconfigparser test_v20_deps for OpenBLAS/gcc

4eeaef3

boegel added the enhancement label Aug 22, 2018

boegel added this to the 3.7.0 milestone Aug 27, 2018

easybuilders deleted a comment from boegelbot Aug 27, 2018

Merge remote-tracking branch 'github_hpcugent/develop' into multiple_…

039bb69

…subtoolchains

bartoldeman closed this Aug 29, 2018

bartoldeman reopened this Aug 29, 2018

boegel changed the title ~~Multiple subtoolchains~~ consider potential of multiple subtoolchains in get_toolchain_hierarchy Aug 29, 2018

Add comment with example hierarchy.

3dc630d

boegel requested changes Aug 31, 2018

View reviewed changes

Make GCCcore toolchain optional to more elegantly deal with legacy.

1d88c92

Legacy where gcc/iccifort/etc have dummy directly as subtoolchain, and GCCcore does not exist.

bartoldeman added 4 commits September 4, 2018 13:54

Simplify det_subtoolchain_version logic by initializing to result to …

9773fb5

…None.

Replace logic that finds golf/2018a as subtoolchain of goolf/2018a.

247530e

This is now done in the candidate search instead of det_toolchain_version, and applies to all subtoolchains that are composite, so they can have the same version as the parent.

Add tests for test_det_subtoolchain_version.

1866e0f

houndci-bot reviewed Sep 4, 2018

View reviewed changes

Remove blank lines (hound-ci)

237df6c

boegel reviewed Sep 4, 2018

View reviewed changes

Updated comments following review from @boegel

e6fb647

boegel previously approved these changes Sep 4, 2018

View reviewed changes

Added multiple subtoolchains to intel-para and intelcuda.

345c515

Missed those, thanks @akesandgren.

bartoldeman dismissed boegel’s stale review via 345c515 September 4, 2018 19:15

boegel approved these changes Sep 4, 2018

View reviewed changes

akesandgren merged commit 811f3a3 into easybuilders:develop Sep 5, 2018

ocaisa reviewed Sep 21, 2018

View reviewed changes

ocaisa mentioned this pull request Sep 21, 2018

fix SUBTOOLCHAINS spec for intel-para toolchain #2585

Merged

consider potential of multiple subtoolchains in get_toolchain_hierarchy #2466

consider potential of multiple subtoolchains in get_toolchain_hierarchy #2466

Conversation

bartoldeman commented Apr 4, 2018 • edited by boegel Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akesandgren commented Aug 28, 2018

bartoldeman commented Aug 28, 2018

bartoldeman commented Aug 29, 2018

akesandgren commented Aug 29, 2018

bartoldeman commented Aug 29, 2018

bartoldeman commented Aug 29, 2018

bartoldeman commented Aug 29, 2018

akesandgren commented Aug 29, 2018

bartoldeman commented Aug 29, 2018

bartoldeman commented Aug 29, 2018

boegel commented Aug 29, 2018

riccardomurri commented Aug 29, 2018

boegel commented Aug 29, 2018

boegel 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

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

boegel commented Sep 1, 2018

akesandgren commented Sep 3, 2018

bartoldeman commented Sep 3, 2018

Choose a reason for hiding this comment

bartoldeman commented Sep 4, 2018

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 Sep 4, 2018 • edited Loading

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

akesandgren commented Sep 4, 2018

boegel commented Sep 4, 2018

boegel commented Sep 4, 2018

bartoldeman commented Sep 4, 2018

akesandgren commented Sep 4, 2018

boegel commented Sep 4, 2018

akesandgren commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoldeman commented Apr 4, 2018 •

edited by boegel

Loading

boegel Sep 4, 2018 •

edited

Loading