FSL - Improve configuration choice by vanzod · Pull Request #1498 · easybuilders/easybuild-easyblocks

vanzod · 2018-09-04T22:59:31Z

FSL build relies on an internal check and a script to identify the appropriate configuration to use. Since the available configurations are for relatively old compilers, the current easyblock uses difflib.get_close_matches to identify the closest configuration and copies the directory containing it into a new one named after the configuration targeted by FSL. In certain cases the guessed closest configuration is not the same as the one at which required patches are applied, resulting in a build failure.
This easyblock change uses the patch files as first source of information to guess the target directory and rolls back to difflib.get_close_matches only if no target configuration is found in the patches.

boegel · 2018-09-17T14:12:05Z

+
+        for patch in self.patches:
+            try:
+                for line in open(patch['path'], 'r'):


@vanzod Please use the read_file function from filetools

boegel · 2018-09-17T14:12:49Z

+        for patch in self.patches:
+            try:
+                for line in open(patch['path'], 'r'):
+                    if all(regex.match(line) for regex in [systype_regex, diff_regex]):


Since there are only two, this is probably clearer:

if systype_regex.match(line) and diff_regex.match(line):

But, is there a specific reason why both regex's can't be joined in a single regex?

The reason they are separate is because I use systype_regex twice and only once in conjunction with diff_regex. Hence I preferred to split them like that instead of having systype_regex being a subset of diff_regex (i.e. avoid copy/paste).

boegel · 2018-09-17T14:17:15Z

+            try:
+                for line in open(patch['path'], 'r'):
+                    if all(regex.match(line) for regex in [systype_regex, diff_regex]):
+                        matches = [m.group(0) for l in line.split('/') for m in [systype_regex.search(l)] if m]


Hmm, why use a list if you're only considering systype_regex? Ah, because you need a handle for the match (m)... It looks a bit weird like this, probably better roll this out for the sake of clarity:

for part in line.split('/'): res = systype_regex.search(part) if res: patched_cfgs.append(res.group(0))

boegel · 2018-09-17T14:18:17Z

+                        matches = [m.group(0) for l in line.split('/') for m in [systype_regex.search(l)] if m]
+                        patched_cfgs.extend(matches)
+            except OSError, err:
+                raise EasyBuildError("Unable to open patch file: %s", patch['path'])


No more need for the the try/except when you use read_file

boegel · 2018-09-17T14:18:51Z

+        # If no patched config is found, pick best guess
        cfgdir = os.path.join(self.fsldir, "config")
-        try:
+        if not best_cfg:


The try is still relevant here, os.listdir may throw an OSError

boegel · 2018-09-17T14:19:27Z


+        # Prepare config
+        # Either use patched config or copy closest match
+        try:


@vanzod Please use copy_file or copy_dir below (and then there's no need for the try here

boegel · 2018-09-17T14:20:01Z

+                raise EasyBuildError("Unable to open patch file: %s", patch['path'])
+
+        if patched_cfgs:
+            if patched_cfgs[1:] == patched_cfgs[:-1]:


Why not check for len(patched_cfgs) == 1?

That would work in the first if statement. In the second I am checking that all strings in patched_cfgs are the same, which means that the patch targets only one configuration.

OK, then please clarify with a comment above this line, that's not clear at all :)

boegel · 2018-09-17T14:20:18Z

+        if patched_cfgs:
+            if patched_cfgs[1:] == patched_cfgs[:-1]:
+                best_cfg = patched_cfgs[0]
+                self.log.debug("Found patched config dir: %s" % (best_cfg))


self.log.debug("...", best_cfg)

boegel · 2018-09-17T14:20:40Z

+                self.log.debug("Found patched config dir: %s" % (best_cfg))
+            else:
+                dirs = ', '.join(patched_cfgs)
+                raise EasyBuildError("Patch files are editing multiple config dirs: %s", dirs)


it's OK to pass the raw patched_cfgs in the error message imho

boegel · 2018-09-17T14:23:17Z

@vanzod I quickly ran into trouble when trying this with an old FSL easyconfig (FSL-4.1.9-goolf-1.4.10.eb):

Patch files are editing multiple config dirs: apple-darwin10-gcc4.2, apple-darwin10-gcc4.2, apple-darwin7-gcc3.1, apple-darwin7-gcc3.1, apple-darwin7-gcc3.3, apple-darwin7-gcc3.3, apple-darwin8-gcc4.0, apple-darwin8-gcc4.0, apple-darwin9-gcc4.0, apple-darwin9-gcc4.0, i686-pc-cygwin-gcc3.2, i686-pc-c

So this will need a version check...

boegel · 2018-09-17T15:30:00Z

@vanzod It looks like this can only work with FSL 5.0.10 & newer (works with FSL-5.0.10-intel-2017a.eb and FSL-5.0.11-foss-2018b.eb

houndci-bot · 2018-09-17T21:39:35Z

+                cfgs = os.listdir(cfgdir)
+                best_cfg = difflib.get_close_matches(fslmachtype, cfgs)[0]
+                self.log.debug("Best matching config dir for %s is %s" % (fslmachtype, best_cfg))
+        except OSError, err:


SyntaxError: invalid syntax

This should probably be except OSError as err:

houndci-bot · 2018-09-17T21:39:36Z

+            systype_regex = re.compile("^diff.*config\/(.*(apple|gnu|i686|linux|spark)(?:(?!\/).)*)", re.M)
+
+            patched_cfgs = []
+


blank line contains whitespace

@vanzod The hound is merciless...

I like that

boegel · 2018-09-18T13:13:54Z

+            systype_regex = re.compile("^diff.*config\/(.*(apple|gnu|i686|linux|spark)(?:(?!\/).)*)", re.M)
+
+            patched_cfgs = []
+


@vanzod The hound is merciless...

boegel · 2018-09-18T13:14:11Z

+                cfgs = os.listdir(cfgdir)
+                best_cfg = difflib.get_close_matches(fslmachtype, cfgs)[0]
+                self.log.debug("Best matching config dir for %s is %s" % (fslmachtype, best_cfg))
+        except OSError, err:


This should probably be except OSError as err:

boegel · 2018-09-18T13:16:15Z

+                patched_cfgs.extend([i[0] for i in res])
+
+            # Check that at least one config has been found
+            if len(patched_cfgs) != 0:


if patched_cfgs: is fine here

boegel · 2018-09-18T13:17:16Z

+            # Check that at least one config has been found
+            if len(patched_cfgs) != 0:
+                # Check that a single config has been patched
+                if patched_cfgs[1:] == patched_cfgs[:-1]:


@vanzod This hurts my brain... How about using nub from vsc.utils.missing and change this to:

if len(nub(patched_cfgs)) == 1:

I like that. I did not know about nub

boegel · 2018-09-18T13:17:52Z

+                best_cfg = difflib.get_close_matches(fslmachtype, cfgs)[0]
+                self.log.debug("Best matching config dir for %s is %s" % (fslmachtype, best_cfg))
+        except OSError, err:
+            raise EasyBuildError("Unable to access configurations directory: %s", err)


include value of cfgdir in error message please

boegel · 2018-09-18T18:35:36Z

+                best_cfg = difflib.get_close_matches(fslmachtype, cfgs)[0]
+                self.log.debug("Best matching config dir for %s is %s" % (fslmachtype, best_cfg))
+        except OSError as err:
+            raise EasyBuildError("Unable to access configuration directory: %s", cfgdir, err)


@vanzod You have a %s missing here, this is going to result in a crash rather than an error message, should be:

raise EasyBuildError("Unable to access configuration directory %s: %s", cfgdir, err)

boegel · 2018-09-18T18:53:50Z

-                self.log.debug("Copied %s to %s" % (srcdir, tgtdir))
-        except OSError, err:
-            raise EasyBuildError("Failed to copy closest matching config dir: %s", err)
+            if not best_cfg:


I'm hitting this now:

File "/tmp/eb-EdQY9x/included-easyblocks/easybuild/easyblocks/fsl.py", line 95, in configure_step if not best_cfg: UnboundLocalError: local variable 'best_cfg' referenced before assignment

It looks like I have accidentally deleted the definition of best_cfg during the refactoring

houndci-bot · 2018-09-19T13:44:52Z

+
+            patched_cfgs = []
+            best_cfg = None
+


blank line contains whitespace

boegel · 2018-09-19T14:44:05Z

+            systype_regex = re.compile("^diff.*config\/(.*(apple|gnu|i686|linux|spark)(?:(?!\/).)*)", re.M)
+
+            patched_cfgs = []
+            best_cfg = None


@vanzod This needs to go outside of this if clause to ensure it's always defined for old FSL versions?

True. Sloppy brain

boegel · 2018-09-19T16:55:22Z

Tests with existing easyconfigs are under way...

boegel · 2018-09-20T08:51:31Z

-            raise EasyBuildError("Failed to copy closest matching config dir: %s", err)
+            if not best_cfg:
+                cfgs = os.listdir(cfgdir)
+                best_cfg = difflib.get_close_matches(fslmachtype, cfgs)[0]


@vanzod This whole block is now below the if for FSL 5.0.10 or newer, while it wasn't before. That makes this change backward incompatible, resulting in breaking the installation of FSL-5.0.9-intel-2016a.eb for example...

Wait, no, it's not... But there still seems to be a problem with FSL-5.0.9-intel-2016a.eb (will re-check).

boegel · 2018-09-20T10:53:26Z

Good to go, thanks @vanzod!

vanzod added 3 commits August 31, 2018 13:57

Parse patch files to find configure dir

17b9b19

Moved patch check out of try block

c4e8f63

Re-added close match guess

b46b047

vanzod mentioned this pull request Sep 4, 2018

{bio}[foss/2017b,intel/2017b] FSL v5.0.10 easybuilders/easybuild-easyconfigs#6814

Merged

vanzod requested a review from boegel September 5, 2018 19:41

boegel added the bug fix label Sep 17, 2018

boegel requested changes Sep 17, 2018

View reviewed changes

vanzod added 2 commits September 17, 2018 15:55

Used filetools methods and improved regex

46118f1

Added version check

83c46fa

houndci-bot reviewed Sep 17, 2018

View reviewed changes

boegel requested changes Sep 18, 2018

View reviewed changes

Applied corrections

c77ee10

boegel requested changes Sep 18, 2018

View reviewed changes

boegel reviewed Sep 18, 2018

View reviewed changes

boegel added this to the 3.7.0 milestone Sep 18, 2018

Re-added accidentally deleted definition

e7c4301

houndci-bot reviewed Sep 19, 2018

View reviewed changes

Comment thread easybuild/easyblocks/f/fsl.py Outdated

patched_cfgs = []

best_cfg = None

Copy link
Copy Markdown

houndci-bot Sep 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line contains whitespace

Removed spaces

3ee772b

boegel requested changes Sep 19, 2018

View reviewed changes

Moved declaration outside version check

3fe8904

boegel approved these changes Sep 19, 2018

View reviewed changes

boegel requested changes Sep 20, 2018

View reviewed changes

boegel approved these changes Sep 20, 2018

View reviewed changes

boegel merged commit b8fbddc into easybuilders:develop Sep 20, 2018

		systype_regex = re.compile("^diff.config\/(.(apple\|gnu\|i686\|linux\|spark)(?:(?!\/).)*)", re.M)

		patched_cfgs = []

Conversation

vanzod commented Sep 4, 2018

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!

boegel commented Sep 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boegel commented Sep 17, 2018

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!

Choose a reason for hiding this comment

Uh oh!

boegel commented Sep 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Sep 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

boegel commented Sep 17, 2018 •

edited

Loading