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

Fix estimator convert from Keras to export_savedmodel() #14284 #14354

Merged
merged 3 commits into from
Nov 30, 2017

Conversation

yjmade
Copy link
Contributor

@yjmade yjmade commented Nov 8, 2017

PR for #14284
[FIX]the estimator generate by tf.keras.model_to_estimator() cannot export saved_model because the model_fn provided by _create_keras_model_fn wasn't set export_outputs in the returned EstimatorSpec. Here I provide a default export_outputs with serve_default key and Predict API, and the result inside is same as predictions

[FIX]_save_first_checkpoint call saver.save with only a path and without filename, that make the saved ckpt files with name like {model_dir}/.meta and {model_dir}/.index, which is not be able to found by latest_checkpoint("{model_dir}"). As state by Saver.save(), save_path should be a path to the checkpoint name. So to fix this, I change the name to {model_dir}/keras_model.ckpt

…xport saved_model because the model_fn provided by _create_keras_model_fn wasn't set export_outputs in the returned EstimatorSpec. Here I provide a default export_outputs with serve_default key and Predict API, and the result inside is same as predictions

[FIX]_save_first_checkpoint call saver.save with only a path and without filename, that make the ckpt saved with name like `{model_dir}/.meta` and `{model_dir}/.index`, which can not be found by latest_checkpoint("{model_dir}"). As state by save method of Saver, save_path should be a path to the checkpoint name. So to fix this, I change the name to `{model_dir}/keras_model.ckpt`
@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@jhseu jhseu requested a review from fchollet November 16, 2017 01:16
@jhseu jhseu self-assigned this Nov 16, 2017
@fchollet fchollet requested a review from yifeif November 16, 2017 01:28
@@ -33,6 +35,9 @@
from tensorflow.python.platform import tf_logging as logging
from tensorflow.python.training import saver as saver_lib
from tensorflow.python.training import training_util
from tensorflow.python.saved_model import signature_constants
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add new dependency to tensorflow/python/keras/BUILD.

@yjmade
Copy link
Contributor Author

yjmade commented Nov 22, 2017

Thanks @yifeif for the review. The PR has been updated with review comments addressed. Please take a look.

@yifeif yifeif added the kokoro:force-run Tests on submitted change label Nov 22, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 22, 2017
@sb2nov sb2nov added awaiting review Pull request awaiting review and removed awaiting testing (then merge) labels Nov 29, 2017
@yifeif
Copy link
Contributor

yifeif commented Nov 30, 2017

Thanks @yjmade!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review Pull request awaiting review cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants