Page MenuHomePhabricator

Guard `find_library(tensorflow_c_api ...)` by checking for TENSORFLOW_C_LIB_PATH to be set by the user
ClosedPublic

Authored by mehdi_amini on Sep 26 2020, 8:21 PM.

Details

Summary

Also have CMake fails if the user provides a TENSORFLOW_C_LIB_PATH but
we can't find TensorFlow at this path.

At the moment the CMake script tries to figure if TensorFlow is
available on the system and enables support for it. This is in general
not desirable to customize build features this way and instead it is
preferable to let the user opt-in explicitly into the features they want
to enable. This is in line with other optional external dependencies
like Z3.
There are a few reasons to this but amongst others:

  • reproducibility: making features "magically" enabled based on whether we find a package on the system or not makes it harder to handle bug reports from users.
  • user control: they can't have TensorFlow on the system and build LLVM without TensorFlow right now. They also would suddenly distribute LLVM with a different set of features unknowingly just because their build machine environment would change subtly.

Right now this is motivated by a user reporting build failures on their system:

.../mesa-git/llvm-git/src/llvm-project/llvm/lib/Analysis/TFUtils.cpp:23:10: fatal error: tensorflow/c/c_api.h: No such file or directory

23 | #include "tensorflow/c/c_api.h"
   |          ^~~~~~

It looks like we detected TensorFlow at configure time but couldn't set all the paths correctly.

Diff Detail

Event Timeline

mehdi_amini created this revision.Sep 26 2020, 8:21 PM
mehdi_amini requested review of this revision.Sep 26 2020, 8:21 PM
mehdi_amini edited the summary of this revision. (Show Details)Sep 26 2020, 8:26 PM

There are 2 types of TF dependencies, orthogonal from each other, controlled by TENSORFLOW_AOT_PATH and TENSORFLOW_C_LIB_PATH, respectively.

The former should already provide the desired behavior.

The latter is the problem, due to find_library behavior (line 840 old code): I think it's missing a NO_DEFAULT_PATH, which should then result in the desired behavior.

mehdi_amini added a comment.EditedSep 28 2020, 1:46 PM

There are 2 types of TF dependencies, orthogonal from each other, controlled by TENSORFLOW_AOT_PATH and TENSORFLOW_C_LIB_PATH, respectively.

The former should already provide the desired behavior.

Right!

The latter is the problem, due to find_library behavior (line 840 old code): I think it's missing a NO_DEFAULT_PATH, which should then result in the desired behavior.

I don't think we should invoke find_library without an opt-in from the user, this is what this patch is addressing here (see how Z3 is setup for example)

mehdi_amini retitled this revision from Introduce a LLVM_ENABLE_TENSORFLOW CMake option for users to opt-in instead of silently trying to enable it to Guard `find_library(tensorflow_c_api ...)` by checking for TENSORFLOW_C_LIB_PATH to be set by the user.
mehdi_amini edited the summary of this revision. (Show Details)

Remove the new option and rely on TENSORFLOW_C_LIB_PATH to be set instead

mtrofin accepted this revision.Sep 28 2020, 2:06 PM

lgtm, thanks for doing this!

This revision is now accepted and ready to land.Sep 28 2020, 2:06 PM