Enabled filtering of paths in the CPATH variable generated for the wrapper by damianam · Pull Request #1436 · easybuilders/easybuild-easyblocks

damianam · 2018-06-13T10:21:18Z

As we know, bazel pulls a lot of stuff on its own. In the existing setup, these versions might conflict with packages already installed and included in the CPATH variable generated for the icc wrapper. This PR enables package filtering there. What this does not do is filtering CPATH when using gcc-based toolchains. Input? Should we extend the filter to work also under other toolchains where a wrapper is not generated?

…apper

houndci-bot · 2018-06-13T10:21:23Z

easybuild/easyblocks/t/tensorflow.py

+            # filter out paths from CPATH
+            wrapper_filter = self.cfg['wrapper_filter']
+            cpath = os.getenv('CPATH').split(':')
+            filtered_cpath = [path for fil in wrapper_filter for path in cpath if not fil in path]


test for membership should be 'not in'

boegel · 2018-06-13T11:28:28Z

@damianam Do you have an example of how you use this, and which exact problem/errors it fixes?

damianam · 2018-06-13T11:59:42Z

We have protobuf pulled in as a dependency in our Python module. The version that tensorflow pulls in is incompatible with ours (slightly different API). So via CPATH our protobuf leaks into the bazel setup and compilation fails. Setting wrapper_filter = ['protobuf'] in the easyconfig fixes it.

boegel · 2018-06-14T13:21:11Z

@akesandgren Thoughts on this?

akesandgren · 2018-06-14T13:51:00Z

It should do the same regardless of toolchain used or people will get confused
But wouldn't you need to filter LD_LIBRARY_PATHS too then?

damianam · 2018-06-14T14:45:59Z

Agreed. We should filter CPATH in gcc toolchains too. Seems logical.

Regarding LD_LIBRARY_PATH, I don't think so. That is a runtime environment variable. LIBRARY_PATH on the other hand......

…e filtering

houndci-bot · 2018-06-14T15:38:40Z

easybuild/easyblocks/t/tensorflow.py

            icc_wrapper_txt = INTEL_COMPILER_WRAPPER % {
                'compiler_path': which('icc'),
-                'cpath': os.getenv('CPATH'),
+                'cpath': cpath,


undefined name 'cpath'

… paths

boegel · 2018-06-15T14:13:49Z

easybuild/easyblocks/t/tensorflow.py

+        package_filter = self.cfg['package_filter']
+        for var in ['CPATH', 'LIBRARY_PATH']:
+            path = os.getenv(var).split(':')
+            filtered_path = [p for fil in package_filter for p in path if fil not in p]


@damianam Please log the value of the environment variable before and after filtering.

boegel · 2018-06-15T14:14:46Z

easybuild/easyblocks/t/tensorflow.py

        extra_vars = {
            # see https://developer.nvidia.com/cuda-gpus
            'cuda_compute_capabilities': [[], "List of CUDA compute capabilities to build with", CUSTOM],
+            'package_filter': [[], "List of paths to filter out", CUSTOM],


It's actually a list of filter patterns for paths (since you're checking with if fil not in p), please clarify that and maybe also rename to cpath_libpath_filter_patterns to make it more explicit?

boegel · 2018-06-15T14:15:05Z

easybuild/easyblocks/t/tensorflow.py


        tmpdir = tempfile.mkdtemp(suffix='-bazel-configure')

+        # filter out paths from CPATH and LIBRARY_PATH


please add an extra comment line or two explaining why this may be needed

boegel · 2018-06-15T14:16:51Z

easybuild/easyblocks/t/tensorflow.py

+        for var in ['CPATH', 'LIBRARY_PATH']:
+            path = os.getenv(var).split(':')
+            filtered_path = [p for fil in package_filter for p in path if fil not in p]
+            os.environ[var] = ':'.join(filtered_path)


please use setvar from easybuild.tools.environment (which logs), and use os.pathsep

setvar(var, os.pathsep.join(filtered_path))

…d why

houndci-bot · 2018-06-22T14:47:02Z

easybuild/easyblocks/t/tensorflow.py

+                self.log.info("$%s old value was %s" % (var, path))
+                filtered_path = os.pathsep.join([p for fil in path_filter for p in path if fil not in p])
+                self.log.info("$%s new value is %s" % (var, filtered_path))
+                setvar(var, filtered_path)


undefined name 'setvar'

damianam · 2018-07-04T14:18:48Z

@boegel @akesandgren please review again.

akesandgren · 2018-07-04T15:01:28Z

Code looks sane, though I don't know what one would put in the filter. (Literally that is)

damianam · 2018-07-04T15:18:09Z

Our easyconfig has this: package_filter = ['protobuf']. Without it the protobuf package we have included as part of the other stuff that gets pulled in by Python leaks in and breaks the compilation.

boegel · 2018-07-04T15:23:31Z

easybuild/easyblocks/t/tensorflow.py

+        # might conflict with dependencies on the system and/or installed with EB. For example: protobuf
+        path_filter = self.cfg['path_filter']
+        if len(path_filter > 0):
+            self.log.info("Filtering $CPATH and $LIBRARY_PATH")


@damianam Please change to:

if path_filter: self.log.info("Filtering $CPATH and $LIBRARY_PATH with path filter %s", path_filter)

boegel · 2018-07-04T15:24:36Z

easybuild/easyblocks/t/tensorflow.py

+                path = os.getenv(var).split(':')
+                self.log.info("$%s old value was %s" % (var, path))
+                filtered_path = os.pathsep.join([p for fil in path_filter for p in path if fil not in p])
+                self.log.info("$%s new value is %s" % (var, filtered_path))


@damianam env.setvar already logs both new & old value, so this extra logging is overkill?

boegel · 2018-07-04T15:25:14Z

easybuild/easyblocks/t/tensorflow.py

+        if len(path_filter > 0):
+            self.log.info("Filtering $CPATH and $LIBRARY_PATH")
+            for var in ['CPATH', 'LIBRARY_PATH']:
+                path = os.getenv(var).split(':')


Please also use os.pathsep here, like you do below:

path = os.getenv(var).split(os.pathsep)

boegel · 2018-07-05T17:49:53Z

Reviewed & tested with an existing TensorFlow easyconfig, lgtm, thanks @damianam!

Enabled filtering of paths in the CPATH variable generated for the wr…

bdb5964

…apper

damianam added the enhancement label Jun 13, 2018

damianam added this to the 3.6.2 milestone Jun 13, 2018

houndci-bot reviewed Jun 13, 2018

View reviewed changes

Removed paths also for any toolchain, and included LIBRARY_PATH in th…

90a98fc

…e filtering

houndci-bot reviewed Jun 14, 2018

View reviewed changes

Damian Alvarez added 2 commits June 14, 2018 17:40

Fixed non defined cpath variable

043d756

Fixed bug that created a list of lists, instead of a list of filtered…

0a54a16

… paths

boegel requested changes Jun 15, 2018

View reviewed changes

Added logging, and some extra explanation of what path_filter does an…

1f4c7e5

…d why

houndci-bot reviewed Jun 22, 2018

View reviewed changes

Fixed undefined var

d9bdb3f

damianam changed the title ~~Enabled filtering of paths in the CPATH variable generated for the wrapper (WIP)~~ Enabled filtering of paths in the CPATH variable generated for the wrapper Jul 4, 2018

boegel requested changes Jul 4, 2018

View reviewed changes

Style fixes

6b46635

boegel approved these changes Jul 5, 2018

View reviewed changes

boegel merged commit df5fe5d into easybuilders:develop Jul 5, 2018

pavelToman mentioned this pull request Sep 23, 2025

{lib}[foss/2024a] TensorFlow v2.18.1 w/ CUDA 12.6.0 easybuilders/easybuild-easyconfigs#22921

Merged


		tmpdir = tempfile.mkdtemp(suffix='-bazel-configure')

		# filter out paths from CPATH and LIBRARY_PATH

Conversation

damianam commented Jun 13, 2018 • 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 commented Jun 13, 2018

Uh oh!

damianam commented Jun 13, 2018

Uh oh!

boegel commented Jun 14, 2018

Uh oh!

akesandgren commented Jun 14, 2018

Uh oh!

damianam commented Jun 14, 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!

damianam commented Jul 4, 2018

Uh oh!

akesandgren commented Jul 4, 2018

Uh oh!

damianam commented Jul 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!

boegel commented Jul 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

damianam commented Jun 13, 2018 •

edited

Loading