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

Fix long type not found for json input in python3 #2269

Merged
merged 2 commits into from
May 23, 2019

Conversation

tolga-b
Copy link
Contributor

@tolga-b tolga-b commented May 22, 2019

  • Motivation for features / changes
    long type does not exist in python 3, changed type testing to six.integer types from long to prevent errors when wit is used with json inputs.

  • Technical description of changes

  • Screenshots of UI changes
    None

  • Detailed steps to verify changes work correctly (as executed by you)
    Tested cloud xgboost model with python 3 kernel in colab.

  • Alternate designs / implementations considered
    None

@tolga-b
Copy link
Contributor Author

tolga-b commented May 22, 2019

@jameswex

@tolga-b
Copy link
Contributor Author

tolga-b commented May 22, 2019

@stephanwlee please review.

@jameswex jameswex requested a review from stephanwlee May 22, 2019 20:27
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

The change looks straightforward but I am surprised by lack of python deps in http://go/gh/tensorflow/tensorboard/blob/master/tensorboard/plugins/interactive_inference/witwidget/BUILD.

How does it work? (if it works because of transitive deps, wouldn't it be brittle?)

@tolga-b
Copy link
Contributor Author

tolga-b commented May 23, 2019

You are right. We realized that BUILD file for witwidget is missing a lot of dependencies and it mainly works because the pip package depends on tensorflow which has them. We will update the witwidget BUILD file in a separate PR.

@jameswex jameswex merged commit b3546cc into tensorflow:master May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants