Better version of PR #7985 (Modify load() for inference) #8024

sidgoyal78 · 2018-02-01T03:47:45Z

Related PR #7985
Fix #7972

Xreki · 2018-02-01T11:14:09Z

paddle/inference/io.cc

+                      framework::Scope& scope,
+                      const std::string& dirname,
+                      const framework::ProgramDesc& main_program,
+                      const std::string& param_filename) {


There are two LoadPersistables. Can merge them into one?

Xreki · 2018-02-01T11:19:17Z

paddle/inference/io.cc

+    framework::Executor& executor,
+    framework::Scope& scope,
+    const std::string& dirname,
+    const std::string& param_filename) {


I'd like to keep two Load() interfaces as:

std::unique_ptr<framework::ProgramDesc> Load( framework::Executor& executor, framework::Scope& scope, const std::string& dirname); std::unique_ptr<framework::ProgramDesc> Load( framework::Executor& executor, framework::Scope& scope, const std::string& prog_filename, const std::string& params_filename);

Users are free to rename the file which saving the program. The argument list of save_inference_model may be changed in the future. #7995 is merged, we need to test the new Load. Please use the new Load in the unittest instead.

… modify_load

Xreki · 2018-02-05T10:00:21Z

paddle/inference/io.cc

+    framework::Scope& scope,
+    const std::string& dirname,
+    const std::string& prog_filename,
+    const std::string& param_filename) {


Sorry for not describing my suggest clearly. I mean to define the interface to:

std::unique_ptr<framework::ProgramDesc> Load( framework::Executor& executor, framework::Scope& scope, const std::string& prog_filepath, const std::string& param_filepath);

No need of dirname any more.

Oh, I think you described your suggestion clearly: #8024 (comment)

But, I chose to keep another argument of dirname to make stuff consistent with the python side. More specifically, in save_vars() we have: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/fluid/io.py#L124, and in load_vars() we have: https://github.com/PaddlePaddle/Paddle/blob/develop/python/paddle/v2/fluid/io.py#L202

Hence, I put that extra argument. But it seems that you would prefer an implementation without it, so I will modify it. Thanks.

Xreki · 2018-02-05T10:02:33Z

paddle/inference/io.cc

+  inputfs.seekg(0, std::ios::beg);
+  VLOG(3) << "program_desc_str's size: " << program_desc_str.size();
+  inputfs.read(&program_desc_str[0], program_desc_str.size());
+  inputfs.close();


Please try to reuse these codes (line 101 - 109 and line 124 - 133).Maybe we can directly call the following Load() to simplify the implementation.

Xreki · 2018-02-05T10:46:36Z

paddle/inference/tests/book/CMakeLists.txt

+cc_test(test_inference_recognize_digits_mlp_combine
+    SRCS test_inference_recognize_digits_combinels.cc
+    DEPS ARCHIVE_START paddle_fluid ARCHIVE_END
+    ARGS --dirname=${PYTHON_TESTS_DIR}/book/recognize_digits_mlp_combine.inference.model --prog_filename=__model__ --param_filename=params.dat)


We can implement the two unittests in one .cc file.

Xreki · 2018-02-05T10:49:08Z

paddle/inference/tests/book/test_inference_recognize_digits_combinels.cc

+
+DEFINE_string(dirname, "", "Directory of the inference model.");
+DEFINE_string(prog_filename, "", "File storing programDesc of inference model");
+DEFINE_string(param_filename, "", "File storing parameters of inference model");


In our unittests, we can fix the param_filename to __PARAMS__, so that no need for the two gflags variables.

Okay, sure. I will hardcode the string then.

Xreki · 2018-02-05T10:54:06Z

paddle/inference/tests/book/test_inference_recognize_digits_combinels.cc

+
+  // 2. Initialize the inference_program and load all parameters from file
+  auto inference_program = paddle::inference::Load(
+      executor, *scope, dirname, prog_filename, param_filename);


Change the definition of TestInference to:

template <typename Place, typename T, boolean IsCombined> void TestInference(const std::string& dirname, ...

Then change line 38 to:

if (IsCombined) { ... } else { ... }

So that we can define and call a common TestInference function for the two unittests.

Xreki · 2018-02-05T10:59:24Z

paddle/inference/tests/book/test_inference_recognize_digits_combinels.cc

+  delete scope;
+}
+
+TEST(inference, recognize_digits_combine) {


Move this function to test_inference_recognize_digits.cc.
Please note that we should avoid repeated codes.

Sorry, i didn't think about hardcoding the strings, hence couldn't think of a way to have everything together.

Xreki · 2018-02-05T11:07:29Z

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

@@ -214,7 +226,8 @@ def inject_all_tests():
    for use_cuda in (False, True):
        for parallel in (False, True):
            for nn_type in ('mlp', 'conv'):
-                inject_test_method(use_cuda, parallel, nn_type)
+                for combine in (False, True):


Please don't add this loop. This will twice the burden of CI. If there is not another way, we may only test the combined save/load.

I added just 1 unit-test for the 'separate' params case.

… modify_load

kexinzhao · 2018-02-07T01:12:40Z

python/paddle/v2/fluid/io.py

@@ -384,7 +389,12 @@ def load_inference_model(dirname, executor, load_file_name=None):
    if not os.path.isdir(dirname):
        raise ValueError("There is no directory named '%s'", dirname)

-    model_file_name = dirname + "/__model__"
+    model_file_name = ""


Same here. Please delete this line.

kexinzhao · 2018-02-07T01:13:16Z

python/paddle/v2/fluid/io.py

@@ -342,7 +342,12 @@ def save_inference_model(dirname,
    prepend_feed_ops(inference_program, feeded_var_names)
    append_fetch_ops(inference_program, fetch_var_names)

-    model_file_name = dirname + "/__model__"
+    model_file_name = ""


Please delete this line.
Unlike C++, if else statements don't define a scope in Python. So model_file_name is visible outside.
https://stackoverflow.com/questions/7382638/python-variable-scope-in-if-statements

Done, thanks.

… modify_load

kexinzhao

LGTM!

Xreki · 2018-02-08T12:06:58Z

paddle/inference/io.cc

@@ -21,6 +21,18 @@ limitations under the License. */
 namespace paddle {
 namespace inference {

+void ReadProgramDescFromFile(const std::string& model_filename,
+                             std::string& program_desc_str) {


I think this function is a common function to read binary to string. If there is no other use for this reading function, how about change it to

std::unique_ptr<framework::ProgramDesc> ReadProgram(const std::string& filename)

But we can change this in next PR.

I think this function will also be useful for "loading-from-buffer" case. I have changed the name to ReadBinaryFile.

Xreki · 2018-02-08T12:16:12Z

paddle/inference/io.cc

+
+  if (!param_filename.empty()) {
+    // sort paramlist to have consistent ordering
+    std::sort(paramlist.begin(), paramlist.end());


Tensor and LoDTensor have no name. If there is something wrong (for example, user coincidentally gives a wrong param_filename), there is no warning information at all. It will be difficult for users to debug. Can we add another mechanism to check or ensure the correctness? We can refer to other frameworks. Also, this can be done in next PR.

Good point, will think about it and as you mentioned will do later.

Xreki · 2018-02-08T12:23:25Z

python/paddle/v2/fluid/io.py

+    if save_file_name == None:
+        model_file_name = dirname + "/__model__"
+    else:
+        model_file_name = dirname + "/__model_combined__"


The naming style is not friendly for users. We need to refine this interface.

… modify_load

kexinzhao

Some of the issues mentioned by @Xreki will be fixed in future PRs.

Refine load

e556d23

sidgoyal78 mentioned this pull request Feb 1, 2018

Modify load() in io.cc for taking into account load_combine op #7985

Closed

sidgoyal78 requested review from kexinzhao and Xreki February 1, 2018 03:48

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

Xreki reviewed Feb 1, 2018

View reviewed changes

sidgoyal78 added 6 commits February 2, 2018 03:12

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

503f73a

… modify_load

Address review comments: round 1

749e33e

Make API consistent with python-save/load

f8570e6

Add another unit test

076e0e5

Remove commented function

0692742

Fix GPU bug

b17af18

Xreki requested a review from luotao1 February 2, 2018 09:21

Xreki reviewed Feb 5, 2018

View reviewed changes

sidgoyal78 added 7 commits February 6, 2018 01:07

Address review comments

e2b88c8

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

55f40e5

… modify_load

Modify wrt PR 8147

19343d4

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

4177474

… modify_load

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

a6f0b0f

… modify_load

Fix filenames for combined case

e0f8e74

Fix typo

8fb97bc

kexinzhao reviewed Feb 7, 2018

View reviewed changes

Address review comments: round 2

e813d13

sidgoyal78 mentioned this pull request Feb 7, 2018

Add support for loading from buffer for inference framework #8246

Closed

sidgoyal78 added 3 commits February 7, 2018 21:28

Resolve merge conflict

5430ddd

Unify TestInference by keeping default param in template

83b4616

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

8a002ce

… modify_load

kexinzhao previously approved these changes Feb 8, 2018

View reviewed changes

Xreki reviewed Feb 8, 2018

View reviewed changes

sidgoyal78 added 3 commits February 8, 2018 18:30

Merge branch 'develop' of https://github.com/PaddlePaddle/Paddle into…

4d61569

… modify_load

Address review comment

805b415

Fix spacing

33570f7

sidgoyal78 dismissed kexinzhao’s stale review via 33570f7 February 8, 2018 19:44

kexinzhao approved these changes Feb 8, 2018

View reviewed changes

sidgoyal78 merged commit 36da529 into PaddlePaddle:develop Feb 8, 2018

Xreki mentioned this pull request Feb 9, 2018

Implement/refine load() with load_combine_op #7972

Closed

sidgoyal78 mentioned this pull request Feb 15, 2018

Add useful debugging information in load() in inference API #8452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better version of PR #7985 (Modify load() for inference) #8024

Better version of PR #7985 (Modify load() for inference) #8024

sidgoyal78 commented Feb 1, 2018 •

edited by Xreki

Loading

Xreki Feb 1, 2018

sidgoyal78 Feb 2, 2018

Xreki Feb 1, 2018

sidgoyal78 Feb 2, 2018

sidgoyal78 Feb 2, 2018

Xreki Feb 5, 2018

sidgoyal78 Feb 5, 2018

Xreki Feb 5, 2018

sidgoyal78 Feb 5, 2018

Xreki Feb 5, 2018

sidgoyal78 Feb 6, 2018

Xreki Feb 5, 2018

sidgoyal78 Feb 5, 2018

Xreki Feb 5, 2018

sidgoyal78 Feb 6, 2018

Xreki Feb 5, 2018

sidgoyal78 Feb 5, 2018

Xreki Feb 5, 2018

sidgoyal78 Feb 6, 2018

kexinzhao Feb 7, 2018

sidgoyal78 Feb 7, 2018

kexinzhao Feb 7, 2018

sidgoyal78 Feb 7, 2018

kexinzhao left a comment

Xreki Feb 8, 2018

sidgoyal78 Feb 8, 2018

Xreki Feb 8, 2018

sidgoyal78 Feb 8, 2018

Xreki Feb 8, 2018

kexinzhao left a comment

Better version of PR #7985 (Modify load() for inference) #8024

Better version of PR #7985 (Modify load() for inference) #8024

Conversation

sidgoyal78 commented Feb 1, 2018 • edited by Xreki Loading

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

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

Choose a reason for hiding this comment

kexinzhao 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

kexinzhao left a comment

Choose a reason for hiding this comment

sidgoyal78 commented Feb 1, 2018 •

edited by Xreki

Loading