graph_to_program topology sort by thisjiang · Pull Request #33949 · PaddlePaddle/Paddle

thisjiang · 2021-07-04T05:17:46Z

PR types

Bug fixes

PR changes

APIs

Describe

This PR solve two problem:

Try new topology sort function for graph_to_program, ensure graph node ordering same.
Allow multi-graph convert to multi-block program.(control by FLAG convert_all_blocks)
Other change (commit:45ed5ba merge from @zhhsplendid 's PR34289.
1. Sub-block can use variables from parent block and forward block if the sub-block is a backward block. The corresponding graph has nodes for the variables but we didn't pass VarDesc for those nodes, this PR added it. The key code is line 98 of graph.cc.
2. Due to i, we should distinguish the block for the nodes. We added block_id_ to node.h and related functional code. The key code is line 440 of graph.h
3. IsConstructedByPartialProgram() should return sub_graph's is_partial_ instead of returning this graph. The key code is line 107 of graph.h
4. while_op_eager_deletion_pass only handles MainGraph

As before, we can convert graph and program by the following code:

graph = fluid.core.Graph(before_program.desc)
ir_graph = fluid.framework.IrGraph(graph, for_test=False)
after_program = ir_graph.to_program()

The following are some examples:

y = paddle.abs(x)

y = paddle.increment(x)

cond = fluid.layers.greater_than(x, y)
ie = fluid.layers.IfElse(cond)
with ie.true_block():
    hidden = paddle.increment(t)
    ie.output(hidden)
with ie.false_block():
    hidden = paddle.increment(t)
    ie.output(hidden)

paddle-bot-old · 2021-07-04T05:17:49Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

levi131 · 2021-07-06T10:48:12Z

paddle/fluid/framework/ir/graph.cc

  for (auto *op : program.Block(0).AllOps()) {
    ir::Node *node = CreateOpNode(op);
+    node->SetDescOrder(desc_order);
+    ++desc_order;


45行可以和44行合并。

我觉得分开写更清晰~多一行少一行无所谓

levi131 · 2021-07-06T11:16:20Z

paddle/fluid/framework/ir/graph_helper.cc

+    }
+
+    if (out_ops.count(n) == 0) {
+      out_ops[n] = std::unordered_set<Node *>();


感觉只对out_ops[n]进行初始化是不够的，比如第一个op n 有很多个输入 n1， n2 ，这里就会访问out_ops[n1]和out_ops[n2].

如果只是为了初始化其实是没必要显式这么写的，unordered_map对于一个之前没遇到的key肯定先初始化再insert，这儿特意这样写的目的其实是为了确保out_ops和in_ops里会有所有的node。

为表述清晰，已单独拎出来，并使用at替换[]确保没有未知op

levi131 · 2021-07-06T11:17:36Z

paddle/fluid/framework/ir/graph_helper.cc

+  }
+
+  while (!q.empty()) {
+    auto cur_op = q.top();


const auto cur_op = q.top();

levi131 · 2021-07-06T11:23:36Z

paddle/fluid/framework/ir/graph_to_program_pass.cc

+    block = program_pb->add_blocks();
+    block->set_idx(idx);
+    GraphToBlock(graph->GetSubGraph(idx), block);
+  }


这个for循环是否也应该放在 if (FLAGS_convert_all_blocks) { } 中

可以的，之前我想的是如果没指定FLAGS_convert_all_blocks的话SubGraphSize的返回值应该是0，这样就不会走循环了。不过加上可以逻辑会更清晰点

levi131 · 2021-07-06T11:28:29Z

paddle/fluid/framework/ir/node.h


 #pragma once

+#include <cstdint>


注意到 id 是int类型的，如果desc_order也使用int类型，是不是就不用引入这个头文件了

可以的，desc_order_用int或者size_t应该都行

levi131 · 2021-07-06T11:30:33Z

paddle/fluid/framework/ir/node.h

  int id_;

+  static constexpr size_t NO_DESC_ORDER = SIZE_MAX;
+  size_t desc_order_;


为什么不在这里赋默认值而是在构造函数的初始化列表中赋值呢。

C++中非static const修饰的成员变量是不能在定义时赋初值的（有些编译器是直接报错有些是忽略这个初值），只能在构造函数里赋值。

嗨，看到你们的讨论，插入一下。

查看了一下，似乎是C++ 11后引入的可以在header中定义赋值。

https://stackoverflow.com/questions/24149924/has-the-new-c11-member-initialization-feature-at-declaration-made-initializati

是的，后续有一些讨论我和姜程在如流上沟通了

levi131 · 2021-07-06T11:35:31Z

paddle/fluid/framework/ir/graph.h

+  }
+
+  size_t SubGraphSize() const {
+    // wait for https://github.com/PaddlePaddle/Paddle/pull/33320 merged


这里需要的接口，在#33320 中写一个comment吧

… add_graph_to_program_toposort

zhhsplendid

LGTM

chenwhql

LGTM for graph&node

try whether topology sort can solve speed decease problem

770923f

thisjiang added 3 commits July 6, 2021 08:12

add multi block topo function

1450bda

add convert_all_blocks flags

8f9e886

solve synax bug

3e8f358

levi131 reviewed Jul 6, 2021

View reviewed changes

thisjiang added 18 commits July 6, 2021 12:20

optimize some description

1f3101c

optimize sort function

e9e98fe

optimize function name and solve some bug

89b98e8

add single test script

af57fdd

remove IsParameter and StopGradient test for PR33771 not merged

3de8418

using self-define string instead of protobuf's ShortDebugString

a109c74

solve conflict

0484189

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

84c8ed2

… add_graph_to_program_toposort

Fix Bug: node.ToString consider OpHandle, an Op but hasn't OpDesc.

eb5b8cd

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

d154917

… add_graph_to_program_toposort

add flag control TopologySort function

2f4f1b7

replace scale_loss_grad op to fill_constant in graph2program

9e691a5

add single test script for CI converage

819c1e2

fix node test bug

ec23b38

complete node test script for node.ToString test

b3ebb83

avoid redundant copy of scalelossgrad op replace

c89fa02

remove GetBlockId

4fa0bdf

merge branch zhhsplendid:ci_test_convert_all_block PaddlePaddle#34289

45ed5ba

zhhsplendid approved these changes Jul 27, 2021

View reviewed changes

chenwhql approved these changes Jul 28, 2021

View reviewed changes

zhhsplendid merged commit 167523e into PaddlePaddle:develop Jul 28, 2021

thisjiang deleted the add_graph_to_program_toposort branch July 28, 2021 07:10

Conversation

thisjiang commented Jul 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR types

PR changes

Describe

Uh oh!

paddle-bot-old bot commented Jul 4, 2021

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!

zhhsplendid left a comment

Choose a reason for hiding this comment

Uh oh!

chenwhql left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thisjiang commented Jul 4, 2021 •

edited

Loading