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

Extra python dependencies #942

Merged
merged 31 commits into from
Oct 22, 2019

Conversation

adriangonz
Copy link
Contributor

This PR partly addresses #564

Changes

  • tensorflow is now an extra package, which can be installed as
    $ pip install seldon_core[tensorflow]
  • If tensorflow is not present, the tftensor data type won't be usable. This is because the tftensor type still requires a couple of tensorflow methods: tf.make_ndarray() and tf.make_tensor_proto().
  • Our seldon_core.proto.prediction_pb2 protobuf still needs the definitions for the tensorflow tensor protobuffs:
    from google.protobuf import struct_pb2 as google_dot_protobuf_dot_struct__pb2
    from tensorflow.core.framework import tensor_pb2 as tensorflow_dot_core_dot_framework_dot_tensor__pb2

    As a workaround, if tensorflow is present, then we don't do anything and we just use the definitions from tensorflow.core.framework.tensor_pb2. However, if tensorflow is not present, we patch that import path on seldon_core/proto/__init__.py so that it points to our own compiled tensorflow.core.framework.tensor_pb2 protobuffs. Patching syspath is not very clean and can cause problems down the line. I'm still not 100% sure it's a good choice to do this. Suggestions are more than welcome.
  • The Makefile now compiles the tensorflow.core.framework.tensor_pb2 protobuffs in case we need to patch the import path.
  • Pillow has also been moved to the test dependencies, since it's only used on the tests and it's a pretty heavy dependency.

Notes

As mentioned above, the solution we have implemented is patching syspath to point to our own set of pre-compiled tensorflow.core.framework.tensor_pb2 protobuffs when tensorflow is not present. This is required because our own seldon_core.proto.prediction_pb2 has a hard dependency on them.

A different approach we tried was to keep our set of pre-compiled tensorflow protobuffs on a different package which doesn't shadow the tensorflow.core.framework.tensor_pb2 import path (e.g. a new tf_proto package). However, if tensorflow is present, it will also try to import its own set of protobuffs and these will then conflict since they have the same ID.

Renaming the package of our pre-compiled tensorflow.core.framework.tensor_pb2 protobuffs to avoid this conflict doesn't help either, since in that case the tf.make_ndarray() and tf.make_tensor_proto() methods stop working because the types are different.

Before moving forward, we should consider the pros and cons of overriding syspath to make tensorflow an optional dependency and perhaps evaluate different alternatives (like writing our own make_ndarray and make_tensor_proto methods and getting rid of the tensorflow dep altogether).

@axsaucedo
Copy link
Contributor

/uncc @gipster

@seldondev seldondev removed the request for review from gipster October 11, 2019 10:27
@adriangonz
Copy link
Contributor Author

/assign @axsaucedo @cliveseldon

@axsaucedo
Copy link
Contributor

/cc @jklaise @cliveseldon

@adriangonz
Copy link
Contributor Author

@jklaise @cliveseldon @axsaucedo This solution to make the tensorflow dep optional is a bit hacky. Before moving forward, I would love to get your thoughts on this.

@axsaucedo
Copy link
Contributor

Perhaps the changes in the JX file had this effect? You could test by reverting the java piece, but perhaps it's the server

@adriangonz
Copy link
Contributor Author

@axsaucedo I think that one doesn't affect the tests. I've seen the binding error on the Java logs before. I haven't digged into it, but it doesn't make the tests fail.

In this case it failed because an error on the python tests:

_____________________ ERROR collecting tests/test_utils.py _____________________
Traceback (most recent call last):
  File "/workspace/source/python/seldon_core/proto/tensorflow/core/framework/__init__.py", line 8, in <module>
    from tensorflow.core.framework import (
ModuleNotFoundError: No module named 'tensorflow'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/workspace/source/python/tests/test_utils.py", line 6, in <module>
    import seldon_core.utils as scu
  File "/workspace/source/python/seldon_core/utils.py", line 11, in <module>
    from seldon_core.proto import prediction_pb2
  File "/workspace/source/python/seldon_core/proto/prediction_pb2.py", line 17, in <module>
    from seldon_core.proto.tensorflow.core.framework import tensor_pb2 as tensorflow_dot_core_dot_framework_dot_tensor__pb2
  File "/workspace/source/python/seldon_core/proto/tensorflow/core/framework/__init__.py", line 18, in <module>
    logger.notice(notice)
AttributeError: 'Logger' object has no attribute 'notice'

I was using logger.notice() for some unknown reason, which doesn't exist. The thing is that I remember testing that as logger.info(). I've changed that, but even then I'm expecting it to fail since there are a couple files which I haven't committed yet which should make the build fail when tensorflow is not present.

It's been a really good idea to make Jenkins X test the build path without tensorflow!

@seldondev seldondev added size/XXL and removed size/L labels Oct 21, 2019
@adriangonz
Copy link
Contributor Author

@axsaucedo @cliveseldon the tests are now added and fixed. I had to change how I was checking if tensorflow is present because of a bug in v1.14.0 which makes tensorflow incompatible with importlib.

I've also removed a few no-longer used lines on the .gitignore file and added the pre-built TF protobuff interfaces (which were being ignored).

@axsaucedo
Copy link
Contributor

Looks good from my side. @cliveseldon are we good for landing?

@jklaise
Copy link
Contributor

jklaise commented Oct 22, 2019

/lgtm

@axsaucedo
Copy link
Contributor

/lgtm

@axsaucedo
Copy link
Contributor

/hold

@ukclivecox
Copy link
Contributor

/approve

@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: axsaucedo, cliveseldon, jklaise

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [axsaucedo,cliveseldon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@axsaucedo
Copy link
Contributor

/hold remove

@seldondev seldondev merged commit c73fb85 into SeldonIO:master Oct 22, 2019
@adriangonz adriangonz deleted the extra-python-dependencies branch October 22, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants