optarch can now be specified on a toolchain basis by damianam · Pull Request #2071 · easybuilders/easybuild-framework

damianam · 2016-12-29T16:53:20Z

This PR enables EB to accept a more powerful --optarch syntax. Valid options are:

--optarch=someflag -> Same as now
--optarch=GENERIC -> Same as now
--optarch=Intel:xCORE-AVX512,GCC:GENERIC -> Tells Intel based toolchains to use -xCORE-AVX512 as a flag, and GCC based toolchains to use a generic set of flags (as specified in the toolchain definition)
--optarch=Intel:,GCC:someflag -> Tells Intel based toolchains to have the default behaviour (currently -xHost), and GCC based toolchains to use -someflag
--optarch=Intel,GCC:someflag -> Same as the previous case
--optarch=Intel:xCORE-AVX512,GCC:GENERIC,Intel:xHost -> Tells Intel based toolchains to use -xCORE-AVX512. The second case is ignored.

This allows to install packages on multiple toolchains using the correct --optarch option in a single command, or to have a single definition of EASYBUILD_OPTARCH that is correct regardless of the toolchain used.

A test is pending.

…tarch=Intel:xHost,GCC:GENERIC

boegel · 2016-12-29T17:02:56Z

@damianam A couple of quick thoughts:

The 'list of strings' aspect should be handled by making the optarch option of type strlist in tools/options.py
What's the point of specifying that the default flags should be used for a particular toolchain? If the toolchain is not listed, that would/should be the default behaviour, so there's no point in listing those? Also, how would you then specify not using any 'optarch' compiler options at all?
The unit tests should be updated too (see test_*_optarch in test/framework/toolchain.py)
Are you up for also updating the documentation (http://easybuild.readthedocs.io/en/latest/Controlling_compiler_optimization_flags.html), via a PR to https://github.com/hpcugent/easybuild/blob/master/docs/Controlling_compiler_optimization_flags.rst ?

damianam · 2016-12-29T17:11:42Z

I'll look into the strlist suggestion
I'll fix the --optarch=Intel:'' option. I didn't noticed that you could force it to don't add any flag.
My mindset regarding listing a toolchain but without any particular option is to simply make it explicit that no special option is wanted for that toolchain.
The tests are pending as I said in the PR ;-P. I wanted to get some feedback first, and get it out before taking a look at the tests
No problem with the documentation, I can update it once this gets merged.

damianam · 2016-12-29T17:21:46Z

Now --optarch=Intel: and --optarch=Intel:'' effectively disable any optarch flag. --optarch=Intel does nothing.

boegel · 2016-12-29T17:38:20Z

@damianam but the behaviour of using Intel: changes when combined with GCC:foo?

damianam · 2016-12-29T18:03:08Z

@boegel no, it behaves the same:

eb darshan-runtime-3.1.1-ipsmpi-2016b.eb darshan-runtime-3.1.1-gpsmpi-2016b.eb --extended-dry-run  --optarch=Intel: | grep "export CFLAGS"
  export CFLAGS="-O2 -ftz -fp-speculation=safe -fp-model source"
  export CFLAGS="-O2 -march=native"

eb darshan-runtime-3.1.1-ipsmpi-2016b.eb darshan-runtime-3.1.1-gpsmpi-2016b.eb --extended-dry-run  --optarch=Intel:,GCC:GENERIC | grep "export CFLAGS"
  export CFLAGS="-O2 -ftz -fp-speculation=safe -fp-model source"
  export CFLAGS="-O2 -march=x86-64 -mtune=generic"

eb darshan-runtime-3.1.1-ipsmpi-2016b.eb darshan-runtime-3.1.1-gpsmpi-2016b.eb --extended-dry-run  --optarch=Intel,GCC:GENERIC | grep "export CFLAGS"
  export CFLAGS="-O2 -xHost -ftz -fp-speculation=safe -fp-model source"
  export CFLAGS="-O2 -march=x86-64 -mtune=generic"

eb darshan-runtime-3.1.1-ipsmpi-2016b.eb darshan-runtime-3.1.1-gpsmpi-2016b.eb --extended-dry-run  --optarch=GCC:GENERIC | grep "export CFLAGS"
  export CFLAGS="-O2 -xHost -ftz -fp-speculation=safe -fp-model source"
  export CFLAGS="-O2 -march=x86-64 -mtune=generic"

damianam · 2017-01-07T19:36:17Z

@boegel I changed the option type. I'll take a look to the tests soon[ish].

boegel · 2017-01-09T17:55:56Z

@damianam please remove the (WIP) tag in the PR title when you consider this good to go

…some craype and some easyblocks

damianam · 2017-01-11T22:12:27Z

@boegel I think the PR is ready now. Please review

…e correctly set

damianam · 2017-01-12T01:35:09Z

I've noticed a few caveats:

Clang won't accept this syntax because it doesn't set COMPILER_FAMILY (!?!?). Maybe we should use the compiler class name instead? That doesn't seem very clean though. I suggest to actually assign a COMPILER_FAMILY to Clang
Currently I am using PGI as a failsafe check in the tests, to check that nothing in particular is done to its settings if it is not explicitly listed. The issue here is that by default the optimal architecture setting is simply not setting anything (ie: ''), and therefore I can't check that xFLAGS are correctly set ('' in any_string is always True). I'd gladly change the check to clang but we need to fix the first issue first.

boegel · 2017-01-12T08:18:21Z

We haven't considered Clang a full compiler family up until now because it doesn't include a Fortran compiler (all other compiles we support do), but I guess that's not a very compelling argument... I'm OK with defining COMPILER_FAMILY for Clang by itself.
See inline comment for the check involving an empty string.

boegel · 2017-01-12T08:19:57Z

            'mpi-cmd-template': ("Template for MPI commands (template keys: %(nr_ranks)s, %(cmd)s)",
                                 None, 'store', None),
            'mpi-tests': ("Run MPI tests (when relevant)", None, 'store_true', True),
+            # optarch is in most cases equivalent to strlist, but changing the type breaks compatibility


hmm, I think this comment would be more confusing in the long run, it's better to mention this where we split the optarch value by ,?

Alright. I'll move it in the next commit

boegel · 2017-01-12T08:34:21Z

+                # Option is generic, so mark it as to be set later and stop iterating
+                elif optarch_option == OPTARCH_GENERIC:
+                    use_generic = True
+                    break


With this approach, a value like --optarch=GENERIC,intel:foo would be equivalent to --optarch=GENERIC, while --optarch=intel:foo,GENERIC would not be, right? That seems very confusing to me...

I think we should do some validation here, and either except --optarch=GENERIC, --optarch=<flags> or --optarch=<flags-by-compiler>, and not allow mixing; if you detect a dict-like value, we should validate that all entries really specify a mapping, and not make things too flexible (since that makes it too easy to shoot yourself in the foot and end up with a configuration that doesn't do what you think it does).

Another problem I'm concerned with is how to discriminate between --optarch=<flags> where the , character is included in the list of flags (e.g. -O2 -Wl,-Bstatic), and --optarch=<flags-by-compiler>, where the entries for the different compiler are separated by a ,.

So, I think we should go with a more 'obscure' separation character rather than ,, something like | or @ or &, i.e. stuff that is a lot less likely to appear in a set of compiler flags.
The same goes for the : character that is now used to separate the compiler name from the set of flags to use for that compiler.

Either that, or let --optarch accept the path to a file that specifies to compiler-to-flag mapping in some syntax that's easy to parse (e.g. .ini format like we have for external modules metadata, see http://easybuild.readthedocs.io/en/latest/Using_external_modules.html#metadata-for-external-modules).

I actually strongly prefer the latter, since that makes things a lot easier; you can just check whether --optarch points to an existing file, and fall back to what we do now if it doesn't. Parsing a dict-like value from a string is just painful, and it's probably better to avoid that?

You are right in the first part of the comment. My mind set was to accept the "first match" and properly document it. Even validating every entry you need something like that to deal with things like --optarch=Intel:xHost,GCC:something,Intel:xSSE2.

I think that like it is now is not just more powerful, but also easier to implement (the validation requires a couple of lines more of code, even though it is not significant). It comes with a cost, which is lack of strictness (sounds worse than flexibility :-P). However I have no strong feelings about doing it like you suggest. Just let me know which direction you want to take.

Regarding the parsing bit and the separators: Couldn't we use some similar library to parse that? Even without it, it shouldn't be too complicated to accept something like --optarch=Intel:'O2 -Wl,-Bstatic',GCC:something.

Beyond that, do we really need that capability? I mean, none of the compilers I know require it (see list below), and if an user wants to put flags in --optarch that don't belong there, should we get out of our way to support that? In my opinion we should support that just if a compiler requires it.

Intel: -xHost or -xAVX2, etc

GCC: -march=x86-64 -mtune=generic

PGI: -tp=haswell

clang: -march=x86-64 -mtune=generic

IBM XL: -qtune=auto

And just as I finish typing this I see that nvcc accepts things like --gpu-architecture=compute_50 --gpu-code=sm_50,compute_50 and my argument falls apart and goes down the drain. So, coming back to the previous point, do we accept something like --optarch=Intel:'O2 -Wl,-Bstatic',GCC:something, or do we change the characters? Changing them is trivial so I am ok with it.

Last but not least: I strongly oppose the configuration file idea. My arguments are:

Disrupts our workflow. We do this sort of tuning with environment variables that are set in configuration modules. We just use files for python code (custom easyblocks, toolchains, MNS)

It is kind of overkill. The longest string I can think of is something like Intel:xCORE-AVX2,GCC:-march=x86-64 -mtune=generic,PGI:-tp=haswell,CLANG:-march=x86-64 -mtune=generic,IBMXL:-qtune=auto,CUDA:--gpu-architecture=compute_50 --gpu-code=sm_50,compute_50. This is an exaggeration, but even exaggerating it, it is pretty manageable.

I would be willing to accept a file as an additional option, if it fits better in people's workflow, but not excluding the --optarch=Compiler1:flag1,Compiler2:flag2 syntax

I think that being too flexible may be an issue too, since it makes it too easy to have eb do something while you're expecting something else. Even if this is clearly documented, we should try and avoid surprises, and do a bit of input validation to protect against silly mistakes (we all make them).

I'm OK with your opposition against using (only) a file, I understand the argument. Also supporting a file can always be added later.

The easiest way to go forward seems to be:

use different characters than , and : to specify the mapping of compiler to flags (the nvcc flags are an excellent example); wrapping flags in single quotes is not something I'd go for, since you need to be quite careful to make sure bash doesn't eat your single quotes...

I would also input validation, see my previous comment and add to that to not allow includes the same compiler twice (like in your Intel:...,...,Intel:.... example), there's no reason we should allow that, it will only lead to confusion. And it's fairly easy to check.

Ok, changed to : and ;, and now it is more strict

boegel · 2017-01-12T08:36:14Z

+                    current_compiler = getattr(self, 'COMPILER_FAMILY', None)
+                    comp = optarch_option.split(":")[0]
+                    comp_optarch = optarch_option.split(":")[1] if len(optarch_option.split(":")) > 1 else None 
+                    if comp == current_compiler:


you're now both parsing the --optarch value and checking for a match in one go, it would make more sense to do the parsing a lot earlier (when initialising the toolchain), to catch any formatting problems with the specified --optarch value early on

the actual checking of which flags to use from that parsed value should still occur here

Mmmmmh. It looks reasonable in principle but following that approach I think it would make more sense to have an input sanitizing step for all the options. Is there something like that?

Otherwise seems kind of unnatural, this option would be the only one that gets parsed before it is actually processed.

We have type checking support for easyconfig parameters, but not for configuration options.

We do a little bit of option checking in the postprocess method in options.py (see e.g. the check w.r.t. module syntax), but not to a big extent. That would be the right location for input validation though.

boegel · 2017-01-12T08:38:27Z

+                build_options = {'optarch': optarch_var}
+                init_config(build_options=build_options)
+                for toolchain, toolchain_version in [('iccifort','2011.13.367'),('GCC','4.7.2'),('PGI','16.7-GCC-5.4.0-2.26')]:
+                    for enable in [True, False]:


wow, a quadruple for-loop is a bit hard to understand...

Can't we construct one large list of test cases outside of the for loop, and then just have a single for loop over those?

That would also help with avoiding the deep indenting here...

You mean manually constructing that list of 54 elements, or construct the list in 4 loops, and then iterate over it?

In any case, they are 4 loops but fairly simple

No, I mean creating 4 separate lists, and then letting Python create a list of 4-tuples with all possible options (seems like itertools.product does what you need).

Anyway, this is less important, since this is only a test where code style is less of an issue

Ignored, at least for the moment :-P

boegel · 2017-01-12T08:39:42Z

+                            flag = gcc_expanded_flags 
+                        else: # PGI as an example of compiler not set
+                            # default optarch flag
+                            flag = tc.COMPILER_OPTIMAL_ARCHITECTURE_OPTION[(tc.arch,tc.cpu_family)]


This should be handled outside the for loop, where you construct a list of test cases?

Also, hardcode the value for PGI rather than grabbing it from the Toolchain instance?

If I hardcode it it would break if one day changes. A change like that should be irrelevant for the test.

If something changes in framework that affects this test, we can update the test accordingly.

I'd rather be aware of the impact of a change in framework.

boegel · 2017-01-12T08:40:33Z

+                        self.assertTrue(flag == flags, "optarch: True means '%s' == '%s'" % (flag, flags))
+
+                        # Also check that it is correctly passed to xFLAGS, honoring 'enable'
+                        if flag != '':


what if flag is == '', no checks at all?

The only way I can think of to check when flag is == '' is to have a blacklist and check against it, which is kind of overkill I think....

well, we should have an else in any case?

damianam · 2017-01-20T17:46:41Z

@boegel I've implemented most of the discussed changes. From my perspective there is just one thing remaining: Making test to make sure that the parsing is done correctly, not just checking for exceptions.

And waiting for travis to report on all the tests, I haven't run them myself, just the new ones.

boegel · 2017-01-22T12:57:20Z

 from easybuild.tools.run import run_cmd
 from easybuild.tools.package.utilities import avail_package_naming_schemes
-from easybuild.tools.toolchain.compiler import DEFAULT_OPT_LEVEL, Compiler
+from easybuild.tools.toolchain.compiler import DEFAULT_OPT_LEVEL, OPTARCH_TOOLCHAIN_DELIMITER, OPTARCH_OPTION_DELIMITER, Compiler


@damianam line too long

boegel · 2017-01-22T12:58:19Z

+        # make sure --optarch has a valid format
+        if self.options.optarch:
+            # optarch is in most cases equivalent to strlist, but changing the type breaks compatibility, so we hack it here
+            n_compilers = len(self.options.optarch.split(OPTARCH_TOOLCHAIN_DELIMITER))


Please put this whole block into a ('private') dedicated method, i.e. self._postprocess_optarch()

just to the split once, and save it in a variable you reuse later:

optarch_parts = self.options.optarch.split(OPTARCH_TOOLCHAIN_DELIMITER)

boegel · 2017-01-22T13:00:48Z


+        # make sure --optarch has a valid format
+        if self.options.optarch:
+            # optarch is in most cases equivalent to strlist, but changing the type breaks compatibility, so we hack it here


I'm not sure if we should include this comment, since we now have other reasons to take this approach, e.g. strlist only supports using , as a separator, while we're using ;

boegel · 2017-01-22T13:02:18Z

+                # if there are options for different compilers, we set up a dict
+                if OPTARCH_OPTION_DELIMITER in self.options.optarch:
+                    optarch_dict = dict()
+                    for compiler, compiler_opt in [i.split(OPTARCH_OPTION_DELIMITER) for i in self.options.optarch.split(OPTARCH_TOOLCHAIN_DELIMITER)]:


reuse optarch_parts here, and I'd use x rather than i (i is usually an index)

boegel · 2017-01-22T13:03:42Z

+                raise EasyBuildError("The optarch option has an incorrect syntax: %s" % self.options.optarch)
+            else:
+                # if there are options for different compilers, we set up a dict
+                if OPTARCH_OPTION_DELIMITER in self.options.optarch:


this should be if OPTARCH_OPTION_DELIMITER in optarch_parts[0]?

boegel · 2017-01-22T13:13:47Z

-        # no --optarch specified, no default value specified
-        elif (self.arch, self.cpu_family) in (self.COMPILER_OPTIMAL_ARCHITECTURE_OPTION or []):
+        # no --optarch specified, no option found for the current compiler, and no default optarch
+        elif use_generic == False and optarch is None and (self.arch, self.cpu_family) in (self.COMPILER_OPTIMAL_ARCHITECTURE_OPTION or []):


you know use_generic is False, otherwise you could never reach here

boegel · 2017-01-22T13:15:26Z

+                raise EasyBuildError("optarch is neither an string or a dict. This should never ever happen")
+
+            if optarch is None and use_generic == False:
+                self.log.info("_set_optimal_architecture: no optarch found for compiler %s. Ignoring option." % current_compiler)


self.log.info("...", current_compiler)

(replace % with, )

boegel · 2017-01-22T13:15:48Z

        self.assertTrue(re.search('^\s+\* GCC v4.6.3: dummy', txt, re.M))
        self.assertFalse(re.search('gzip', txt, re.M))

+    def test_optarch(self):


call this test_parse_optarch?

boegel · 2017-01-22T13:16:18Z

+        error_msg = "The optarch option contains duplicated entries for compiler"
+        self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True)
+
+        # TODO: Check for correct parsing and dictionary creation


yes please :)

also check with a list of flags, GENERIC, ...

boegel · 2017-01-22T13:20:58Z

+                                if enable:
+                                    self.assertTrue(flag in tc.get_variable(var), "optarch: True means '%s' in '%s'" % (flag, tc.get_variable(var)))
+                                else:
+                                    self.assertFalse(flag in tc.get_variable(var), "optarch: False means no '%s' in '%s'" % (flag, tc.get_variable(var)))


several lines are way too long here, partially due to the deep indent...

also, just do the tc.get_variable(var)s once, above the if enable...

damianam · 2017-01-23T17:47:41Z

@boegel I addressed all your comments, even the one that altered the logic for the code avoiding calling too many times build_option('optarch'). The only thing missing as far as I can see is completing the tests of options.py

damianam · 2017-01-24T17:16:55Z

@boegel I think the PR is ready for a final review. I am only wondering if there is a better way to test that EasyBuild doesn't generate exceptions when it shouldn't, than simply run it and see what happens. Is there some sort of asssertNoError?

boegel · 2017-01-24T21:29:44Z

+        self.assertErrorRegex(EasyBuildError, error_msg, self.eb_main, args, raise_error=True)
+
+        # Check that it doesn't raise an exception when the input is correct
+        test_easyconfig = 'easyconfigs/test_ecs/t/toy/toy-0.0.eb'


@damianam please specify the full path here, usually like this:

test_ecs = os.path.join(os.path.dirname(os.path.abspath(__file__)), 'easyconfigs', 'test_ecs') toy_ec = os.path.join(test_ecs, 't', 'toy', 'toy-0.0.eb')

Mmmmh, ok, but can I ask why?

The main reason to use the full path to toy-0.0.eb is to make sure the test always runs, also if you're running it from a different location.

This test was failing on Travis with

EasyBuildError: "Can't find path /home/travis/easyconfigs/test_ecs/t/toy/toy-0.0.eb"

It probably worked for you because you were running the tests from the test/framework subdirectory?

boegel · 2017-01-24T21:33:54Z

+                ['--optarch=Intel:;GCC:somethingelse;',test_easyconfig]
+                ]
+        for args in test_cases:
+            self.eb_main(args, raise_error=True)


@damianam this doesn't seem like a very useful test, you're not actually checking anything here (other than the lack of crashing)...

Why not test with an existing test easyconfig that uses GCC as a toolchain (which is a lot better than using the dummy toolchain where <toolchain>.prepare() is never called...), and also include --extended-dry-run as an option. That wait, you can check in the output whether you're seeing what you expect to see.

What you say is done in the tests of toolchain.py. There I check against xFLAGS.

What I wanted to check here is simply that there aren't any issues when the input is correct. Arguably, it doesn't make too much sense, since the affected code is tested right below, calling options.postprocess(), where the code is really validated.

The reason I included these apparently useless tests is because the tests below check against just the postprocessing of options, instead of a "full" run. If you think this is not necessary I can simply delete these lines, but a more thorough test is already done in toolchain.py, I see no point in repeating it.

Doing a 'full' run using toy-0.0.eb is meaningless imho, since toolchain.prepare will never be called since the dummy toolchain is used.

So, yes, I would drop this, since this test is mainly about how the optarch value is being parsed.

I would also change the tests where you check for the error on faulty output to not use eb_main but options.postprocess()?

boegel · 2017-01-24T21:34:48Z

+            # if flag is '' there is not much to check. A more robust approach
+            # would be to check against a blacklist, but that is probably not necessary
+            else:
+                pass


this doesn't do anything at all... :)

That's not true. It is not completely useless, it can be used as a bad example ;-P. I'll make a blacklist check then.

… remaining against options.postprocess instead of eb_main

boegel · 2017-01-26T13:48:52Z

 from easybuild.tools.package.utilities import avail_package_naming_schemes
-from easybuild.tools.toolchain.compiler import DEFAULT_OPT_LEVEL, Compiler
+from easybuild.tools.toolchain.compiler import DEFAULT_OPT_LEVEL, OPTARCH_SEP, \
+        OPTARCH_MAP_CHAR, Compiler


please revert to a single line, sort alphabetically (constants first)

boegel · 2017-01-26T13:50:20Z

+        n_compilers = len(optarch_parts)
+
+        # if the format is not adequate we refuse continuing
+        if n_compilers > 1 and n_compilers != self.options.optarch.count(OPTARCH_MAP_CHAR):


I would change this to:

if len(optarch_parts) > 1 and not all(OPTARCH_MAP_CHAR in p for p in optarch_parts):

which is a little bit stricter (it matter where the :s are, there should be one in each entry, otherwise "foo;bar::baz" is also valid)

and then you can drop n_compilers; not used anymore

boegel · 2017-01-26T13:50:50Z

+        if n_compilers > 1 and n_compilers != self.options.optarch.count(OPTARCH_MAP_CHAR):
+            raise EasyBuildError("The optarch option has an incorrect syntax: %s" % self.options.optarch)
+        else:
+            logger = fancylogger.getLogger()


just use self.log rather than defining another logger

Sounds reasonable, but we should be consistent. There are more loggers being instantiated in that file.

In a method (where self is an argument), you should use self.log.

In a function (no self in scope, like in process_software_build_specs), you may need to create a separate logger.

In this case, there's actually a global _log available, we should be using that consistently where a self.log is not available, e.g. in process_software_build_specs

boegel · 2017-01-26T13:51:32Z

+                optarch_dict = {}
+                for compiler, compiler_opt in [x.split(OPTARCH_MAP_CHAR) for x in optarch_parts]:
+                    if compiler in optarch_dict:
+                        raise EasyBuildError("The optarch option contains duplicated entries for compiler %s: %s" 


use raise EasyBuildError("...", foo, bar) (rather than "..." % (foo, bar))

Same comment as above. I am ok with the change, but we should be consistent (to be done in a different PR?)

Yeah, let's not clutter this PR with pure style changes in code we're not touching.

Feel free to tackle this in a separate PR, if you're up for it.

boegel · 2017-01-26T13:51:54Z

+                    else:
+                        optarch_dict[compiler] = compiler_opt
+                self.options.optarch = optarch_dict 
+                logger.info("Transforming optarch into a dict: %s" % self.options.optarch)


replace "..." % with "...",

boegel · 2017-01-26T14:13:07Z

+                       gcc_options[1][1],
+                       'xHost', # default optimal for Intel
+                       'march=native', # default optimal for GCC
+                       ]


please fix indent + left-align ]

boegel · 2017-01-26T14:14:11Z

+                       intel_options[0][1],
+                       intel_options[1][1],
+                       gcc_options[0][1],
+                       gcc_options[1][1],


hmm, would it be clearer to just hardcode those strings here?

hardcoding is fine in tests if it makes things clearer

I think hardcoding will create more troubles down the road. If somebody needs to update this test they'll have to update that in various places........ which is plain annoying. Sure, that will be more readable, but less maintainable, and the only time somebody will read this test is to maintain it, so........ I vote for leaving it as it is ;-P

I consider maintainability of tests less of an issue...

If you changes the values in intel_options and gcc_options, it's good to know where those are (also) used. Hardcoding the strings here forces you to also make the corresponding changes here, and makes you think about whether making the changes actually makes sense.

But fine, I don't feel too strongly about it, we're less strict in the tests.

boegel · 2017-01-26T14:16:23Z

+                 # Check that the correct flags are there
+                 if enable and flag != '':
+                     self.assertTrue(flag in flags, "optarch: True means '%s' in '%s'" 
+                             % (flag, flags))


I'd prefer:

error_msg = "optarch: True means '%s' in '%s'" % (flags, optarch_flags) self.assertTrue(flags in optarch_flags, error_msg)

boegel · 2017-01-26T14:16:59Z

+                 else:
+                     for blacklisted_flag in blacklist:
+                         self.assertFalse(blacklisted_flag in flags, "optarch: False means no '%s' in '%s'" 
+                                 % (blacklisted_flag, flags))


same here:

error_msg = "optarch: False means no '%s' in '%s'" % (blacklisted_flag, flags) self.assertFalse(blacklisted_flag in optarch_flags, error_msg)

boegel · 2017-01-26T14:18:13Z

+                     self.assertTrue(flag in flags, "optarch: True means '%s' in '%s'" 
+                             % (flag, flags))
+
+                 # Check that there aren't unexpected flags


aren't any

damianam

@boegel I applied all your suggested changes, unless said otherwise in the individual comments.

boegel · 2017-01-26T16:56:37Z

+        optarch_parts = self.options.optarch.split(OPTARCH_SEP)
+
+        # we expect to find a ':' in every entry in optarch, in case optarch is specified on a per-compiler basis
+        if len(optarch_parts) > 1 and not all(OPTARCH_MAP_CHAR in p for p in optarch_parts):


@damianam I gave this some more thought, and I actually think we need a more complex check to really validate this... We should check that there exactly one OPTARCH_MAP_CHAR in each part, now we may have multiple, which will screw things up later where we use .split(OPTARCH_MAP_CHAR)...

boegel · 2017-01-26T16:57:07Z

+                for compiler, compiler_opt in [x.split(OPTARCH_MAP_CHAR) for x in optarch_parts]:
+                    if compiler in optarch_dict:
+                        raise EasyBuildError("The optarch option contains duplicated entries for compiler %s: %s",
+                                compiler, self.options.optarch)


@damianam minor style issue: please indent further to align the c with the first " above...

boegel · 2017-01-26T17:04:04Z

+                    self.log.info("_set_optimal_architecture: no optarch found for compiler %s. Ignoring option.", 
+                            current_compiler)
+            else:
+                raise EasyBuildError("optarch is neither an string or a dict. This should never ever happen")


@damianam we should show what's in optarch to make the error clearer? (although it's unlikely this will ever happen)

…hingelse kind of mistakes

boegel · 2017-01-26T18:22:52Z

+
+        # we expect to find a ':' in every entry in optarch, in case optarch is specified on a per-compiler basis
+        if (len(optarch_parts) > 1 and not all(p.count(OPTARCH_MAP_CHAR) == 1 for p in optarch_parts)) or \
+                (len(optarch_parts) == 1 and optarch_parts[0].count(OPTARCH_MAP_CHAR) > 1):


wait, this is just

if not all(p.count(OPTARCH_MAP_CHAR) == 1 for p in optarch_parts)):

is it not?

No. If you pass a simple string you'll raise and exception and break backwards compatibility, and force everyone to specify compiler. The count of OPTARCH_MAP_CHAR can be 0 if no compiler is specified

OK, I missed that bit, but this is very long and messy...

How about:

n_parts = len(optarch_parts) map_char_cnts = [p.count(OPTARCH_MAP_CHAR) for p in optarch_parts] if (n_parts > 1 and any(c != 1 for c in map_char_cnts)) or (n_parts == 1 and map_char_cnts[0] > 1):

Looks more messy to me..... The if is shorter, sure, but everything else seems unnecessarly complicated to me. But I don't really care....

Well, the variable names are key in my approach, maybe they could be improved.

In your version there's a lot of copy-pasting of code snippets, which makes it hard to parse imho...

Very well then. Done

boegel · 2017-01-27T12:12:23Z

Going in, thanks @damianam!

Please also look into a documentation update @ http://easybuild.readthedocs.io/en/latest/Controlling_compiler_optimization_flags.html

ocaisa · 2017-01-27T12:43:08Z

This has got to be the longest conversation for a PR EasyBuild has ever had?

damianam · 2017-01-27T13:03:40Z

We are social people, we like to talk. That has to be reason, right?

Damian Alvarez added 2 commits December 29, 2016 17:35

optarch can now be specified on a toolchain basis. For instance: --op…

98a0124

…tarch=Intel:xHost,GCC:GENERIC

Fix default for when no option is found for the current compiler

744428b

Fix bug when passing empty string to a particular toolchain

7451c0a

boegel added this to the 3.1.0 milestone Dec 30, 2016

Damian Alvarez added 2 commits January 7, 2017 20:06

Changed optarch to strlist

a1425d6

Merged with upstream

0f2abe6

boegel changed the title ~~optarch can now be specified on a toolchain basis.~~ optarch can now be specified on a toolchain basis (WIP) Jan 9, 2017

Damian Alvarez and others added 2 commits January 10, 2017 18:29

Removed strlist as type for optarch, as it breaks compatibility with …

440f7f8

…some craype and some easyblocks

Test for compiler dependent optarch

1adfd85

damianam changed the title ~~optarch can now be specified on a toolchain basis (WIP)~~ optarch can now be specified on a toolchain basis Jan 11, 2017

Improved test_compiler_dependent_optarch to check also that xFLAGS ar…

3416f01

…e correctly set

Updated the number of shipped modules

c7b01db

boegel reviewed Jan 12, 2017

View reviewed changes

Moved optarch sanitizing and parsing to options.py

51bced0

boegel requested changes Jan 22, 2017

View reviewed changes

Damian Alvarez added 2 commits January 23, 2017 18:42

Refactor and cleanup

494d44c

Synched

ba12200

Damian Alvarez added 2 commits January 24, 2017 17:52

Completed unit tests for parsing of --optarch option

9c43a6b

Expanded tests for optarch in options.py

ad78955

Cleaned optarch test in options.py

cf139fe

boegel reviewed Jan 24, 2017

View reviewed changes

Damian Alvarez added 2 commits January 25, 2017 12:29

Added blacklisted tests for optarch options

c44e763

Dropped some useless tests of the parsing of optarch, and checked the…

9ec1fb8

… remaining against options.postprocess instead of eb_main

boegel requested changes Jan 26, 2017

View reviewed changes

Style fixes

9baa750

damianam commented Jan 26, 2017

View reviewed changes

boegel reviewed Jan 26, 2017

View reviewed changes

Improved tests and extra checking for --optarch=Intel:something:somet…

a9b4b44

…hingelse kind of mistakes

boegel reviewed Jan 26, 2017

View reviewed changes

Refactor the condition checking for correct optarch syntax

7cdff97

boegel approved these changes Jan 27, 2017

View reviewed changes

boegel merged commit c8e3542 into easybuilders:develop Jan 27, 2017

damianam deleted the optarch branch July 12, 2017 09:51

Conversation

damianam commented Dec 29, 2016

Uh oh!

boegel commented Dec 29, 2016

Uh oh!

damianam commented Dec 29, 2016

Uh oh!

damianam commented Dec 29, 2016

Uh oh!

boegel commented Dec 29, 2016

Uh oh!

damianam commented Dec 29, 2016

Uh oh!

damianam commented Jan 7, 2017

Uh oh!

boegel commented Jan 9, 2017

Uh oh!

damianam commented Jan 11, 2017

Uh oh!

damianam commented Jan 12, 2017

Uh oh!

boegel commented Jan 12, 2017

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!

boegel Jan 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel Jan 12, 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!

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!

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!

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!

damianam commented Jan 20, 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!

Choose a reason for hiding this comment

Uh oh!

boegel Jan 12, 2017 •

edited

Loading

boegel Jan 12, 2017 •

edited

Loading

damianam commented Jan 20, 2017 •

edited

Loading