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

add Python3 support #6

Merged
merged 1 commit into from
Sep 16, 2019
Merged

add Python3 support #6

merged 1 commit into from
Sep 16, 2019

Conversation

voidism
Copy link
Contributor

@voidism voidism commented Sep 12, 2019

Detect the Python version at the running time and handle the cases that will raise errors in Python3.

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @voidism to sign the Salesforce.com Contributor License Agreement.

Copy link

@peterldowns peterldowns left a comment

Choose a reason for hiding this comment

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

You should use six to do version-dependent things.

@@ -14,6 +14,9 @@
from tensorflow.python.ops import math_ops
from tensorflow.python.ops import embedding_ops
import fastBPE
import platform

use_py3 = platform.python_version()[0] == '3'

Choose a reason for hiding this comment

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

You should use six to check python version.

@@ -159,7 +162,7 @@ def serving_input_fn():
topk = args.topk

while True:
prompt = raw_input('ENTER PROMPT: ')
prompt = raw_input('ENTER PROMPT: ') if not use_py3 else input('ENTER PROMPT: ')

Choose a reason for hiding this comment

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

You can import a consistently named input function with six:

from six.moves import input
prompt = input('ENTER PROMPT: ')

@@ -14,6 +14,9 @@
from tensorflow.python.ops import math_ops
from tensorflow.python.ops import embedding_ops
import fastBPE
import platform

use_py3 = platform.python_version()[0] == '3'

Choose a reason for hiding this comment

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

see comment on the same code in generation.py above.

@@ -150,7 +153,7 @@ def serving_input_fn():


while True:
_prompt = raw_input('ENTER PROMPT: ')
_prompt = raw_input('ENTER PROMPT: ') if not use_py3 else input('ENTER PROMPT: ')

Choose a reason for hiding this comment

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

see comment on the same code in generation.py above.

@@ -27,7 +30,7 @@
np.random.seed(args.seed)

# load the vocabulary from file
vocab = open('vocab').read().decode(encoding='utf-8').split('\n')
vocab = open('vocab').read().decode(encoding='utf-8').split('\n') if not use_py3 else open('vocab', encoding='utf-8').read().split('\n')

Choose a reason for hiding this comment

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

see comment on the same code in generation.py above.

@@ -37,7 +40,7 @@
np.random.seed(args.seed)

# load the vocabulary from file
vocab = open('vocab').read().decode(encoding='utf-8').split('\n')
vocab = open('vocab').read().decode(encoding='utf-8').split('\n') if not use_py3 else open('vocab', encoding='utf-8').read().split('\n')
Copy link

@peterldowns peterldowns Sep 13, 2019

Choose a reason for hiding this comment

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

You can use six to ensure that the result is unicode text (not encoded bytes) in both versions:

vocab = six.ensure_text(open('vocab').read()).split(u'\n')

@voidism
Copy link
Contributor Author

voidism commented Sep 13, 2019

@peterldowns Awesome! Sorry that I don't know six can also handle this.

@peterldowns
Copy link

Yeah, six is great! It's the library specifically for doing this kind of compatibility work :) If you're doing this kind of multi-version stuff in other repositories, I would highly recommend looking into using six.

@keskarnitish
Copy link
Contributor

I agree, six would the perfect tool to use here. Otherwise, despite a few more TensorFlow warnings, everything seems OK.

@keskarnitish keskarnitish merged commit bc293d0 into salesforce:master Sep 16, 2019
@keskarnitish
Copy link
Contributor

I'm merging this for now so people can start using it; I will clean up to make it six-ified later.

@cclauss
Copy link

cclauss commented Sep 16, 2019

Sloppy!

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.

4 participants