Add variant of new load and save ops for storing model params in a single file #7909

sidgoyal78 · 2018-01-27T04:11:25Z

This PR basically implements a different approach for issue #7722 , and is based on the suggestions in PR #7780.

Things added:

load_combine_op
save_combine_op
A unit test for saving and loading multiple LoDTensors
A unit test for saving and loading 1 LoDTensor (so as to verify that it performs the function of the original load_op and save_op as well).

TODO: To remove the original load_op and save_op and rename these.

… serialize_alternate

Xreki · 2018-01-29T12:15:56Z

paddle/operators/load_combine_op.cc

+                   "Cannot open file %s for load_combine op", filename);
+
+    auto out_var_names = Outputs("Out");
+    PADDLE_ENFORCE(out_var_names.size(), "Output variables cannot be found");


Change to PADDLE_ENFORCE_LT(out_var_names.size(), 0, "The number of output variables should be > 0.");

Done. (I think you mean PADDLE_ENFORCE_GT :) )

Xreki · 2018-01-29T12:21:19Z

paddle/operators/load_combine_op.cc

+      auto *tensor = out_var->GetMutable<framework::LoDTensor>();
+
+      uint64_t data_length;
+      char *buffer = NULL;


I think there is no need a buffer, we can directly use fin to read the file sequentially and deserialize the LoDTensor one by one.

Done, thanks for the suggestion.

Xreki · 2018-01-29T12:23:40Z

paddle/operators/load_combine_op.cc

+      current_serialized_data.assign(buffer, data_length);
+
+      // Create an input string stream
+      std::istringstream ist(current_serialized_data);


No need another stream, we can directly use fin.

Xreki · 2018-01-29T12:25:00Z

paddle/operators/load_combine_op.cc

+
+      auto *tensor = out_var->GetMutable<framework::LoDTensor>();
+
+      uint64_t data_length;


I am not sure is there still a need to save the data_length...

Yes, thanks for pointing this out. I have simplified the logic.

kexinzhao · 2018-01-30T00:09:31Z

paddle/operators/save_load_op_new_test.cc

+#include "gtest/gtest.h"
+#include "paddle/framework/op_registry.h"
+
+USE_NO_KERNEL_OP(save);


save -> save_combine?

Fixed, thanks.

kexinzhao · 2018-01-30T00:09:47Z

paddle/operators/save_load_op_new_test.cc

+#include "paddle/framework/op_registry.h"
+
+USE_NO_KERNEL_OP(save);
+USE_NO_KERNEL_OP(load);


load -> load_combine?

Fixed, thanks. Based on the naming issues, i fixed some other things as well. Thanks.

kexinzhao · 2018-01-30T00:24:24Z

paddle/operators/load_combine_op.cc

+    AddComment(R"DOC(
+LoadCombine Operator.
+
+LoadCombine operator combines together various tensor variable into a file.


Should it be something like "load various tensor variables from a file"?

Yes, will change the comment.

Xreki

Great work~ It is almost LGTM except some minor aspects.
We can merge this PR first and fix the comments in next PR if it blocks other work.

Xreki · 2018-01-30T11:40:55Z

paddle/operators/load_combine_op.cc

+ public:
+  LoadCombineOpProtoMaker(OpProto *proto, OpAttrChecker *op_checker)
+      : OpProtoAndCheckerMaker(proto, op_checker) {
+    AddOutput("Out", "(LoDTensor) The tensor need to be load_combined")


(LoDTensor) The tensor need to be load_combined -> (vector) The tensors need to be loaded

Done, thanks.

Xreki · 2018-01-30T11:42:20Z

paddle/operators/load_combine_op.cc

+        .AsDuplicable();
+    AddAttr<std::string>("file_path",
+                         "(string) "
+                         "Variable will be load_combined from \"file_path\".")


Variable will be load_combined from "file_path". -> Variables will be loaded from "file_path".
or The file_path to load Variables.

Xreki · 2018-01-30T11:42:59Z

paddle/operators/load_combine_op.cc

+    AddComment(R"DOC(
+LoadCombine Operator.
+
+LoadCombine operator loads tensor variables from a file.


More comments here.

Xreki · 2018-01-30T11:50:33Z

paddle/operators/save_combine_op.cc

+    PADDLE_ENFORCE(static_cast<bool>(fout), "Cannot open %s to write",
+                   filename);
+
+    auto inames = Inputs("X");


inames -> in_names

Modified, to make it consistent with variable name in load_combine: I modified it to inp_var_names

Xreki · 2018-01-30T11:50:57Z

paddle/operators/save_combine_op.cc

+    auto inames = Inputs("X");
+    PADDLE_ENFORCE_GT(
+        static_cast<int>(inames.size()), 0,
+        "The number of output variables should be greater than 0");


output -> input

Xreki · 2018-01-30T11:57:47Z

paddle/operators/save_load_op_new_test.cc

+USE_NO_KERNEL_OP(save_combine);
+USE_NO_KERNEL_OP(load_combine);
+
+TEST(SaveLoadNewOp, CPU) {


Move this unittest to save_load_combine_op_test.cc

Xreki · 2018-01-30T11:58:39Z

paddle/operators/save_load_combine_op_test.cc

+  auto load_var = scope.Var(out_var_name);
+  auto target = load_var->GetMutable<paddle::framework::LoDTensor>();
+  return target;
+}


Add a blank line here.

Xreki · 2018-01-30T11:59:14Z

paddle/operators/save_load_combine_op_test.cc

+USE_NO_KERNEL_OP(save_combine);
+USE_NO_KERNEL_OP(load_combine);
+
+int* create_for_save_combine_op(int x, int y, const std::vector<int>& lod_info,


create_for_save_combine_op -> CreateSaveCombineOp
the same for following functions.

… serialize_alternate

kexinzhao · 2018-01-30T21:40:43Z

paddle/operators/save_load_combine_op_test.cc

+  return actual;
+}
+
+void check_values(int* expect, int* actual, paddle::framework::LoD expect_lod,


check_values -> CheckValues

kexinzhao

LGTM!

sidgoyal78 added 8 commits January 22, 2018 16:48

Add save_combine_op

7781287

Add load_combine_op and test

6adc36e

Add unit-test

99d1d07

Add a delete to free buffer memory

a24fca3

Add new variant of load/save

11e3181

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

53cc3e3

… serialize_alternate

Fix unit-test

b1cc306

Add another unit test for compatibility with original save/load

6a3710f

sidgoyal78 requested review from Xreki, luotao1 and kexinzhao January 27, 2018 04:14

Xreki added this to DOING in Inference Framework Jan 29, 2018

Xreki reviewed Jan 29, 2018

View reviewed changes

sidgoyal78 added 2 commits January 29, 2018 19:15

Address review comments and simplify logic

98d2aba

Address review comments and simplify code - part 2

c7c2125

kexinzhao reviewed Jan 30, 2018

View reviewed changes

Fix naming issues and CMake problems

57bcdbb

sidgoyal78 mentioned this pull request Jan 30, 2018

Implement/refine load() with load_combine_op #7972

Closed

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

Xreki reviewed Jan 30, 2018

View reviewed changes

sidgoyal78 added 3 commits January 30, 2018 18:47

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

3d0f724

… serialize_alternate

Address review comments

383acc7

Fix LoD information in tests

511f472

kexinzhao reviewed Jan 30, 2018

View reviewed changes

Address review comments: round 2

d9de9ea

kexinzhao approved these changes Jan 30, 2018

View reviewed changes

sidgoyal78 merged commit 2e907c3 into PaddlePaddle:develop Jan 30, 2018

Xreki mentioned this pull request Jan 31, 2018

Add new load and save ops for storing model params in a single file #7780

Closed

kexinzhao moved this from DOING to DONE in Inference Framework Feb 1, 2018

Xreki mentioned this pull request Feb 9, 2018

Saving all trained params in a single file #7722

Closed


		auto *tensor = out_var->GetMutable<framework::LoDTensor>();

		uint64_t data_length;

Add variant of new load and save ops for storing model params in a single file #7909

Add variant of new load and save ops for storing model params in a single file #7909

Conversation

sidgoyal78 commented Jan 27, 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

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

Xreki 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

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