Skip to content

Conversation

@jameswex
Copy link
Contributor

@jameswex jameswex commented Jun 5, 2019

  • Motivation for features / changes

In WIT, models that accept JSON as input were failing in python 3 notebooks due to how bytes_list protos are handled.

  • Technical description of changes

Added wrapper method for handling of objects that in py3 are bytes but in py2 are string (such as bytes_list handling from tf Example proto) to ensure they encoded as strings for use in WIT.

  • Screenshots of UI changes

N/A

  • Detailed steps to verify changes work correctly (as executed by you)

Ran py3 colab notebook with the offending model with local pip package built with this change and verified that it now works correctly in WIT.
Ran other colab notebooks to verify it didn't cause regressions.

@jameswex
Copy link
Contributor Author

jameswex commented Jun 5, 2019

@tolga-b please review

@jameswex jameswex requested a review from stephanwlee June 5, 2019 16:11
@stephanwlee stephanwlee requested review from nfelt and removed request for stephanwlee June 5, 2019 16:20
@stephanwlee
Copy link
Contributor

This roughly looks like something that six probably addresses. Since I lack profound knowledge, I assigned the PR to @nfelt for review.

@tolga-b
Copy link
Contributor

tolga-b commented Jun 5, 2019 via email

@jameswex
Copy link
Contributor Author

jameswex commented Jun 6, 2019

switched to six.ensure_str - thanks

from IPython import display
from google.protobuf import json_format
from numbers import Number
from six import ensure_str
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to add six dependency on the BUILD file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


import base64
import json
import googleapiclient.discovery
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably do this as a separate PR but how was this previously working? GAPI isn't part of our dependency declaration and I believe we didn't do anything special to install it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

acknowledged

@jameswex jameswex merged commit 46c510d into tensorflow:master Jun 6, 2019
@jameswex jameswex deleted the py3aip-solo branch June 6, 2019 15:37
@tolga-b tolga-b mentioned this pull request Sep 19, 2019
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.

3 participants