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

Tensorflow-cpu is ignored when installing tensorflow-addons #864

Closed
gabrieldemarmiesse opened this issue Jan 12, 2020 · 16 comments · Fixed by #974
Closed

Tensorflow-cpu is ignored when installing tensorflow-addons #864

gabrieldemarmiesse opened this issue Jan 12, 2020 · 16 comments · Fixed by #974
Labels

Comments

@gabrieldemarmiesse
Copy link
Member

System information

  • OS Platform and Distribution (e.g., Linux Ubuntu 16.04): ubuntu 18.04
  • TensorFlow version and how it was installed (source or binary): tensorflow-cpu 2.1.0
  • TensorFlow-Addons version and how it was installed (source or binary): 0.7.0
  • Python version: 3.7
  • Is GPU used? (yes/no): no

Describe the bug

In an environement where tensorflow-cpu is already installed, installing tensorflow-addons should not download and install tensorflow (now being the gpu version by default).

Code to reproduce the issue

FROM python:3.7

RUN pip install tensorflow-cpu
RUN pip install tensorflow-addons
RUN pip freeze | grep tensorflow

result:

tensorflow (gpu) is still being installed (400MB download) and the installation of tensorflow is now duplicated:

tensorflow==2.1.0
tensorflow-addons==0.7.0
tensorflow-cpu==2.1.0
tensorflow-estimator==2.1.0

A solution would be to remove the tensorflow dependency in the setup.py and let people install the version of tensorflow they want. It's not a perfect solution but this is the best one we found, at least for keras-tuner and autokeras. (keras-team/keras-tuner#211 and keras-team/autokeras#860)

The problem is similar to libraries that depend on Pillow and users have already pillow-SIMD installed.

@seanpmorgan
Copy link
Member

Hi @gabrieldemarmiesse this is a good point and previously we used to just do a TF version check during import. Since https://github.com/tensorflow/community/blob/master/rfcs/20190816-tf-project-versioning.md#versioning-policy however we moved toward pip dependencies.

This is certainly a problem, but now that tensorflow package for TF 1.15 and TF 2.1 supports CPU/GPU I'm hoping that tensorflow-cpu usage will significantly drop off.

It looks as though tensorflow-cpu is orders of magnitude less common than tensorflow (they used to be the same package):
https://pypistats.org/packages/tensorflow
https://pypistats.org/packages/tensorflow-cpu

@gabrieldemarmiesse
Copy link
Member Author

I believe that if we don't support tensorflow-cpu, we are forgetting a lot of users who don't have access to a high bandwidth.

I live in France, in Paris, and I don't have fiber. It takes me 20 minutes to download tensorflow and 5 minutes to download tensorflow-cpu.

I'm worried about people in developing countries who dont have the luxury of having a connection as fast as mine. We're excuding them if we don't support tensorflow-cpu.

@seanpmorgan
Copy link
Member

I believe that if we don't support tensorflow-cpu, we are forgetting a lot of users who don't have access to a high bandwidth.

I live in France, in Paris, and I don't have fiber. It takes me 20 minutes to download tensorflow and 5 minutes to download tensorflow-cpu.

I'm worried about people in developing countries who dont have the luxury of having a connection as fast as mine. We're excuding them if we don't support tensorflow-cpu.

That's a valid point I haven't considered. So a simple way to get around this is pip install tensorflow-addons --no-deps and it'll work perfectly fine using tensorflow-cpu. I'm not sure if documentation on the README would be sufficient for most users to find that info though.

@gabrieldemarmiesse
Copy link
Member Author

gabrieldemarmiesse commented Jan 13, 2020 via email

@gabrieldemarmiesse
Copy link
Member Author

Other problem that we have with tensorflow-cpu:

FROM python:3.7

RUN pip install tensorflow-cpu
RUN pip install --no-deps tensorflow-addons
RUN python -c 'import tensorflow_addons'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/lib/python3.7/site-packages/tensorflow_addons/__init__.py", line 21, in <module>
    from tensorflow_addons import activations
  File "/usr/local/lib/python3.7/site-packages/tensorflow_addons/activations/__init__.py", line 21, in <module>
    from tensorflow_addons.activations.gelu import gelu
  File "/usr/local/lib/python3.7/site-packages/tensorflow_addons/activations/gelu.py", line 24, in <module>
    get_path_to_datafile("custom_ops/activations/_activation_ops.so"))
  File "/usr/local/lib/python3.7/site-packages/tensorflow_core/python/framework/load_library.py", line 57, in load_op_library
    lib_handle = py_tf.TF_LoadLibrary(library_filename)
tensorflow.python.framework.errors_impl.NotFoundError: /usr/local/lib/python3.7/site-packages/tensorflow_addons/custom_ops/activations/_activation_ops.so: undefined symbol: __cudaPushCallConfiguration

This could definitly benefit from #855 being fixed. I'll make a PR.

@seanpmorgan
Copy link
Member

Yes good point indeed, we surely don't want people to blindly do "pip
install tensorflow-addons" and then think that they're stuck with
downloading the gpu version.
We might not have to change the setup.py as you said. I'll let you decide
what's the best move on this one.

So one thing we can do is to make the dependency an extra similar to how TF Graphics is doing it:
https://github.com/tensorflow/graphics/blob/master/setup.py#L69

If we do that then we'll want to add the import check again:
463f5f8#diff-ee7613fc7cd655fc8da0aa8a0838c49cL30

I'm okay with this change @tensorflow/sig-addons-maintainers do you have an issues?

@guillaumekln
Copy link
Contributor

An alternative is to release a tensorflow-addons-cpu package similarly to tensorflow-cpu. However, this sounds like a lot of maintenance work.

I'm fine with using the extra dependency as it gives more flexibility.

@foxik
Copy link

foxik commented Jan 15, 2020

I believe working with tensorflow-cpu is an important feature. Many models still work fine on CPU during inference -- but if they use tensorflow_addons, a GPU tensorflow version is required, which increase download and storage requirements.

Our use case are a) a teaching evaluation server which evaluates code from students; b) virtual images (including docker images) of various web services. Right now we do not use tf_addons on them, but if tf_addons worked with tensorflow_cpu, we would.

@foxik
Copy link

foxik commented Jan 15, 2020

BTW, neither tensorflow_datasets nor tensorflow_probability has an explicit dependency on tensorflow, which allows them to work either with CPU or GPU version.

@seanpmorgan
Copy link
Member

seanpmorgan commented Jan 23, 2020

I'd be happy to review a PR which follows suite with TF-Graphics in requiring TF as an extra dependency (for easy test suite installs). We'll also need to perform an import check during __init__ as described above.

@seanpmorgan
Copy link
Member

TFA Nightlies starting tomorrow will have no pip dependency. 0.8 release is upcoming and will also be dependency free

@gabrieldemarmiesse
Copy link
Member Author

Well, for completeness, we do have typeguard as dependency

@seanpmorgan
Copy link
Member

Well, for completeness, we do have typeguard as dependency

Ah yes this is true.. but resolves TF dependency issue 😄

@gabrieldemarmiesse
Copy link
Member Author

For sure. Should we open an issue to track the progress on loading SOs with tensorflow cpu?

@seanpmorgan
Copy link
Member

For sure. Should we open an issue to track the progress on loading SOs with tensorflow cpu?

Yeah that'd be great. I have a couple of other issues I'll put in later today as well.

@gabrieldemarmiesse
Copy link
Member Author

I'll let you do it if you don't mind

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants