Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add remove_op, remove_var in Python end #9816

Merged
merged 1 commit into from
Apr 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 2 additions & 48 deletions paddle/fluid/framework/block_desc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,10 @@ See the License for the specific language governing permissions and
limitations under the License. */

#include "paddle/fluid/framework/block_desc.h"
#include <queue>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be above those "" includes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the result of clang-format. When I move line16 above line 15, the clang-format will change to this version.

The reason is that block_desc.h is Related header, but <queue> is C++ library. The order is: Related header, C library, C++ library, other libraries' .h, your project's .h. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you.

#include "paddle/fluid/framework/operator.h"
#include "paddle/fluid/framework/program_desc.h"

#include <queue>

namespace paddle {
namespace framework {

Expand Down Expand Up @@ -147,52 +146,7 @@ void BlockDesc::RemoveOp(size_t s, size_t e) {
if (ops_.begin() + s == ops_.end() || ops_.begin() + e == ops_.end()) {
return;
}
auto get_vars = [](std::deque<std::unique_ptr<OpDesc>>::iterator &op,
std::vector<std::string> &v) {
auto in_names = (*op)->InputArgumentNames();
v.insert(v.end(), in_names.begin(), in_names.end());
auto out_names = (*op)->OutputArgumentNames();
v.insert(v.end(), out_names.begin(), out_names.end());
std::sort(v.begin(), v.end());
auto last = std::unique(v.begin(), v.end());
v.erase(last, v.end());
};
need_update_ = true;

for (size_t i = s; i < e; i++) {
// since remove op one by one, every time remove the first op.
auto op = ops_.begin() + s;

// collect input and output variables from current delete op
std::vector<std::string> cur_vars;
get_vars(op, cur_vars);

// remove current op
ops_.erase(ops_.begin() + s);

// collect input and output variables from other ops
std::vector<std::string> other_vars;
for (auto it = ops_.begin(); it != ops_.end(); it++) {
get_vars(it, other_vars);
}

// variables should be deleted
std::vector<std::string> delete_vars;
// delete_vars = cur_vars - cur_vars ^ other_input_vars
std::set_difference(cur_vars.begin(), cur_vars.end(), other_vars.begin(),
other_vars.end(),
std::inserter(delete_vars, delete_vars.end()));
// remove variables
for (size_t i = 0; i < delete_vars.size(); i++) {
auto name = delete_vars[i];
auto it = vars_.find(name);
PADDLE_ENFORCE(it != vars_.end(),
"%s is not in variable list, it should not be deleted",
name);
vars_.erase(it);
VLOG(3) << "deleting variable " << name;
}
}
ops_.erase(ops_.begin() + s, ops_.begin() + e);
}

std::vector<OpDesc *> BlockDesc::AllOps() const {
Expand Down
11 changes: 11 additions & 0 deletions python/paddle/fluid/framework.py
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,11 @@ def rename_var(self, name, new_name):
del self.vars[name]
self.sync_with_cpp()

def remove_var(self, name):
self.sync_with_cpp()
self.desc.remove_var(name)
del self.vars[name]

def create_parameter(self, *args, **kwargs):
global_block = self.program.global_block()
param = Parameter(global_block, *args, **kwargs)
Expand All @@ -838,6 +843,11 @@ def insert_op(self, index, *args, **kwargs):
self.ops.insert(index, op)
return op

def remove_op(self, index):
self.sync_with_cpp()
self.desc.remove_op(index, index + 1)
del self.ops[index]

def delete_ops(self, ops):
# remove from cpp
# FIXME(typhoonzero): remove only the first occurrence.
Expand All @@ -846,6 +856,7 @@ def delete_ops(self, ops):
end = list(self.ops).index(ops[-1])
except Exception, e:
raise e

self.desc.remove_op(start, end + 1)

def slice_ops(self, start, end):
Expand Down
20 changes: 0 additions & 20 deletions python/paddle/fluid/tests/unittests/test_protobuf_descs.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,33 +201,13 @@ def test_remove_op(self):
op1.set_type("test")
op2.set_type("test")

var0 = block.var("var0")
var1 = block.var("var1")
var2 = block.var("var2")
var3 = block.var("var3")
var4 = block.var("var4")
var5 = block.var("var5")

op0.set_input("X", ["var0"])
op0.set_output("Y", ["var0"])
op1.set_input("X", ["var1", "var2"])
op1.set_output("Y", ["var3", "var4"])
op2.set_input("X", ["var1"])
op2.set_output("Y", ["var4", "var5"])

program.sync_with_cpp()

# remove op1, its input var2 and output var3 will be removed at the same time,
# but its input var1 and output var4 will not be removed since they are used for op2.
block.remove_op(1, 2)
program.sync_with_cpp()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does sync_with_cpp need to be updated to a simpler version that clear all ops and add back the descs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @jacquesqiao, this simpler version is so dangerous since it will clear all ops.

If we create an op1 first, then call insert_op, and then using op1 will fail. The reason is that all the instances in Python end are removed, they are changed to C++ instances instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That make sense, I see sync_with_cpp have already deleted those clear operations.


all_ops = []
for idx in xrange(0, block.op_size()):
all_ops.append(block.op(idx))
self.assertEqual(all_ops, [op0, op2])
all_vars = block.all_vars()
self.assertEqual(set(all_vars), {var0, var1, var4, var5})


if __name__ == '__main__':
Expand Down