google-deepmind / graph_nets

Build Graph Nets in Tensorflow
https://arxiv.org/abs/1806.01261
Apache License 2.0
5.34k stars 783 forks source link

Compatibility for TF2 #115

Closed cirpote closed 3 years ago

cirpote commented 4 years ago

Hi all,

first of all, I would like to say that I found the paper and this library extremely useful and with many possible real applications. However, I have a small concern about its compatibility with TF2. I tried to re-adapt the shortest_path.py example with the TF2 framework, but I found several compatibility issues.

Is there an easy way to fix it? thanks in advance

alvarosg commented 4 years ago

Thank you for your message. Could you clarify what compatibility issues you are referring to?

There is a TF2 version of the sort.py demo in graph_nets/demos_tf2. So I don't anticipate big issues converting the shortest path demo, as the main differences between the demos are the inputs and targets, but not the model.

cirpote commented 4 years ago

Screenshot from 2020-04-23 19-25-10

I am not a big expert in python and TensorFlow in general, but it seems the problem arises in the utils_tf.py, after I call utils_tf.placeholders_from_networkxs() in my main .py file.

thanks for your reply

zafarali commented 4 years ago

Hi cirpote, looks like you're trying to use placeholders in TF2 which no longer exist. I highly recommend looking at the notebook in sort example in Tensorflow 2.

You will need to remove placeholders completely from your code and pass direct Tensors to your graph networks. Hope this helps!

cirpote commented 4 years ago

Now it is working, thanks again.

Mistobaan commented 4 years ago

feels like the other demos should be updated as well, specifically the graph_net_basics where even the links to TF1 are not even working (i.e. for unsorted_segment_sum https://www.tensorflow.org/api_docs/python/tf/math/unsorted_segment_sum). Is this task something good for a pull request? Is the repository committed to drop TF1/py2.7 compatibility?

alvarosg commented 4 years ago

Thanks for flagging @Mistobaan , the main problem is that we don't have integration tests for the demos, just for the tests, we will set something up :)

Most of the library works exactly the same in TF2 and TF1, as the library is actually the same, and we thought the only differences (session.run calls, and tf.function) were covered enough to in the TF2 demo to get some users started, so we favored releasing the actual TF2 compatibility earlier for those users with just one demo, rather than delaying the release to include more demos. We will add more demos in the near future :)

alvarosg commented 4 years ago

@Mistobaan With respect to the second question:

Mistobaan commented 4 years ago

@alvarosg thank you for the quick response.

for the PR I was thinking just a simple update of the notebook to be fully TF2 runnable with the updated links and imports.

alvarosg commented 4 years ago

@Mistobaan Of course, if you want to add a separate GraphNets Basics demo to the TF2 demo folder that covers the existing topics + tf.function that could be useful.

Maybe start by covering only tf.function, and then we can incrementally go from there if you are up to the task :)