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

Port to tensorflow 2 #25

Open
renatobellotti opened this issue Nov 12, 2019 · 20 comments
Open

Port to tensorflow 2 #25

renatobellotti opened this issue Nov 12, 2019 · 20 comments

Comments

@renatobellotti
Copy link

renatobellotti commented Nov 12, 2019

Running this module using tensorflow 2.0.0 yields an error:
AttributeError: module 'tensorflow' has no attribute 'get_default_graph'

Are there any plans to port the library to tensorflow 2.0.0?

Perhaps all you have to do is change import keras to from tensorflow import keras and perhaps use fully qualified names when using layers...

@renatobellotti
Copy link
Author

The following minimal example gives another error message. The same code without ImportanceTraining works fine.

from tensorflow import keras
import numpy as np

from importance_sampling.training import ImportanceTraining

num_x_features = 5
num_y_features = 2

x_train = np.random.uniform(0, 1, (100, num_x_features))
y_train = np.random.uniform(0, 1, (100, num_y_features))

model = keras.models.Sequential()
model.add(keras.layers.Dense(10, activation='relu', input_shape=(5,)))
model.add(keras.layers.Dense(10, activation='relu'))
model.add(keras.layers.Dense(num_y_features))
model.compile("adam", loss="mse")

history = ImportanceTraining(model).fit(
    x_train, y_train,
    batch_size=8,
    epochs=5,
)

Error message:

AttributeError: 'Node' object has no attribute 'output_masks'

@renatobellotti
Copy link
Author

renatobellotti commented Nov 18, 2019

Will tensorflow 2 be supported at some point? I'd like to try this for my master's thesis and it would be nice to save the model in tensorflow 2 because keras will soon become obsolete.

@angeloskath
Copy link
Collaborator

Hi, sorry for the late reply.

It would be nice to update to tf 2. If instead of from tensorflow import keras you do import keras does it still have issues? I have not had the time to look at it yet.

Feel free to dig in the code and ask question I will help as best I can.

Cheers,
Angelos

@renatobellotti
Copy link
Author

It is not enough, unfortunately.

import keras does not work anymore. Starting with tensorflow 2, keras will be completely integrated into tensorflow. Quote from www.keras.io:

At this time, we recommend that Keras users who use multi-backend Keras with the TensorFlow backend switch to tf.keras in TensorFlow 2.0. tf.keras is better maintained and has better integration with TensorFlow features (eager execution, distribution support and other).

Keras 2.2.5 was the last release of Keras implementing the 2.2.* API. It was the last release to only support TensorFlow 1 (as well as Theano and CNTK).

I've tried to upgrade the project automatically, but apparently there are some things that are not caught by the script.

The point where I'm failing is in importance-sampling/training.py. What is CallbackList? I was not able to find any documentation, so I don't know how to replace it.

I can probably invest a bit time in porting this library, but I'd probably need some help.

@MaximilianBoemer
Copy link

MaximilianBoemer commented Jan 14, 2020

Try: from tensorflow.python.keras.callbacks import CallbackList

tensorflow/tensorflow#23880 (comment)

@renatobellotti
Copy link
Author

renatobellotti commented Jan 20, 2020

I have made some formal changes to the code in my fork (4456623). There are still some things I couldn't solve yet:

  • test_model_learning_phase.py: The tests fail.
  • test_seq2seq.py: The tests fail.
  • test_keras_utils.py: The test fails. Other than that: I think these tests are invalid, because in lines 29 and 47 only the values that already are in memory are compared. This makes the failure even stranger...
  • test_finetuning.py: Passes, but the loss value is NaN.

I suspect most of the errors come from lines 262 and 384 model_wrappers.py. I'm pretty sure this should be different, but I couldn't find out how to avoid the error that is thrown in the original code.

I'd appreciate your feedback on the issue.

@angeloskath
Copy link
Collaborator

@renatobellotti Sorry for the late reply, if you still need help with anything, let me know...

@renatobellotti
Copy link
Author

I have not worked on this again, and my thesis does not allow me to spend more time ob this issue. I hope my contributions are at least a bit helpful.

@maryam2013
Copy link

Hi everybody,
I come across the same error,

AttributeError: module 'tensorflow' has no attribute 'get_default_graph',

when applying importance sampling with Keras.
have you found a solution?
I would be pleased if you help me with the issue.
Thank you in advance
Maryam

@angeloskath
Copy link
Collaborator

At this point I would advise using TF 1 instead of 2.

@maryam2013
Copy link

maryam2013 commented Apr 26, 2020 via email

@angeloskath
Copy link
Collaborator

In my case it works with keras 2.3.1 and tf 2.1 however when the code was first developed there was tf 1 and keras 2 so if you are having issues, then try to use a tensorflow version < 2.

@renatobellotti
Copy link
Author

Perhaps a clarification is needed. With the transition to tf2, the Keras API was merged into TensorFlow and is now a part of it. The standalone version of Keras will be discontinued. That means if this package cannot be run with tf2 only (without the separate Keras package), there will not be much future for it.

I'm sorry to say this so directly, but I think this is a very important issue.

@maryam2013
Copy link

Dear all,
I come across the same error when implementing importance sampling with Keras,
from importance_sampling.training import ImportanceTraining

AttributeError: 'Node' object has no attribute 'output_masks'

have you found a solution?
I would be grateful if you help me with the problem.
Thank you in advance
Maryam

@renatobellotti
Copy link
Author

Dear all,
I come across the same error when implementing importance sampling with Keras,
from importance_sampling.training import ImportanceTraining

AttributeError: 'Node' object has no attribute 'output_masks'

have you found a solution?
I would be grateful if you help me with the problem.
Thank you in advance
Maryam

@maryam2013 I am not using this package because I'm using TF2, but perhaps the developers need your version of Keras to help you.

@angeloskath
Copy link
Collaborator

@renatobellotti I am sorry if I came across as dismissive.

I agree 100% that we need to move to TF2. However, I do not think that it would be trivial. For instance, since TF now is eager by default maybe we can completely redesign and simplify the library. There is a lot of magic happening behind the scenes in the current version so that we can recreate the training experience one has with Keras.

I do not see myself doing this kind of redesign in the coming weeks or months and I realize that it will probably hurt the adoption of the library quite a lot.

@renatobellotti
Copy link
Author

@angeloskath Thank you for your answer. I formulated my post harsher than intended, sorry for that.

Of course I understand that time is always a limited resource. If you decide in the future to do a rewrite, please let me know, perhaps I can help, at least with testing.

@ibarrien
Copy link

Hello, could anyone supply the precise tensorflow and keras versions used to successfully run examples/mnist_mlp.py with args.importance_training=True ?

The _augment_model method in model_wrappers.py is problematic (at least with tensorflow > 2.0) since that method calls get_output before corresponding layers are used:

_get_node_attribute_at_index:
'The layer has never been called and thus has no defined ' + attr_name + '.'

Thanks for your help!

@QuetzalcoatlRosso
Copy link

Here is a working example (as of earlier this year):
https://github.com/ibarrien/Adam-with-Bandit-Sampling/blob/main/banditutils/setup.py

See in general that repo:
https://github.com/ibarrien/Adam-with-Bandit-Sampling/

@QuetzalcoatlRosso
Copy link

Perhaps this issue could be closed once the authors create a proper setup.py with explicit install requirements that work in this repo.

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

6 participants