Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

TF-Seq2Seq via Docker #258

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

ReDeiPirati
Copy link

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.

darrengarvey and others added 10 commits June 11, 2017 17:47
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
Copy link

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
Copy link
Author

I signed it!

@googlebot
Copy link

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.

@ReDeiPirati
Copy link
Author

ReDeiPirati commented Jun 17, 2017

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
Copy link

@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
Copy link

@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
Copy link
Author

@darrengarvey Thank you for your help!! Now it works fine.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants