New Run() method for framework::Executor #7807

kexinzhao · 2018-01-24T01:39:53Z

The API is a little bit different than in #7610. I encountered memory issue when using std::map<std::string, LoDTensor>. So I changed to pointer instead.

  void Run(const ProgramDesc& program, Scope* scope,
           std::map<std::string, const LoDTensor*>& feed_targets,
           std::map<std::string, LoDTensor*>& fetch_targets,
           const std::string& feed_holder_name = "feed",
           const std::string& fetch_holder_name = "fetch");

Xreki · 2018-01-25T11:33:59Z

paddle/framework/executor.cc

@@ -149,5 +150,169 @@ void Executor::Run(const ProgramDesc& pdesc, Scope* scope, int block_id,
  }
 }

+// Return false if the block does not have any feed operators.


Add description of Check whether the block already has feed operators. here.

Xreki · 2018-01-25T11:37:28Z

paddle/framework/executor.cc

+    const std::string& feed_holder_name) {
+  size_t feed_count = 0;
+  for (auto* op : block->AllOps()) {
+    if (op->Type() == "feed") {


op->Type() == "feed" -> op->Type() == kFeedOpType

Xreki · 2018-01-25T11:40:10Z

paddle/framework/executor.cc

+  for (auto* op : block->AllOps()) {
+    if (op->Type() == "feed") {
+      feed_count++;
+      PADDLE_ENFORCE(op->Input("X")[0] == feed_holder_name,


Can we use PADDLE_ENFORCE_EQ here?

Xreki · 2018-01-25T11:43:05Z

paddle/framework/executor.cc

+  }
+
+  if (feed_count > 0) {
+    PADDLE_ENFORCE(feed_count == feed_targets.size(),


Use PADDLE_ENFORCE_EQ here.

Xreki · 2018-01-25T11:44:30Z

paddle/framework/executor.cc

+    const std::string& fetch_holder_name) {
+  size_t fetch_count = 0;
+  for (auto* op : block->AllOps()) {
+    if (op->Type() == "fetch") {


op->Type() == "fetch" -> op->Type() == kFetchOpType

Xreki · 2018-01-25T11:46:08Z

paddle/framework/executor.cc

+          feed_targets.find(feed_target_name) != feed_targets.end(),
+          "Feed operator output name '%s' cannot be found in 'feed_targets'",
+          feed_target_name);
+      PADDLE_ENFORCE(op->GetAttr("col").type() == typeid(int),


Do we need to check this?

Removed this.

Xreki · 2018-01-25T11:56:50Z

paddle/framework/executor.cc

+                     "'fetch_holder_name' should match the program desc");
+      fetch_holder = var;
+    }
+  }


Line 228 - 238 is used to check whether there is already a feed_holder? In my opinion, if has_feed_operators() returns true, there should exist a feed_holder and we also check the feed_holder_name in has_feed_operators(). Maybe we can simplify the codes.

Great point! Have removed these codes and move the check of feed/fetch_holder_name in has_feed/fetch_operator().

Xreki · 2018-01-25T12:17:19Z

paddle/framework/executor.cc

+    int i = 0;
+    for (auto& feed_target : feed_targets) {
+      std::string var_name = feed_target.first;
+      LOG(INFO) << "feed target's name: " << var_name;


LOG(INFO) -> VLOG(3)

sidgoyal78

I think this looks good, and seems consistent with the Python version (as modified in #7712). We can merge this first and then I can modify the load() PR accordingly.
LGTM, but will prefer @Xreki to also approve.

Xreki · 2018-01-26T01:57:04Z

paddle/framework/executor.cc

+          "Feed operator output name '%s' cannot be found in 'feed_targets'",
+          feed_target_name);
+    } else {
+      break;


Remove the else branch. It seems not necessary here.

Xreki · 2018-01-26T02:02:44Z

paddle/framework/executor.cc

+}
+
+void Executor::Run(const ProgramDesc& program, Scope* scope,
+                   std::map<std::string, const LoDTensor*>& feed_targets,


Can we use const LoDTensor& here?

Reminded by @kexinzhao , it is impossible to use reference in std::map, https://stackoverflow.com/questions/4239253/c-is-it-possible-to-use-a-reference-as-the-value-in-a-map.

Xreki · 2018-01-26T02:03:35Z

paddle/framework/executor.cc

+      SetFeedVariable(scope, *feed_targets[feed_target_name], feed_holder_name,
+                      idx);
+    } else {
+      break;


Remove the else branch.

Xreki · 2018-01-26T02:22:18Z

@sidgoyal78 we can merge the PR first so that it will not block your work. We can fix the minor code aspects in next PR.

kexinzhao added 8 commits January 19, 2018 16:59

initial commit

4e7ce62

add new executor run function

466d40b

fix bug

9166de5

fix multiple definition of feed_fetch_method issue

a4d99fe

fix cmake

55b2e4a

fix tensor copy error

5f1c614

refine executor code

ab9b859

add comments

4d255f0

kexinzhao requested review from Xreki and sidgoyal78 January 24, 2018 01:39

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

Xreki requested a review from luotao1 January 24, 2018 01:50

kexinzhao added 2 commits January 24, 2018 15:27

temporary modification

9ff7578

Merge remote-tracking branch 'upstream/develop' into new_executor_run

e8f7777

Xreki reviewed Jan 25, 2018

View reviewed changes

kexinzhao added 2 commits January 25, 2018 12:11

address comments

3b8b462

fix bug

9a3b510

sidgoyal78 approved these changes Jan 26, 2018

View reviewed changes

Xreki reviewed Jan 26, 2018

View reviewed changes

kexinzhao merged commit 788f5c6 into PaddlePaddle:develop Jan 26, 2018

kexinzhao deleted the new_executor_run branch April 4, 2018 02:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Run() method for framework::Executor #7807

New Run() method for framework::Executor #7807

kexinzhao commented Jan 24, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

Xreki Jan 25, 2018

kexinzhao Jan 25, 2018

sidgoyal78 left a comment

Xreki Jan 26, 2018

Xreki Jan 26, 2018

Xreki Jan 26, 2018

Xreki Jan 26, 2018

Xreki commented Jan 26, 2018

New Run() method for framework::Executor #7807

New Run() method for framework::Executor #7807

Conversation

kexinzhao commented Jan 24, 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

Choose a reason for hiding this comment

sidgoyal78 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

Xreki commented Jan 26, 2018