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

`import sagemaker.tensorflow` is broken after upgrading TF serving #139

Closed
zmjjmz opened this Issue Apr 10, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@zmjjmz
Copy link

zmjjmz commented Apr 10, 2018

Hey there,

So I recently upgraded my local tensorflow serving version to uh, 1.5:

(dataplayground2) zach@wa1okdba002:~/stats/omc/utils$ apt-cache show tensorflow-model-server           
Package: tensorflow-model-server
Version: 1.5.0
Architecture: all
Maintainer: TensorFlow Serving team
Priority: optional
Section: contrib/devel
Filename: pool/tensorflow-model-server/t/tensorflow-model-server/tensorflow-model-server_1.5.0_all.deb
Size: 89631204
SHA256: 4f25bde0f8dad88ec5184d060e5e10d2c15f1241977f72e50d233d5f7ddb6fa6
SHA1: a20e4e9d242cf874b12289ba9a8fb3cc97da7df7
MD5sum: 8867dad0c1258813a8a75b46ef8df536
Description: TensorFlow Serving ModelServer
Description-md5: 9b7e03f5296f318009581d6e285e2f89
Homepage: https://github.com/tensorflow/serving
Built-Using: Bazel

And my tensorflow-serving-api package as well:

tensorflow-serving-api==1.6.0

After this upgrade, I realized I can't run import sagemaker.tensorflow in Python using the latest sagemaker SDK version (1.2.2) (also the version that I had installed before I realized I hadn't updated it and did so in the vain hope of fixing this issue).

The exact issue I get is as follows:

(dataplayground2) zach@wa1okdba002:~/stats/omc/utils$ python -c "import sagemaker.tensorflow"
/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/h5py/__init__.py:34: FutureWarning: Conversion of the second argum
ent of issubdtype from `float` to `np.floating` is deprecated. In future, it will be treated as `np.float64 == np.dtype(float).type`.
  from ._conv import register_converters as _register_converters
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/sagemaker/tensorflow/__init__.py", line 28, in <module>
    from sagemaker.tensorflow.estimator import TensorFlow  # noqa: E402
  File "/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/sagemaker/tensorflow/estimator.py", line 25, in <module>
    from sagemaker.tensorflow.model import TensorFlowModel
  File "/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/sagemaker/tensorflow/model.py", line 18, in <module>
    from sagemaker.tensorflow.predictor import tf_json_serializer, tf_json_deserializer
  File "/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/sagemaker/tensorflow/predictor.py", line 22, in <module>
    from tensorflow_serving.apis import predict_pb2, classification_pb2, inference_pb2, regression_pb2
  File "/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/sagemaker/tensorflow/tensorflow_serving/apis/inference_pb2
.py", line 16, in <module>
    from tensorflow_serving.apis import classification_pb2 as tensorflow__serving_dot_apis_dot_classification__pb2
  File "/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/tensorflow_serving/apis/classification_pb2.py", line 26, i
n <module>
    dependencies=[tensorflow__serving_dot_apis_dot_input__pb2.DESCRIPTOR,tensorflow__serving_dot_apis_dot_model__pb2.DESCRIPTOR,])
  File "/home/u1/zach/proj/dataplayground2/local/lib/python2.7/site-packages/google/protobuf/descriptor.py", line 829, in __new__
    return _message.default_pool.AddSerializedFile(serialized_pb)
TypeError: Couldn't build proto file into descriptor pool!
Invalid proto descriptor for file "tensorflow_serving/apis/classification.proto":
  tensorflow_serving/apis/classification.proto: A file with this name is already in the pool.

It seems to me like somehow it's trying to load that tensorflow_serving/apis/classification.proto twice, which is probably not intentional, although I'll note that I'm able to do this myself with no issues:

(dataplayground2) zach@wa1okdba002:~/stats/omc/utils$ python -c "from tensorflow_serving.apis import classification_pb2; from tensorflow_serving.apis import classification_pb2"
(dataplayground2) zach@wa1okdba002:~/stats/omc/utils$

At the moment this is entirely preventing me from using SageMaker through the Python SDK which is certainly a problem...

@zmjjmz

This comment has been minimized.

Copy link
Author

zmjjmz commented Apr 11, 2018

Quick update: I tried this out a few times and can only replicate it with tensorflow-serving-api==1.6.0, lower versions do not cause this issue. I'm unsure at this point if this is an issue w/the sagemaker package or the tensorflow-serving-api package, but it seems to me like sagemaker might have been relying on deprecated / undocumented behavior that broke?

@lukmis

This comment has been minimized.

Copy link
Contributor

lukmis commented Apr 30, 2018

Hi zmjjmz,

sorry for a long delay. Indeed I observe the same behavior. The reason behind this is that Python SDK has copies of the tensorflow-serving protobuf files (under src/sagemaker/tensorflow/tensorflow_serving). Version 1.6.0 has changed these files potentially making them back-incompatible from previous versions.

When you try to import from from tensorflow_serving.apis outside of the SDK it goes to the installed packages and works.

This will be analyzed and addressed by the team. Thank you for reporting this!

@mvsusp

This comment has been minimized.

Copy link
Contributor

mvsusp commented May 11, 2018

Hi @zmjjmz,

I investigated the issue today and got to the following conclusions:

1 - tensorflow-serving-api is only available in python 2 and cannot be installed using python 3. TensorFlow serving is not planning to create a python 3 version (tensorflow/serving#700) although the content of these versions would be the same. That is the main reason for us to maintain a copy of the protobuf messages inside SageMaker Python SDK.

2 - The issue occurs because Python 2 will prioritize loading the system installed module instead of the relative module version. That issue is solved adding future absolute import in any file that loads the reference. I enforced absolute imports in SageMaker to avoid it happening again: #180

3 - I have an additional PR updating the TF serving protobuf messages #181

I will merge these important changes as soon as possible to unblock you.

I appreciate you alerting us the issue and thanks for the patience.

Best,

Marcio

@mvsusp

This comment has been minimized.

Copy link
Contributor

mvsusp commented May 11, 2018

Hi @zmjjmz

The fix is merged in master. I will release the new version in pypi Monday.

Thanks for the patience.

@mvsusp

This comment has been minimized.

Copy link
Contributor

mvsusp commented May 31, 2018

Hi @zmjjmz

The release https://github.com/aws/sagemaker-python-sdk/releases/tag/v1.2.5 includes the fixes for the issue.

Thanks again for reporting this issue.

@mvsusp mvsusp closed this May 31, 2018

apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this issue Nov 15, 2018

Merge pull request aws#139 from channy/master
Fix some typos including Python SDDK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.