idiap / importance-sampling

Code for experiments regarding importance sampling for training neural networks
Other
320 stars 60 forks source link

Port to tensorflow 2 #25

Open renatobellotti opened 4 years ago

renatobellotti commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

https://github.com/tensorflow/tensorflow/pull/23880#issuecomment-514821825

renatobellotti commented 4 years ago

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

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 commented 4 years ago

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

renatobellotti commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

maryam2013 commented 4 years ago

Dear Angelos, thank you for replying so fast, but have u solved the problem bt using TF1? Best regards Maryam

On Mon, Apr 27, 2020 at 1:09 AM Angelos Katharopoulos < notifications@github.com> wrote:

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/idiap/importance-sampling/issues/25#issuecomment-619621608, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJCCHE3PYMYHJJXSI63LY2LROSLZRANCNFSM4JMFHO4Q .

angeloskath commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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