Make Keras models pickle-able (refactored) #11030

farizrahman4u · 2018-08-30T07:38:08Z

No description provided.

fchollet

I think it looks better with this common logic. The H5Dict class seems quite complex, however.

fchollet · 2018-08-30T17:56:27Z

keras/utils/hdf5_utils.py

+
+
+# FLAGS
+_is_boxed = '_is_dict_boxed'


What is this for? Non-obvious from the name...

fchollet · 2018-08-30T17:56:54Z

keras/utils/hdf5_utils.py

@@ -0,0 +1,197 @@
+"""HDF5 related utilities."""


Couldn't we just put this in io_utils.py?

fchollet · 2018-08-30T17:57:35Z

keras/utils/hdf5_utils.py

+            self._is_file = False
+            self.data[_is_boxed] = True
+        else:
+            raise Exception('Required Group, str or dict.'


Never raise a generic Exception, always use a specific exception (TypeError in this case).

fchollet · 2018-08-30T17:57:47Z

keras/utils/hdf5_utils.py

+
+    def __setitem__(self, attr, val):
+        if self.read_only:
+            raise Exception('Can not set item in read only mode.')


Never raise a generic Exception

fchollet · 2018-08-30T17:58:05Z

keras/utils/hdf5_utils.py

+        if self.read_only:
+            raise Exception('Can not set item in read only mode.')
+        is_np = type(val).__module__ == np.__name__
+        if type(self.data) is dict:


Do not use type(...) is, use isinstance (this applies to multiple lines in this PR)

fchollet · 2018-08-30T17:59:17Z

keras/utils/hdf5_utils.py

+                attr = attr.decode('utf-8')
+            if is_np:
+                self.data[attr] = pickle.dumps(val)
+                # We have to remeber to unpickle in __getitem__


fchollet · 2018-08-30T17:59:23Z

keras/utils/hdf5_utils.py

+                self.data[attr] = val
+            return
+        if attr in self:
+            raise Exception('Can not set attribute.'


Never raise a generic Exception

fchollet · 2018-08-30T18:06:02Z

keras/engine/saving.py

-    used for model definition or training.
+    This method is used for both writing to HDF5 file/group,
+    as well as pickling. This is achieved via a
+    keras.utils.hdf5_utls.H5Dict object, which can wrap HDF5


Use backquotes around code keywords

fchollet · 2018-08-30T18:06:25Z

keras/utils/hdf5_utils.py

+
+
+class H5Dict(object):
+    """ A dict-like wrapper around h5py groups (or dicts).


Remove leading space

fchollet · 2018-08-30T18:07:13Z

keras/utils/hdf5_utils.py

+        if isinstance(path, h5py.Group):
+            self.data = path
+            self._is_file = False
+        elif type(path) is str:


Do not use type(...) is, use isinstance. To test for string types, use six

fchollet · 2018-08-30T18:20:21Z

Please also add a quick standalone unit test for the H5Dict class.

farizrahman4u · 2018-08-31T00:05:54Z

keras/engine/saving.py

-def load_model(filepath, custom_objects=None, compile=True):
-    """Loads a model saved via `save_model`.
+def _deserialize_model(f, custom_objects=None, compile=True):
+    """De-serializes a model a serialized via _serialize_model


(Reminder to self)
Typo

joelthchao · 2018-08-31T16:23:56Z

keras/engine/saving.py

+            weight_names.append(name.encode('utf8'))
+        layer_group['weight_names'] = weight_names
+        for name, val in zip(weight_names, weight_values):
+            layer_group[name] = val


forloop 101-102 can combine with forloop 94-99

joelthchao · 2018-08-31T16:31:56Z

keras/engine/saving.py

-                                    name = str(w.name) + '_' + str(i)
-                                else:
-                                    name = str(w.name)
+                name = 'param_' + str(i)


Since we don't make constraint on naming, is it possible to have a duplicate name here? (Not sure if weight name need to be unique)

I do share the same concern.

joelthchao · 2018-08-31T16:45:18Z

keras/engine/saving.py

+        raise ValueError('You are trying to load a weight file '
+                         'containing ' + str(len(layer_names)) +
+                         ' layers into a model with ' +
+                         str(len(filtered_layers)) + ' layers.')


Suggest to use % style

raise ValueError('You are trying to load a weight file ' 'containing %d layers into a model with ' '%d layers.' % (len(layer_names), len(filtered_layers)))

We don't have a unified way, but I think str.format is more common?

joelthchao · 2018-08-31T16:49:02Z

keras/engine/saving.py

+        if weight_names:
+            filtered_layer_names.append(name)
+
+    layer_names = filtered_layer_names


I think keep using filtered_layer_names makes code more readable.

joelthchao · 2018-08-31T17:18:03Z

keras/utils/hdf5_utils.py

+
+            # Expecting this to never be true.
+            if len(bad_attributes) > 0:
+                raise RuntimeError('The following attributes cannot be saved to HDF5 '


Note that some of your codes exceed 85 characters

joelthchao · 2018-08-31T17:20:02Z

keras/utils/hdf5_utils.py

+                raise RuntimeError('The following attributes cannot be saved to HDF5 '
+                                   'file because they are larger than %d bytes: %s'
+                                   % (HDF5_OBJECT_HEADER_LIMIT,
+                                      ', '.join([x for x in bad_attributes])))


Redundant list comprehension

joelthchao · 2018-08-31T17:26:35Z

keras/utils/hdf5_utils.py

+            data_npy = np.asarray(val)
+
+            num_chunks = 1
+            chunked_data = np.array_split(data_npy, num_chunks)


Can we have a better initial num_chunks?
For example, math.ceil(data_npy.nbytes / HDF5_OBJECT_HEADER_LIMIT)

farizrahman4u · 2018-09-02T06:04:42Z

@fchollet Done.

gabrieldemarmiesse · 2018-09-03T20:08:16Z

keras/engine/saving.py

+        ValueError: In case of an invalid savefile.
+    """
+    if h5py is None:
+        raise ImportError('`load_model` requires h5py.')


This can be considered dead code since h5py is in the keras dependencies in the setup.py.

It's good to have anyway. Keras could have been installed with--no-deps, etc.

fchollet

Looks good to me, thank you. If someone else can take a quick look too, that would be great, as this is a large PR.

farizrahman4u · 2018-09-04T00:18:08Z

@Dref360 Quick review please?

Dref360

Minors comments concerning documentation.
A minor concern that may bite us later on, is that now that a Model is pickable, users will try to share it between processes which is not handled by TF.

Dref360 · 2018-09-04T00:26:08Z

keras/engine/saving.py

+        raise ValueError('You are trying to load a weight file '
+                         'containing ' + str(len(layer_names)) +
+                         ' layers into a model with ' +
+                         str(len(filtered_layers)) + ' layers.')


We don't have a unified way, but I think str.format is more common?

Dref360 · 2018-09-04T00:31:48Z

keras/utils/io_utils.py

+        elif isinstance(path, dict):
+            self.data = path
+            self._is_file = False
+            self.data['_is_group'] = True


Not sure that I get what this flag is, maybe add some quick doc?

Whether a dict is user data (f[attr] = {'x' : 'y'}) or a group (new_group = f['new_group']).

Dref360 · 2018-09-04T00:35:22Z

keras/utils/io_utils.py

+    This allows us to have a single serialization logic
+    for both pickling and saving to disk.
+
+    Note: This is not intended to be a generic wrapper.


Having those documented would be nice, maybe a Wiki page?

Dref360 · 2018-09-04T00:37:06Z

keras/utils/io_utils.py

+        return iter(list(self.data))
+
+    def iter(self):
+        return list(self.data)


self.data could get quite big, shouldn't we just do 'self.data.items()' to be more efficient?

Dref360 · 2018-09-04T00:39:58Z

keras/engine/saving.py

-                                    name = str(w.name) + '_' + str(i)
-                                else:
-                                    name = str(w.name)
+                name = 'param_' + str(i)


I do share the same concern.

farizrahman4u · 2018-09-04T00:49:35Z

A minor concern that may bite us later on, is that now that a Model is pickable, users will try to share it between processes which is not handled by TF.

Can you expand on this?

Dref360 · 2018-09-04T15:27:21Z

If an object is pickable, it's possible to send it to another Process.
The issue here is that a TF Session cannot get shared between processes. This is not handled currently and it shouldn't. It's just something that we should be aware of.

But, the user can try doing something like:

Creates a Model in Process 1
Send the model to Process 2
model.predict won't work on Process 2 because the tf.Session is bounded to Process 1.

fchollet · 2018-09-05T22:15:08Z

@Dref360 I believe such a use case would result in an exception, right? As long as there is a clear exception (and not undetermined behavior), we are fine from a UX standpoint.

Dref360 · 2018-09-06T14:20:43Z

It seems to work nowadays? Maybe it's new from tensorflow 1.10.

from keras import Model
from keras.layers import Input, Dense
import numpy as np
import multiprocessing as mp

inp = Input([10])
d = Dense(1)(inp)
mod = Model(inp, d)
mod.compile('sgd', 'mse')


def send():
    return mod.predict(np.zeros([10,10]))

pool = mp.Pool(5)
promises = [pool.apply_async(send, ) for _ in range(5)]
print([k.get().shape for k in promises])

farizrahman4u · 2018-09-11T05:55:46Z

@fchollet @Dref360

fchollet

LGTM, thanks for the changes!

datumbox · 2018-09-28T10:08:11Z

I believe this commit introduces a regression on model.save().

I have a small-sized CNN with the following stats:

Float OPS: 15,635,220
Parameters: 3,131,224
Layers: 76
Conv Depth: 21

If I install keras from the commit before this PR (5a6af4b), the save method works fine. Nevertheless by installing this PR I get the following error:

>>> model.save('model.h5')
Traceback (most recent call last):
  File "test.py", line 149, in <module>
    model.save('deleteme.h5')
  File "/usr/local/lib/python2.7/dist-packages/keras/engine/network.py", line 1087, in save
    save_model(self, filepath, overwrite, include_optimizer)
  File "/usr/local/lib/python2.7/dist-packages/keras/engine/saving.py", line 381, in save_model
    _serialize_model(model, f, include_optimizer)
  File "/usr/local/lib/python2.7/dist-packages/keras/engine/saving.py", line 78, in _serialize_model
    f['keras_version'] = str(keras_version).encode('utf8')
  File "/usr/local/lib/python2.7/dist-packages/keras/utils/io_utils.py", line 214, in __setitem__
    ' Group with name {} exists.'.format(attr))
KeyError: 'Can not set attribute. Group with name keras_version exists.'

Same problem appears on latest master. It would be good to fix it before we release a new version.

@farizrahman4u let me know if you want me to investigate further.

cc @fchollet @Dref360

farizrahman4u · 2018-10-03T18:38:18Z

@datumbox Thanks for the report. Fixed in #11289.

datumbox · 2018-10-03T18:56:15Z

@farizrahman4u awesome, thanks for the quick fix. I'll try it out on Friday.

marawanokasha · 2018-10-11T09:59:10Z

Hey, quick question, If I am using a custom loss function (or custom layer), is there a way to pass those custom objects when unpickling the model, or do I have to fallback to keras.models.load_model?

farizrahman4u · 2018-10-12T11:49:20Z

As long as the custom classes are available in the scope in which you are unpicking, you are good.

hartikainen · 2018-11-09T21:37:28Z

Are there plans to have this merged into tf.keras at some point? If so, when would it happen?

ryanjulian · 2018-11-15T21:24:58Z

I'm also curious if this will make it into the tf.keras tree

farizrahman4u added 6 commits August 30, 2018 12:16

add h5dict and refactor

d2d9dee

docstrings

3920638

tests

4e8a9f0

cleanup

ea83603

pep8

d706c5f

typo

476ab41

farizrahman4u mentioned this pull request Aug 30, 2018

Make Keras models Pickle-able #10483

Closed

minor fix

0a8430c

fchollet reviewed Aug 30, 2018

View reviewed changes

farizrahman4u commented Aug 31, 2018

View reviewed changes

joelthchao reviewed Aug 31, 2018

View reviewed changes

farizrahman4u added 6 commits August 31, 2018 23:25

Replace generic errors, pep8, type() -> isinstance()

63fcc17

h5dict tests

54e80b4

rem bad file

231d0fe

rem bad file

3853dc4

pep8

ff9731e

pep8

65cc1f6

gabrieldemarmiesse reviewed Sep 3, 2018

View reviewed changes

fchollet reviewed Sep 3, 2018

View reviewed changes

Dref360 reviewed Sep 4, 2018

View reviewed changes

fix weight name conflict, use .format()

b1f64d2

farizrahman4u closed this Sep 10, 2018

farizrahman4u reopened this Sep 10, 2018

fchollet approved these changes Sep 11, 2018

View reviewed changes

fchollet merged commit 9a4c5d8 into keras-team:master Sep 11, 2018

farizrahman4u deleted the pickle2 branch September 12, 2018 01:37

hartikainen mentioned this pull request Nov 15, 2018

Replace Serializable with "constructor objects". rlworkgroup/proposals#3

Closed

edurenye mentioned this pull request Mar 29, 2020

TypeError: can't pickle _thread.RLock objects #10528

Closed

3 tasks



		class H5Dict(object):
		""" A dict-like wrapper around h5py groups (or dicts).

Make Keras models pickle-able (refactored) #11030

Make Keras models pickle-able (refactored) #11030

Conversation

farizrahman4u commented Aug 30, 2018

fchollet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchollet commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farizrahman4u commented Sep 2, 2018

gabrieldemarmiesse Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchollet left a comment

Choose a reason for hiding this comment

farizrahman4u commented Sep 4, 2018

Dref360 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farizrahman4u commented Sep 4, 2018

Dref360 commented Sep 4, 2018

fchollet commented Sep 5, 2018

Dref360 commented Sep 6, 2018

farizrahman4u commented Sep 11, 2018

fchollet left a comment

Choose a reason for hiding this comment

datumbox commented Sep 28, 2018

farizrahman4u commented Oct 3, 2018

datumbox commented Oct 3, 2018

marawanokasha commented Oct 11, 2018

farizrahman4u commented Oct 12, 2018

hartikainen commented Nov 9, 2018

ryanjulian commented Nov 15, 2018

gabrieldemarmiesse Sep 3, 2018 •

edited

Loading