-
Notifications
You must be signed in to change notification settings - Fork 1.3k
TF-Seq2Seq via Docker #258
Conversation
Fixes the following error when running the tests on Python 3. Traceback (most recent call last): File "seq2seq/seq2seq/test/hooks_test.py", line 55, in test_begin self.assertEqual(file_contents.decode(), ...) AttributeError: 'str' object has no attribute 'decode'
distutils.version isn't available on the CI versions, so change the test to not rely explicitly on the version of TensorFlow installed.
googlebot
commented
Jun 16, 2017
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).
📝 Please visit https://cla.developers.google.com/ to sign.
Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.
- If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
- If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
- In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.
ReDeiPirati
commented
Jun 16, 2017
I signed it!
googlebot
commented
Jun 16, 2017
So there's good news and bad news.
👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.
😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.
Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.
Well this is strange because i haven't changed a single line of python code and during test i've never failed a pylint test. But now it fails even in the Docker container as during CI pipeline. @darrengarvey may you help me?
Edit: I've opened a Issue on pylint beacuse both the errors seem to be False positive.
Use of `tf.name_scope` and `tf.variable_scope` cause pylint errors on TF1.2 due to pylint not fully understanding the `tf_contextlib.contextmanager` decorators.
darrengarvey
commented
Jun 18, 2017
@ReDeiPirati - Looks like it's started failing because the tensorflow pip package was updated to v1.2.0 on the 15th. My PR passed Circle CI checks because it was still building against TF 1.1 (while I had TF 1.2 locally).
I'll take a look.
Following some changes in TF to the `LazyLoader` [1], pylint complains about not being able to find some imports under `tf.contrib`. This looks like a pylint issue, emitting errors like: ************* Module seq2seq.encoders.rnn_encoder E: 24, 0: No name 'rnn' in module 'LazyLoader' (no-name-in-module) ************* Module seq2seq.data.input_pipeline E: 32, 0: No name 'slim' in module 'LazyLoader' (no-name-in-module) [1] tensorflow/tensorflow@95c5d7e
darrengarvey
commented
Jun 18, 2017
@ReDeiPirati - I pushed a couple of fixes to #254. The no-name-in-module errors I just had to work around though. The LazyLoader is possibly too dynamic for pylint now, eg:
# OK: from tensorflow.contrib import learn learn.MetricSpec # pylint errors with: "No name 'learn' in module 'LazyLoader' (no-name-in-module)" from tensorflow.contrib.learn import MetricSpec
ReDeiPirati
commented
Jun 18, 2017
@darrengarvey Thank you for your help!! Now it works fine.
I have provided an alternative method of installation via Docker(documented in the docs); i've tested all the Dockerfile image. This pull contains also the #254 otherwise the python3.5 or a Tensorflow 1.2 version would fail the test.