Added block for MXNet by wpoely86 · Pull Request #1135 · easybuilders/easybuild-easyblocks

wpoely86 · 2017-03-14T10:01:15Z

@hajgato this works
~~It needs #1134~~

To Do:

Found out why the python sanity check fails. Most likely PYTHONPATH is missing.

wpoely86 · 2017-03-14T10:02:57Z

+            'dirs': [],
+        }
+        super(EB_MXNet, self).sanity_check_step(custom_paths=custom_paths)
+        self.py_ext.sanity_check_step()


This one fails:

== 2017-03-14 10:54:04,601 run.py:137 DEBUG run_cmd: running cmd /apps/brussel/broadwell/software/Python/2.7.12-foss-2016b/bin/python -c "import mxnet" (in /svub4/wapoelma/.local/easybuild/software/MXNet/0.9.3-foss-2016b-Python-2.7.12) == 2017-03-14 10:54:04,601 run.py:162 INFO running cmd: /apps/brussel/broadwell/software/Python/2.7.12-foss-2016b/bin/python -c "import mxnet" == 2017-03-14 10:54:04,691 run.py:444 DEBUG cmd "/apps/brussel/broadwell/software/Python/2.7.12-foss-2016b/bin/python -c "import mxnet"" exited with exitcode 1 and output: Traceback (most recent call last): File "<string>", line 1, in <module> ImportError: No module named mxnet == 2017-03-14 10:54:04,692 extension.py:163 WARNING Extension: mxnet failed to install, cmd '/apps/brussel/broadwell/software/Python/2.7.12-foss-2016b/bin/python -c "import mxnet"' (stdin: None) output: Traceback (most recent call last): File "<string>", line 1, in <module> ImportError: No module named mxnet == 2017-03-14 10:54:04,692 extensioneasyblock.py:137 WARNING Sanity check for mxnet failed: mxnet failed to install, cmd '/apps/brussel/broadwell/software/Python/2.7.12-foss-2016b/bin/python -c "import mxnet"' (stdin: None) output: Traceback (most recent call last): File "<string>", line 1, in <module> ImportError: No module named mxnet == 2017-03-14 10:54:04,692 easyconfig.py:1208 WARNING Unable to resolve template value library(%(ext_name)s) with dict {'nameletterlower': 'm', 'pyshortver': '2.7', 'versionsuffix': '-Python-2.7.12', 'rshortver': '3.3', 'toolchain_name': 'foss', 'namelower': 'mxnet', 'version_minor': '9', 'name': 'MXNet', 'versionprefix': '', 'version_major': '0', 'nameletter': 'M', 'version_major_minor': '0.9', 'pyver': '2.7.12', 'version': '0.9.3', 'rver': '3.3.3', 'toolchain_version': '2016b'} == 2017-03-14 10:54:04,693 extensioneasyblock.py:114 DEBUG starting sanity check for extension with filter ('R -q --no-save', 'library(%(ext_name)s)')

but it continues nevertheless. The extension works if you load the final module so I'm guessing the issue is that the PYTHONPATH is not set?

@wpoely86 You need to check the return value of sanity_check_step for extensions, or maybe just call the ones for the extensions first, and then call to super

wpoely86 · 2017-03-14T10:06:17Z

+        txt = super(EB_MXNet, self).make_module_extra(*args, **kwargs)
+        txt += self.py_ext.make_module_extra(*args, **kwargs)
+        txt += self.r_ext.make_module_extra(*args, **kwargs)
+        return txt


this gives:

prepend-path CPATH $root/include prepend-path LD_LIBRARY_PATH $root/lib prepend-path LIBRARY_PATH $root/lib prepend-path PATH $root/bin setenv EBROOTMXNET "$root" setenv EBVERSIONMXNET "0.9.3" setenv EBDEVELMXNET "$root/easybuild/MXNet-0.9.3-foss-2016b-Python-2.7.12-easybuild-devel" setenv EBROOTMXNET "$root" setenv EBVERSIONMXNET None setenv EBDEVELMXNET "$root/easybuild/mxnet-None-foss-2016b-Python-2.7.12-easybuild-devel" prepend-path PYTHONPATH $root/lib/python2.7/site-packages setenv EBROOTMXNET "$root" setenv EBVERSIONMXNET None setenv EBDEVELMXNET "$root/easybuild/mxnet-None-foss-2016b-Python-2.7.12-easybuild-devel" prepend-path R_LIBS $root

Any way to use the logic from the extensions easyblock to figure out the PYTHONPATH and R_LIBS without have duplicate entries?

The way you're using it is (clearly) not how it's intended to be used, since make_module_extra also defines $EBROOT* and $EBVERSION*.

You should just extend txt with update statements for $PYTHONPATH and $R_LIBS yourself, we don't have a better way of dealing with that unless we do some refactoring (e.g. flesh out the bit that defines $PYTHONPATH into a separate method that we can call, etc.)

boegel · 2017-03-14T10:56:54Z

+        """Build & Install both Python and R extension"""
+        # we start with the python bindings
+        self.py_ext.src = os.path.join(self.mxnet_src_dir, "python")
+        os.chdir(self.py_ext.src)


use change_dir? requires easybuilders/easybuild-framework#2155

boegel · 2017-03-14T10:57:26Z

+from easybuild.easyblocks.generic.rpackage import RPackage
+from easybuild.framework.easyconfig import CUSTOM
+from easybuild.tools.build_log import EasyBuildError
+from easybuild.tools.filetools import apply_regex_substitutions, copy_file, symlink, write_file, mkdir


please sort (mkdir)

boegel · 2017-03-14T10:58:05Z

+        super(EB_MXNet, self).__init__(*args, **kwargs)
+
+        self.mxnet_src_dir = None
+        self.py_ext = PythonPackage(self, {'name': 'mxnet'})


use self.name rather than hardcoding 'mxnet', and also specify version?

boegel · 2017-03-14T11:00:35Z

+        if self.mxnet_src_dir is None:
+            raise EasyBuildError("Could not find the MXNet source directory")
+
+        self.log.debug("MXNet dir is: %s", self.mxnet_src_dir)


mxnet_dirs = glob.glob(os.path.join(self.builddir, 'mxnet-*')) if len(mxnet_dirs) == 1: self.mxnet_src_dir = mxnet_dirs[0] self.log.debug("MXNet dir is: %s", self.mxnet_src_dir) else: raise EasyBuildError("Failed to find/isolate MXNet source directory: %s", mxnet_dirs)

boegel · 2017-03-14T11:01:49Z

+        for srcdir in os.listdir(self.builddir):
+            if srcdir.startswith("mxnet-"):
+                continue
+            else:


if not srcdir.startswith('mxnet-'):

or

if srcdir != os.path.basename(self.mxnet_src_dir):

boegel · 2017-03-14T11:06:24Z

+        elif toolchain_blas == 'OpenBLAS':
+            blas = "openblas"
+        elif toolchain_blas is None:
+            # This toolchain has no BLAS library


this comment is not really needed, situation is pretty clear?

boegel · 2017-03-14T11:08:04Z

+
+    def install_step(self):
+        """Specify list of files to copy"""
+        self.cfg['files_to_copy'] = ['bin', 'include', 'lib',


if files_to_copy would be specified in the easyconfig, you're hard overwriting it here?

yes. You're the one always telling me to put stuff in the block 😉

at least issue a warning if files_to_copy was specified and you're hard overwriting it now?

We don't do that in any of the blocks that have a similar approach?

Which doesn't mean we shouldn't... ;)

yeah maybe.

I can extend if that makes you happy

I would, just to avoid surprises if people try to specify files_to_copy in the easyconfig (which would get bluntly ignored right now)

boegel · 2017-03-14T11:08:30Z

+        # next up, the R bindings
+        self.r_ext.src = os.path.join(self.mxnet_src_dir, "R-package")
+        try:
+            os.chdir(self.r_ext.src)


change_dir => no need for try

boegel · 2017-03-14T11:08:43Z

+        try:
+            os.chdir(self.r_ext.src)
+            mkdir("inst")
+            symlink(os.path.join(self.installdir, "lib"), "inst/libs")


be consistent about using os.path.join

boegel · 2017-03-14T11:09:34Z

+    def sanity_check_step(self):
+        """Check for main library files for MXNet"""
+        custom_paths = {
+            'files': ["lib/libmxnet.%s" % ext for ext in ['a', get_shared_lib_ext()]],


also check bin, etc.?

It's horrible!

* origin/develop: BUG: don't set $FPATH environment variable SAMtools: Modifications for version 1.4: errmod.h and kprobaln.h are mo longer provided nor needed. Fix typo Rename member variable to avoid conflict with identically named member of base class EasyBlock Make unpacking Python extensions optional modified trinity sanity check for versions 2.3 and above simplified code simplified some code fixed missing comma Modified petsc.py to make it more flexible for downloading packages Fix typo Auto-disable quad-precision on ARM/POWER Adding an option 'downloadinstall' that makes the installation directory before the configure, and keep it after removed patch file for FFTW easyblock disables attempts to build libquadmath versions of FFTW on Power disables attempts to build libquadmath versions of FFTW on Power

wpoely86 · 2017-03-17T15:32:33Z

This one is ready @boegel
It's not going to win any beauty contests but it works.

boegel · 2017-03-22T09:34:53Z

+            raise EasyBuildError("Failed to find/isolate MXNet source directory: %s", mxnet_dirs)
+
+        for srcdir in os.listdir(self.builddir):
+            if not srcdir.startswith('mxnet-'):


collapse to:

for srcdir in [d for d in os.listdir(self.builddir) if not d.startswith('mxnet-')]:

since you're using os.listdir here, better wrap the whole thing in try/except (IOError, OSError) as err?

boegel · 2017-03-22T09:35:18Z

+                newdir = os.path.join(self.mxnet_src_dir, submodule)
+                olddir = os.path.join(self.builddir, srcdir)
+                # first remove empty existing directory
+                rmtree2(newdir)


make sure it's empty?

It's always empty? Else the submodules won't work.

well, ok then

boegel · 2017-03-22T09:35:55Z

+                # first remove empty existing directory
+                rmtree2(newdir)
+                try:
+                    shutil.move(olddir, newdir)


why not just symlink these too like you do for nnvm?

I guess it's a matter of taste? I move because that it what it would look like if you did real git submodules?

well, ok, but we should be using a filetools function here...

There isn't one.

Let's not block this PR for that. If we add it we should use it in all blocks.

I agree to not blocking this PR for it, but then we'll never end up adding it. ;)

boegel · 2017-03-22T09:38:24Z

+            (r"export CXX = g\+\+", r"# \g<0>"),
+            (r"(?P<var>ADD_CFLAGS\s*=)\s*$", r"\g<var> %s" % os.environ['CFLAGS']),
+            (r"(?P<var>ADD_LDFLAGS\s*=)\s*$", r"\g<var> %s" % os.environ['LDFLAGS']),
+        ]


use buildopts instead to override these, no need to do runtime patching?

for (var, env_var) in [('CC', 'CC'), ('CXX', 'CXX'), ('ADD_CFLAGS', 'CFLAGS'), ('ADD_LDFLAGS', 'LDFLAGS')]: self.update.cfg('buildopts', '%s="%s"' % (var, os.getenv(env_var)))

Then you need to patch the build system to respect these. Patching their own config file is the easiest way.

And we need to patch it anyway for other stuff too (see below)

Howso? You're deriving from MakeCp and not customising the build_step, so this is standard make?

boegel · 2017-03-22T09:38:57Z

+            imkl_version = get_software_version('imkl')
+            if LooseVersion(imkl_version) >= LooseVersion('17'):
+                regex_subs.append(("USE_MKL2017 = 0", "USE_MKL2017 = 1"))
+            regex_subs.append((r"(?P<var>MKLML_ROOT=).*$", r"# \g<var>%s" % os.environ["MKLROOT"]))


same here, do this via buildopts?

same answer as above 😉

boegel · 2017-03-22T09:40:31Z

+        symlink(os.path.join(self.installdir, "include"), os.path.join("inst", "include"))
+
+        # MXNet doesn't provide a list of its R dependencies by default
+        namespace = """# Export all names


this should be a constant defined outside of the class?

boegel · 2017-03-22T09:42:00Z

+
+    def sanity_check_step(self):
+        """Check for main library files for MXNet"""
+        # for the extension we are doing the loading of the fake module ourself


also mention why you are sanity checking the extensions first?

boegel · 2017-03-22T09:42:38Z

+        self.clean_up_fake_module(fake_mod_data)
+
+        custom_paths = {
+            'files': ["lib/libmxnet.%s" % ext for ext in ['a', get_shared_lib_ext()]],


I'd go with

'files': ['lib/libmxnet.a', 'lib/libmxnet.%s' % get_shared_lib_ext()],

boegel · 2017-03-22T09:43:52Z

+        """Custom variables for MXNet module."""
+        txt = super(EB_MXNet, self).make_module_extra(*args, **kwargs)
+
+        self.py_ext.set_pylibdirs()


hmm, do this right in prepare_step, since it's now being done twice now (when fake module is created, and again when real module is created)

That's no different then any other Python Extension?

boegel · 2017-03-22T09:44:09Z

+        txt = super(EB_MXNet, self).make_module_extra(*args, **kwargs)
+
+        self.py_ext.set_pylibdirs()
+        for path in self.py_ext.all_pylibdirs:


s/path/pylibdir/

wpoely86 · 2017-03-22T11:08:43Z

@boegel done

wpoely86 · 2017-04-25T07:22:08Z

@boegel Can you have another look at this?

boegel · 2017-04-25T13:46:42Z

+from easybuild.tools.systemtools import get_shared_lib_ext
+
+# the namespace file for the R extension
+r_namespace = """# Export all names


constants should be all caps, so R_NAMESPACE or MXNET_R_NAMESPACE

boegel · 2017-04-25T13:47:26Z

+
+# Import all packages listed as Imports or Depends
+import(
+methods,


I'd just make this a single line? There's no big advantage here in spreading this vertically?

It's readable?

well, pfft, who cares in this case...

Also, I actually find it less readable almost :P

Indeed, who cares? Let's keep it like it is. Doesn't make any difference

boegel · 2017-04-25T13:52:52Z

+                    shutil.move(olddir, newdir)
+                except IOError, err:
+                    raise EasyBuildError("Failed to move %s to %s: %s", olddir, newdir, err)
+        except (IOError, OSError) as err:


why the nested try?

boegel · 2017-04-25T13:56:25Z

+        #   correctly filled and some mappings are done
+        # - Reinstal the exported version
+        self.r_ext.run()
+        run_cmd("R_LIBS=%s Rscript -e \"require(mxnet); mxnet:::mxnet.export(\\\"R-package\\\")\"" % self.installdir)


wrap the cmd in single quotes to avoid the escaping (a least partially)?

If you really insist but it was a pain to get working and I'm sure this one works.

boegel · 2017-04-25T13:57:05Z

+            raise EasyBuildError("Loading fake module failed: %s", err)
+
+        if not self.py_ext.sanity_check_step():
+            raise EasyBuildError("The sanity check of the Python bindings failed")


s/of/for/?

boegel · 2017-04-25T13:59:14Z

+            fullpath = os.path.join(self.installdir, path)
+            # only extend $PYTHONPATH with existing, non-empty directories
+            if os.path.exists(fullpath) and os.listdir(fullpath):
+                txt += self.module_generator.prepend_paths('PYTHONPATH', path)


this is just copy-pasting from the PythonPackage easyblock, we should hoist this into a separate method we can just call out to with txt += self.py_ext.prepend_pythonpath() or something?

or can we just all txt += self.py_ext.make_module_extra()?

tried that. It's copy&pasted for a reason.

boegel · 2017-04-25T14:00:11Z

+            if os.path.exists(fullpath) and os.listdir(fullpath):
+                txt += self.module_generator.prepend_paths('PYTHONPATH', path)
+
+        txt += self.module_generator.prepend_paths("R_LIBS", [''])  # prepend R_LIBS with install path


same here, call out to self.r_ext.make_module_extra() rather than copy-pasting?

boegel · 2017-06-22T11:15:31Z

tested with easybuilders/easybuild-easyconfigs#4765, good to go, thanks @wpoely86!

Added block for MXNet

d48ad18

wpoely86 commented Mar 14, 2017

View reviewed changes

Fix tests

78b0579

boegel added this to the 3.2.0 milestone Mar 14, 2017

wpoely86 mentioned this pull request Mar 14, 2017

{lang} [foss/2016b] MXNet/0.9.3 (REVIEW) easybuilders/easybuild-easyconfigs#4346

Closed

boegel requested changes Mar 14, 2017

View reviewed changes

wpoely86 added 7 commits March 14, 2017 12:29

Another try at fixing the tests

e82e807

Fix remarks

84ff7d2

Use change_dir

ff8be4b

Use correct namespace for R package of mxnet

74f0c01

Fix the R extension of MXNet

d7b5c8c

It's horrible!

Fix sanity check in mxnet

d2d05e3

boegel requested changes Mar 22, 2017

View reviewed changes

wpoely86 added 3 commits March 22, 2017 11:20

Fix remarks for mxnet

14e21f4

Change order of prepare_step in mxnet

8a406da

Pass options to make instead of patching config.mk

29ee356

boegel requested changes Apr 25, 2017

View reviewed changes

boegel reviewed Apr 25, 2017

View reviewed changes

boegel requested changes Apr 25, 2017

View reviewed changes

Fix remarks

6068b1b

boegel modified the milestones: 3.2.0, 3.3.0 May 9, 2017

boegel approved these changes Jun 22, 2017

View reviewed changes

wpoely86 mentioned this pull request Jun 22, 2017

{lang} [foss/2016b] MXNet/0.9.3 (REVIEW) easybuilders/easybuild-easyconfigs#4765

Merged

boegel merged commit 95e67a0 into easybuilders:develop Jun 22, 2017

wpoely86 deleted the mxnet branch June 22, 2017 11:16

Conversation

wpoely86 commented Mar 14, 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!

boegel Mar 14, 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!

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!

wpoely86 commented Mar 17, 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!

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!

wpoely86 Apr 25, 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!

wpoely86 commented Mar 14, 2017 •

edited

Loading

boegel Mar 14, 2017 •

edited

Loading

wpoely86 Apr 25, 2017 •

edited

Loading