Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility for TF2 #115

Closed
cirpote opened this issue Apr 23, 2020 · 9 comments
Closed

Compatibility for TF2 #115

cirpote opened this issue Apr 23, 2020 · 9 comments

Comments

@cirpote
Copy link

cirpote commented Apr 23, 2020

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

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

cirpote commented Apr 23, 2020

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

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

cirpote commented Apr 24, 2020

Now it is working, thanks again.

@Mistobaan
Copy link
Contributor

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

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

@Mistobaan With respect to the second question:

  • py2 compatibility: We may drop the actual compatibility when keeping both actually becomes a problem. For the demos, it should be ok if they are only Py3 compatible.
  • we do plan to keep tf1 compatibility for now.
  • With respect to the PR, what do you have in mind specifically?

@Mistobaan
Copy link
Contributor

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

@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 :)

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

No branches or pull requests

4 participants