Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Devices and external libraries options, make install, Makefile fix #4

Closed
wants to merge 5 commits into from

Conversation

nicolaspi
Copy link

  • New features that allow to generate code according to operator's devices compatibility (cpu and/or gpu), and to specify external libraries to be linked.
  • The Makefile has been fixed to work with the latest tensorflow version (tested on the version 1.3 built from source).
  • Added 'make install' that copies the built library to the tensorflow's library directory.

and external library links. (see "get_slide_patch.yml" example)
- Fix Makefile
- Added an install option in the Makefile that copies the user's lib in
the tensorflow's lib directory
- Fix unit test template.
@nicolaspi
Copy link
Author

Looks like a version issue with tensorflow, the linker does not find libtensorflow_framework.so which was introduced by this commit (19 days ago).

@sjperkins
Copy link
Owner

Thanks for the PR. It looks like the new libtensorflow_framework.so linking style is going into tensorflow 1.4.0 (see tensorflow/tensorflow#13607 for e.g.). It would probably be best to wait for this release before merging this and releasing a new tfopgen.

@sjperkins sjperkins self-requested a review October 13, 2017 12:36
@@ -8,7 +8,7 @@

def make_template_kwargs(op_name, py_op_name,
project, library, op_inputs, op_outputs,
op_type_attrs, op_other_attrs, op_doc):
op_type_attrs, op_other_attrs, op_doc, op_libs, op_devices_comps, op_abi):
Copy link
Owner

Choose a reason for hiding this comment

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

Move op_libs, op_devices_comps, op_abi to a newline.

- "gpu"
extra_libraries_deps:
- "m"
use_old_abi: yes

Copy link
Owner

Choose a reason for hiding this comment

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

I think if we start mixing the Makefile options with the operator options, the yaml will start to be cluttered.

How about a makefile section to distinguish it from the rest of the yaml?

makefile:
    use_old_abi: yes
    libraries:
       - "m"

@@ -73,6 +75,12 @@ For example, we can define the outline for a ``ComplexPhase`` operator in the ``
(2) of (L, M) sky coordinates with shape (nsrc, 2)
(3) of frequencies,
compute the complex phase with shape (nsrc, ntime, nbl, nchan)
devices_compatibilities:
Copy link
Owner

Choose a reason for hiding this comment

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

Rename devices_compatibilities to devices

@@ -49,6 +49,8 @@ The user should provide a YAML configuration file defining the operator:
- polymorphic type attributes.
- other attributes.
- documentation.
- devices compatibilities.
- optional external libraries.
Copy link
Owner

Choose a reason for hiding this comment

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

device compatibilities to devices
optional external libraries to makefile options

@@ -147,6 +155,33 @@ generator code as the range of attribute behaviour is complex.
op_other_attrs:
- "iterations: int32 >= 2",

Sources codes will be generated according to the specified devices.
Copy link
Owner

Choose a reason for hiding this comment

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

Source code will be generated for the requested devices

devices_compatibilities:
- "cpu"
- "gpu"
extra_libraries_deps:
Copy link
Owner

Choose a reason for hiding this comment

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

extra_library_deps to libraries

extra_libraries_deps:
- "openslide"
devices_compatibilities:
- "cpu"
Copy link
Owner

Choose a reason for hiding this comment

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

Update with makefile section

@@ -24,7 +24,7 @@ class Test{{op_name}}(unittest.TestCase):
{% set comma = last_joiner(",") -%}
type_permutations = [
{%- for perm in op_np_type_perms %}
{{ perm | replace("'", "") | indent(4, True) }}{{comma(loop.last)}}
{{ perm[0] | replace("'", "") | indent(4, True) }}{{comma(loop.last)}}
Copy link
Owner

Choose a reason for hiding this comment

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

What does this do?

The binary pip packages available on the TensorFlow website are built
with gcc4 that uses the older ABI. Set the option ``use_old_abi`` with the value ``yes``
to allow the library to be compatible with the older abi.

Copy link
Owner

Choose a reason for hiding this comment

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

Change to:

Note that gcc version 5 and above uses a new C++ ABI.
However, the official tensorflow binary pip packages are built with gcc 4
which uses an older ABI. Set the option `use_old_abi: yes` to make
the library compatible with the older ABI.

CPPFLAGS=-std=c++11 $(TF_FLAGS) $(INCLUDES) -fPIC -fopenmp -O2 -march=native -mtune=native
NVCCFLAGS=-std=c++11 -D GOOGLE_CUDA=$(TF_CUDA) $(TF_FLAGS) $(INCLUDES) \
-x cu --compiler-options "-fPIC" --gpu-architecture=sm_30 -lineinfo

LDFLAGS = -fPIC -fopenmp
LDFLAGS = -fPIC -fopenmp -L$(TF_LIB) -ltensorflow_framework{{extra_libs}}

Copy link
Owner

Choose a reason for hiding this comment

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

One possible way of handling the the presence or absence of the -ltensorflow_framework library would be to add tensorflow_version to the YAML.

@nicolaspi nicolaspi closed this Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants