search

blei-lab / edward

A probabilistic programming language in TensorFlow. Deep generative models, variational inference.
http://edwardlib.org
Other
4.83k stars 761 forks source link

Support recent versions of TensorFlow, one more #911

Open burnpanck opened 5 years ago

burnpanck commented 5 years ago

See issues #882 and #893. Another pull request (#894) apparently never got merged, maybe because it doesn't fix travis?

dustinvtran commented 5 years ago

Thanks for working on this! Yes, getting it to work on Travis is very important. It looks like it's still failing? I restarted the build.

burnpanck commented 5 years ago

I was able to make it work on TensorFlow 1.7, there was one test failing due to round-off. That one actually contained a bug and was passing just accidentally. I have adjusted the test-case such that it would fail now, but fixed the test. For TF 1.8+ there seems to be a lot more things breaking. I identified a problem in copy, where you seem to be constructing the operation first, adding the inputs afterwards. This doesn't work anymore, as TF wants to create the C-implementation in __init__. Since I won't have time to fix this now, I'll simply adjust the requirements to include up to TF 1.7, and leave TF 1.8+ for another day/person. As a side-note, Travis is also failing on TF 1.2 despite that being the minimum requirement in setup.py. I adjusted these requirements to 1.3.0rc0..1.7.0. However Travis seems to fail on some tests irreproducibly, as the comparison of the last two commits shows (they differ only in the travis.yml and the setup.py, but not the actual code).

diego-ardila-alvarez commented 5 years ago

I have been looking at the issue. There are two set of tests failing:

The easy one, in edward/util/random_variables, replace:

bij = tfb.Invert(tfb.SoftmaxCentered(event_ndims=1)) -> bij = tfb.Invert(tfb.SoftmaxCentered())

This because the corresponding signature in tensorflow.contrib.distributions changed in the recent versions.

As for the other one, as @burnpanck pointed out, it's an issue with copy and recursive graphs (e.g. tf.scan(lambda a, x: a + x, tf.constant([1.0, 2.0, 3.0]))). I am trying to find a workaround, as the current logic to handle these cases doesn't seem to work anymore.

diego-ardila-alvarez commented 5 years ago

@dustinvtran Would it make sense to refactor ed.copy to use contrib.graph_editor? https://www.tensorflow.org/api_guides/python/contrib.graph_editor

I believe the current implementation was possibly based on something like:

https://github.com/tensorflow/tensorflow/blob/r1.2/tensorflow/contrib/copy_graph/python/util/copy_elements.py

...Modified to support swapping and recursive graph definitions. However, it also makes edward very dependent on the inner workings of tensorflow.

At the moment the implementation is not working with recent tf versions, as the workaround for recursive graphs (defining inputs/outputs after the creation of the operation) conflicts with some recent changes. With contrib.graph_editor things might become more stable, though online editions are not possible. I have just started to use edward, so I don't know if this is too much of a restriction.