include_optimizer parameter introduced to push_to_hub_keras() #616

merveenoyan · 2022-01-21T15:38:05Z

This PR addresses the discussion we had in #598.
I did a quick and dirty implementation and haven't tested yet so it's a WIP, I want to hear the review first.

For context: this parameter prevents models with custom optimizers (like the ones built with tfa) to be serialized and is by default set to True so I set it to False and made it an optional parameter.

cc: @osanseviero

gante

Should solve the issue 👍

gante · 2022-01-21T15:56:37Z

src/huggingface_hub/keras_mixin.py

@@ -50,7 +52,7 @@ def save_pretrained_keras(
        with open(path, "w") as f:
            json.dump(config, f)

-    tf.keras.models.save_model(model, save_directory)
+    tf.keras.models.save_model(model, save_directory, include_optimizer)


The third argument of save_model is overwrite (doc), so we'll have to specify the keyword argument for include_optimizer

osanseviero · 2022-01-21T16:10:57Z

Thanks for this PR!

Suggestion for future: before sending PRs for review, it's always good idea to test it at least locally since you might realize it does not work for X reason (wrong argument, weird TF bug, etc).

Anyways, overall looks good 👍 feel free to open the PR for official review once you've tested it and added some automated test as well

ariG23498 · 2022-01-26T07:49:22Z

Hey folks,

My two cents on this would be to use a **model_save_kwargs instead of include_optimizer. This would help us in using all the parameters of the tf.keras.models.save_model API with minimal changes.

WDYT?

merveenoyan · 2022-01-26T09:33:41Z

@osanseviero @ariG23498's suggestion is neat. I don't know which choice to pick because we'll set it to False by default to handle the custom optimizers (like the ones that tfa offers) user has to set it to True if they want to push a model with one of the default optimizers. Maybe take include_optimizer only and take the rest with something like **model_save_kwargs?

osanseviero · 2022-01-26T20:39:36Z

Yes, @merveenoyan. I think that sounds like a right approach and consistent with what we do in transformers and other places (setting include_optimizer to false and then other save args through **model_save_kwargs**)

src/huggingface_hub/keras_mixin.py

merveenoyan · 2022-01-27T12:53:52Z

I tested this in multiple ways, first one was directly pushing it by include_optimizer = True which was successful.

Second one was adding save_traces = False (this is one of the **model_save_kwargs) which didn't push the custom layers (and given that I didn't implement get_config it failed to load, which is expected already). This is normally set as True by TensorFlow. Pushing is just fine but from_pretrained_keras() doesn't work (again, expected, up to user to set it to something different than default).

I also didn't include optimizer and saved the traces, then when loading I was asked to compile manually.

I wonder if the UX is good enough at this point, I think it's just fine (so I need someone to criticize this well) @ariG23498 @osanseviero
I'll only do style related formatting now.

src/huggingface_hub/keras_mixin.py

Co-authored-by: Omar Sanseviero <osanseviero@users.noreply.github.com>

ariG23498 · 2022-01-27T14:25:13Z

@merveenoyan I think this works! Thanks for the great work 😄

gante

LGTM

merveenoyan · 2022-01-27T14:43:58Z

I'm writing automated tests at the moment, I will not merge yet. 🤓

merveenoyan · 2022-01-31T09:28:49Z

@osanseviero last week I've removed the pytest assertions from the two new test cases (they weren't going to be raised so). Should I merge?

osanseviero

Thanks a lot for this PR! Overall looks good, but I think the tests could improve a bit 😄

src/huggingface_hub/keras_mixin.py

osanseviero · 2022-01-31T09:39:00Z

src/huggingface_hub/keras_mixin.py

+        include_optimizer (:obj:`bool`, `optional`):
+            Whether or not to include optimizer during serialization.
+        model_save_kwargs(:obj:`dict`, `optional`):
+            Arguments other than default arguments that can be passed to tf.keras.models.save_model().


Same here :)

tests/test_keras_integration.py

osanseviero · 2022-01-31T09:45:23Z

tests/test_keras_integration.py

+        self.assertIn("saved_model.pb", files)
+        self.assertIn("keras_metadata.pb", files)
+        self.assertEqual(len(files), 4)
+


A second test that shows that the optimizer is not there by default would be a good idea

tests/test_keras_integration.py

osanseviero · 2022-01-31T17:26:43Z

Feel free to pin the black version 😄

merveenoyan · 2022-02-01T11:29:45Z

AFTER FIGHTING BLACK FOR HOURS @osanseviero

osanseviero

Thanks a lot for this PR! I left a couple of improvement ideas but looks almost ready to merge! 🚀 🚀

osanseviero · 2022-02-01T13:05:18Z

tests/test_keras_integration.py

+        model = from_pretrained_keras(f"{WORKING_REPO_DIR}/{REPO_NAME}")
+        self.assertIsNone(model.optimizer)


I would suggest to avoid testing this as well in the same test. Maybe let's have a test which is
test_save_pretrained_does_not_save_optimizer_state_by_default which has this

tests/test_keras_integration.py

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

merveenoyan · 2022-02-02T08:58:08Z

@osanseviero on my local (checked black version) the quality test passes but here it fails. Any guesses?

include_optimizer parameter introduced to push_to_hub_keras()

4fb141a

merveenoyan requested review from gante and osanseviero January 21, 2022 15:38

gante reviewed Jan 21, 2022

View reviewed changes

merveenoyan removed the request for review from osanseviero January 27, 2022 11:09

merveenoyan added 2 commits January 27, 2022 12:32

added model save kwargs and made include_optimizer a keyword argument

a541cc5

kwargs fix

89e5b36

ariG23498 reviewed Jan 27, 2022

View reviewed changes

src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved

misc fixes

70f8211

formatted

47fa6d6

osanseviero reviewed Jan 27, 2022

View reviewed changes

src/huggingface_hub/keras_mixin.py Outdated Show resolved Hide resolved

merveenoyan and others added 2 commits January 27, 2022 15:09

Update src/huggingface_hub/keras_mixin.py

54e85d5

Co-authored-by: Omar Sanseviero <osanseviero@users.noreply.github.com>

Removed stars from docstring

2013f67

gante approved these changes Jan 27, 2022

View reviewed changes

merveenoyan added 3 commits January 27, 2022 17:42

committing my work in case if anything happens to me

5d4fa8b

make style

129ecfe

removed pytest assertions

aef366a

merveenoyan marked this pull request as ready for review January 28, 2022 15:25

osanseviero reviewed Jan 31, 2022

View reviewed changes

merveenoyan added 3 commits January 31, 2022 17:10

fixed test casesa and nit to docstring

5abf57c

style format

9158bef

make style

dd55e4c

black reformats

ff09289

merveenoyan added 2 commits February 1, 2022 12:57

Pinned black

119f596

make style

746cb0b

include_optimizer parameter introduced to push_to_hub_keras()

dedc891

osanseviero approved these changes Feb 1, 2022

View reviewed changes

merveenoyan and others added 4 commits February 1, 2022 16:16

Update tests/test_keras_integration.py

ff44712

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

changed model variable name loaded with from_pretrained_keras

e8f27f4

removed loaded_model that wasn't used

adc3c8f

Removed redundant tests

b3c15f5

merveenoyan merged commit 97ed9b9 into main Feb 2, 2022

merveenoyan deleted the keras_mixin_fix branch February 2, 2022 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include_optimizer parameter introduced to push_to_hub_keras() #616

include_optimizer parameter introduced to push_to_hub_keras() #616

merveenoyan commented Jan 21, 2022 •

edited

Loading

gante left a comment

gante Jan 21, 2022

osanseviero commented Jan 21, 2022

ariG23498 commented Jan 26, 2022

merveenoyan commented Jan 26, 2022

osanseviero commented Jan 26, 2022

merveenoyan commented Jan 27, 2022 •

edited

Loading

ariG23498 commented Jan 27, 2022

gante left a comment

merveenoyan commented Jan 27, 2022

merveenoyan commented Jan 31, 2022

osanseviero left a comment

osanseviero Jan 31, 2022

osanseviero Jan 31, 2022

osanseviero commented Jan 31, 2022

merveenoyan commented Feb 1, 2022 •

edited

Loading

osanseviero left a comment

osanseviero Feb 1, 2022

merveenoyan commented Feb 2, 2022

		model = from_pretrained_keras(f"{WORKING_REPO_DIR}/{REPO_NAME}")
		self.assertIsNone(model.optimizer)

include_optimizer parameter introduced to push_to_hub_keras() #616

include_optimizer parameter introduced to push_to_hub_keras() #616

Conversation

merveenoyan commented Jan 21, 2022 • edited Loading

gante left a comment

Choose a reason for hiding this comment

gante Jan 21, 2022

Choose a reason for hiding this comment

osanseviero commented Jan 21, 2022

ariG23498 commented Jan 26, 2022

merveenoyan commented Jan 26, 2022

osanseviero commented Jan 26, 2022

merveenoyan commented Jan 27, 2022 • edited Loading

ariG23498 commented Jan 27, 2022

gante left a comment

Choose a reason for hiding this comment

merveenoyan commented Jan 27, 2022

merveenoyan commented Jan 31, 2022

osanseviero left a comment

Choose a reason for hiding this comment

osanseviero Jan 31, 2022

Choose a reason for hiding this comment

osanseviero Jan 31, 2022

Choose a reason for hiding this comment

osanseviero commented Jan 31, 2022

merveenoyan commented Feb 1, 2022 • edited Loading

osanseviero left a comment

Choose a reason for hiding this comment

osanseviero Feb 1, 2022

Choose a reason for hiding this comment

merveenoyan commented Feb 2, 2022

merveenoyan commented Jan 21, 2022 •

edited

Loading

merveenoyan commented Jan 27, 2022 •

edited

Loading

merveenoyan commented Feb 1, 2022 •

edited

Loading