openai / generating-reviews-discovering-sentiment

Code for "Learning to Generate Reviews and Discovering Sentiment"
https://arxiv.org/abs/1704.01444
MIT License
1.51k stars 379 forks source link

Compatibility with TF >= 1.0.0 #4

Closed g3n1uss closed 7 years ago

g3n1uss commented 7 years ago

tf.unpack (used in 90th line of encoder.py) was deprecated in favor of tf.unstack since release 1.0.0. However, a simple replacement tf.unpack->tf.unstack does not work because tf.split has been changed too.

A full compatible with TF 1.0.1 version can be found here. Thanks to @blester125

rdeese commented 7 years ago

The problem persists even after running the TensorFlow upgrade utility on encoder.py. With python 3.5.0 and tensorflow 1.0.1 I get:

*> python encoder.py 
Traceback (most recent call last):
  File "encoder.py", line 209, in <module>
    mdl = Model()
  File "encoder.py", line 143, in __init__
    cells, states, logits = model(X, S, M, reuse=False)
  File "encoder.py", line 93, in model
    inputs = [tf.squeeze(v, [1]) for v in tf.split(axis=1, num_or_size_splits=nsteps, value=words)]
  File "/Users/rupertdeese/.pyenv/versions/3.5.0/lib/python3.5/site-packages/tensorflow/python/ops/array_ops.py", line 1203, in split
    num = size_splits_shape.dims[0]
IndexError: list index out of range
g3n1uss commented 7 years ago

@rdeese Yes, that is because tf.split has been changed too.

rdeese commented 7 years ago

@g3n1uss well, the upgrade utility is supposed to handle the changes in tf.split-- it makes changes to those function calls. The changes don't resolve the error though.

g3n1uss commented 7 years ago

@rdeese good point! Thanks!

blester125 commented 7 years ago

nsteps is passed into tf.split as the num_os_size_splits argument but nsteps is a "Dimension" this was fine when tf.split() looked like this

def split(split_dim, num_split, value, name="split"):
    return gen_array_ops._split(split_dim=split_dim,
                          num_split=num_split,
                          value=value,
                          name=name)

in v1.0 tf.split() changed not only the order of arguments but the function is now

def split(value, num_or_size_splits, axis=0, num=None, name="split"):
    if isinstance(num_or_size_splits, six.integer_types):
        return gen_array_ops._split(
            split_dim=axis, num_split=num_or_size_splits, value=value, name=name)
    else:
        size_splits = ops.convert_to_tensor(num_or_size_splits)
        if num is None:
            size_splits_shape = size_splits.get_shape()
            num = size_splits_shape.dims[0]
        if num._value is None:
            raise ValueError("Cannot infer num from shape %s" % num_or_size_splits)
        return gen_array_ops._split_v(
            value=value,
            size_splits=size_splits,
            split_dim=axis,
            num_split=num,
            name=name)

Now when tf.split() is called, if isinstance(num_or_size_splits, six.integer_types): returns false and the code tries to get the Dimension's shape but it is an empty list so .dims[0] fails.

g3n1uss commented 7 years ago

@blester125 You are right. I created fully compatible with TF 1.0.1 version here.

bkj commented 7 years ago

@g3n1uss Is the example supposed to yield a vector of all 0s in your fork?

g3n1uss commented 7 years ago

@bkj a vector. On my tensorflow-gpu (1.0.1) the example prints

0.496 seconds to transform 1 examples
(1, 4096)
cerisara commented 7 years ago

@g3n1uss Great ! Just cloned your TF1.0.1 version and it works ! For the others: don't forget git checkout remotes/origin/compatibleWithTF101

Thanks

Newmu commented 7 years ago

Migrated as of https://github.com/openai/generating-reviews-discovering-sentiment/commit/83d940d132fbd585deb63b8e3fcb523ea9557683