Fix save load inference model and remove pickle #7712

kexinzhao · 2018-01-21T05:21:20Z

This pr addresses comments by @Xreki on #7636.

It fixes the following issues:

Remove unused LoadInferenceModel() function
Use a better way to detect if a variable is a feed var.
Allow user to pass different feed/fetch var names other than the default feed and fetch
Remove dependency on pickle when saving / loading inference model
Because of the change in save/load_inference_model function, the executor.run() method in executor.py is also modified to also handle the situation where the loaded inference model has already been prepended/appended feed/fetch operator (it will throw error when the info contained in the already attached feed/fetch ops do not match the feed and fetch_list input arguments)

TODO: In the future PR, we will also create a new executor.run() function in the C++ executor class to mimic how executor.py handle ProgramDesc in this PR.

Xreki

Would you please add an example to show how to use load_inference_model to do inference in Python API? You can append the usage codes in https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/fluid/tests/book/test_recognize_digits_mlp.py.

Xreki · 2018-01-22T03:45:08Z

paddle/inference/inference.cc


 void InferenceEngine::LoadInferenceModel(const std::string& dirname) {
-  std::string model_filename = dirname + "/__model__.dat";
+  std::string model_filename = dirname + "/__model__";


Remove line 22 - 24 (I cannot comment on line 22.)

Xreki · 2018-01-22T03:52:14Z

paddle/inference/inference.cc

    }
  }
 }



Why need to use insert on line 48?

This is to make the ordering of the feed_var_names_ (also known as feed_target_names) consistent with the python side when using the save_inference_model function.

Say you use code like save_inference_model(MODEL_DIR, ["x", "y"], [avg_cost], exe, program) in python.

Then based on our code when prepending feed operators (because we first prepend "x", then "y"), the order of feed ops in block.AllOps will be like [feed("y") feed("x")].

So by doing the insert thing on line 48, we can make the order of vector feed_var_names_ to be ["x", "y"] which is consistent with the python side order. So that we you provide a vector<Tensor> feeds, you know the order is the same as the python save_inference_model.

I see. Thanks~

Xreki · 2018-01-22T06:03:10Z

python/paddle/v2/fluid/executor.py

+        for op in global_block.ops:
+            if op.desc.type() == 'feed':
+                feed_count += 1
+                assert op.desc.input('X')[0] == feed_var_name


Do we need to keep all the input variable of feed_op the same name? I'm not sure...

I assume there will be only one feed_variable in the program Desc, which holds a vector of Tensor.
Each feed operator will take one of the Tensor data from this input feed_variable and copy the data to its output variable. So I think different feed operator should have different output variable names (x, y, etc), but should have the same input variable which is our feed_var_name here.

Could you explain a little more about your point? Thanks!

Well, I am not sure, whether the input data can be feed correctly if we set the feed_holder_name to different string... and do we need to support this...

I prefer to make the feed_holder_name (same for fetch) consistent so that the code is simpler and less error prone.

Even if this feature is later found out to be desirable, we can fix it in future PR. What do you think?

Xreki · 2018-01-22T06:04:44Z

python/paddle/v2/fluid/executor.py

+                core.set_feed_variable(scope, cur_feed, feed_var.name, idx)
+            elif op.desc.type() == 'fetch':
+                fetch_count += 1
+                assert op.desc.output('Out')[0] == fetch_var_name


Do we need to keep all the output variable of fetch_op the same name? I'm not sure...

Xreki · 2018-01-22T06:11:13Z

python/paddle/v2/fluid/executor.py

+                    type='fetch',
+                    inputs={'X': [var]},
+                    outputs={'Out': [fetch_var]},
+                    attrs={'col': i})


This function is too long. Can you simplify it and avoid some repeated codes?

Done. I extract the feed/fetch operator checking code as separate functions and simplify the code a little bit.

kexinzhao

Thanks @Xreki for the suggestions.
I have added some example code using load_inference_model in test_recognize_digits_mlp.py

kexinzhao · 2018-01-23T05:05:14Z

paddle/inference/inference.cc


 void InferenceEngine::LoadInferenceModel(const std::string& dirname) {
-  std::string model_filename = dirname + "/__model__.dat";
+  std::string model_filename = dirname + "/__model__";


kexinzhao · 2018-01-23T05:06:18Z

python/paddle/v2/fluid/executor.py

+                    type='fetch',
+                    inputs={'X': [var]},
+                    outputs={'Out': [fetch_var]},
+                    attrs={'col': i})


Done. I extract the feed/fetch operator checking code as separate functions and simplify the code a little bit.

Xreki · 2018-01-23T11:05:37Z

paddle/inference/inference.cc

    }
  }
 }



I see. Thanks~

Xreki · 2018-01-23T11:11:09Z

python/paddle/v2/fluid/executor.py

    return ans


+def has_feed_operators(block, feed_targets, feed_holder_name):


Add some comment for this function?

Xreki · 2018-01-23T11:11:24Z

python/paddle/v2/fluid/executor.py

+    return feed_count > 0
+
+
+def has_fetch_operators(block, fetch_targets, fetch_holder_name):


Add some comment for this function?

Xreki · 2018-01-23T11:16:41Z

python/paddle/v2/fluid/io.py


-def prepend_feed_ops(inference_program, feeded_var_names):
+def prepend_feed_ops(inference_program,
+                     feeded_var_names,


feeded_var_names -> feed_target_names?

Xreki · 2018-01-23T11:17:55Z

python/paddle/v2/fluid/io.py


-def append_fetch_ops(inference_program, fetch_var_names):
+def append_fetch_ops(inference_program,
+                     fetch_var_names,


fetch_var_names -> fetch_target_names?

Xreki · 2018-01-23T11:21:23Z

python/paddle/v2/fluid/io.py

        predicate=_is_presistable_and_exist_)


+def get_feed_targets(program):


get_feed_targets -> get_feed_target_names?

Xreki · 2018-01-23T11:21:49Z

python/paddle/v2/fluid/io.py

+    return feed_targets
+
+
+def get_fetch_targets(program):


get_fetch_targets -> get_fetch_target_names?

Xreki · 2018-01-23T11:23:59Z

python/paddle/v2/fluid/tests/book/test_recognize_digits_mlp.py

+            results = exe.run(infer_prog,
+                              feed={feed_var_names[0]: tensor_x},
+                              fetch_list=fetch_vars)
+            print(results[0])


Can you move the added lines to the end of the file (before exit) and add some comments?

Xreki

LGTM

sidgoyal78 · 2018-01-24T19:07:42Z

paddle/inference/inference.cc

    for (size_t i = 0; i < program_->Size(); ++i) {
      const framework::BlockDesc& block = program_->Block(i);
      for (auto* op : block.AllOps()) {
+        if (op->Type() == "feed") {


Just a quick clarification, how do we handle vars whose name is "fetch"?

We don't need to handle fetch case. Because "fetch" var, although is persistable, is only going to be the output of ops, which will not be a problem for the logic this code:

for (auto input_argument_name : op->InputArgumentNames()) {

sidgoyal78 · 2018-01-24T19:10:06Z

python/paddle/v2/fluid/tests/book/test_recognize_digits_mlp.py

 exe.run(fluid.default_startup_program())

-PASS_NUM = 100
+PASS_NUM = 1


I think we should change PASS_NUM to 100 :)

sidgoyal78

LGTM

kexinzhao added 5 commits January 19, 2018 18:40

remove pick dependency

0e8bf52

fix bug

52f07d4

small fix

888e48b

modify executor.py for save and load

0580519

clean code

55fdc82

kexinzhao requested review from Xreki and sidgoyal78 January 21, 2018 05:21

Xreki added the 预测原名Inference，包含Capi预测问题等 label Jan 22, 2018

Xreki reviewed Jan 22, 2018

View reviewed changes

kexinzhao added 3 commits January 22, 2018 15:45

Add usage example

8fb74d0

refine executor run function

73cdd63

fix bug

834d8ad

kexinzhao commented Jan 23, 2018

View reviewed changes

kexinzhao added 2 commits January 22, 2018 21:55

refine executor code

2458893

fix block bug

3cce026

Xreki reviewed Jan 23, 2018

View reviewed changes

kexinzhao added 2 commits January 23, 2018 11:36

fix comments

0bda221

fix bug

459d9cb

Xreki previously approved these changes Jan 24, 2018

View reviewed changes

sidgoyal78 reviewed Jan 24, 2018

View reviewed changes

fix pass num

90889d0

kexinzhao dismissed Xreki’s stale review via 90889d0 January 24, 2018 19:23

sidgoyal78 approved these changes Jan 24, 2018

View reviewed changes

kexinzhao merged commit 5009f7c into PaddlePaddle:develop Jan 24, 2018

		return ans


		def has_feed_operators(block, feed_targets, feed_holder_name):

		return feed_count > 0


		def has_fetch_operators(block, fetch_targets, fetch_holder_name):

		predicate=_is_presistable_and_exist_)


		def get_feed_targets(program):

Fix save load inference model and remove pickle #7712

Fix save load inference model and remove pickle #7712

Uh oh!

Conversation

kexinzhao commented Jan 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xreki left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kexinzhao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xreki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kexinzhao commented Jan 21, 2018 •

edited

Loading

Xreki left a comment •

edited

Loading