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.

Sign up for GitHub

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

Jump to bottom

check duplicate of ProtoAndCheckerMaker #2903

Merged
merged 4 commits into from
Jul 17, 2017
Merged

check duplicate of ProtoAndCheckerMaker #2903

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14a84d9
check duplicate of ProtoAndCheckerMaker
jacquesqiao Jul 16, 2017
e6af84a
input/output/attr should not have duplicate name
jacquesqiao Jul 17, 2017
d7e6b40
optimize code
jacquesqiao Jul 17, 2017
bec80dc
fix cpp style
jacquesqiao Jul 17, 2017
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
31 changes: 28 additions & 3 deletions paddle/framework/op_registry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,15 @@ class OpProtoAndCheckerMaker {
OpProtoAndCheckerMaker(OpProto* proto, OpAttrChecker* op_checker)
: proto_(proto), op_checker_(op_checker) {}

~OpProtoAndCheckerMaker() { CheckNoDuplicatedAttrs(); }
~OpProtoAndCheckerMaker() {
PADDLE_ENFORCE(validated_, "should call Validate after build");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just push that validation in the destructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

because Exception in destructor can not be caught, so it can not be tested~

}

void Validate() {
validated_ = true;
CheckNoDuplicatedAttrs();
CheckNoDuplicatedInOut();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not only attributes but also input and output together could not be duplicated.

So maybe one function named CheckNoDuplicatedInOutAttrs is better?

}

protected:
void AddInput(const std::string& name, const std::string& comment,
Expand Down Expand Up @@ -171,11 +179,27 @@ Add a mark to which output is temporary is helpful for future optimization.
++cnt;
}
PADDLE_ENFORCE(names.size() == cnt,
"Cannot register two attribute in same name!");
"Cannot register two attribute with same name!");
}

void CheckNoDuplicatedInOut() {
std::unordered_set<std::string> names;
size_t cnt = 0;
for (auto& input : proto_->inputs()) {
names.insert(input.name());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use:

for (auto& input : proto_->inputs()) {
  std::string input_name = input.name();
  PADDLE_ENFORCE(!names.count(input_name), "%s is duplicated!", input_name);
  names.insert(input_name);
}

In this way, we are able to be informed about which one is duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, it's a little slower~

++cnt;
}
for (auto& output : proto_->outputs()) {
names.insert(output.name());
++cnt;
}
PADDLE_ENFORCE(names.size() == cnt,
"Cannot register two Input or Output with same name!");
}

OpProto* proto_;
OpAttrChecker* op_checker_;
bool validated_{false};
bool has_multiple_input_{false};
bool has_multiple_output_{false};
bool has_temporary_output_{false};
Expand All @@ -190,7 +214,8 @@ class OpRegistry {
creators()[op_type] = [] { return new OpType; };
OpProto& op_proto = protos()[op_type];
OpAttrChecker& op_checker = op_checkers()[op_type];
ProtoMakerType(&op_proto, &op_checker);
auto maker = ProtoMakerType(&op_proto, &op_checker);
maker.Validate();
*op_proto.mutable_type() = op_type;
PADDLE_ENFORCE(
op_proto.IsInitialized(),
Expand Down
36 changes: 34 additions & 2 deletions paddle/framework/op_registry_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#include "paddle/framework/op_registry.h"
#include <gtest/gtest.h>

namespace pd = paddle::framework;

namespace paddle {
namespace framework {
class CosineOp : public OperatorBase {
Expand Down Expand Up @@ -28,8 +30,6 @@ class MyTestOp : public OperatorBase {
void InferShape(const ScopePtr& scope) const override {}
void Run(const ScopePtr& scope,
const platform::DeviceContext& dev_ctx) const override {}

public:
};

class MyTestOpProtoAndCheckerMaker : public OpProtoAndCheckerMaker {
Expand Down Expand Up @@ -182,3 +182,35 @@ TEST(OpRegistry, CustomChecker) {
int test_attr = op->GetAttr<int>("test_attr");
ASSERT_EQ(test_attr, 4);
}

class TestAttrProtoMaker : public pd::OpProtoAndCheckerMaker {
public:
TestAttrProtoMaker(pd::OpProto* proto, pd::OpAttrChecker* op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddAttr<float>("scale", "scale of test op");
AddAttr<float>("scale", "scale of test op");
}
};

TEST(ProtoMaker, DuplicatedAttr) {
pd::OpProto op_proto;
pd::OpAttrChecker op_checker;
auto proto_maker = TestAttrProtoMaker(&op_proto, &op_checker);
ASSERT_THROW(proto_maker.Validate(), paddle::framework::EnforceNotMet);
}

class TestInOutProtoMaker : public pd::OpProtoAndCheckerMaker {
public:
TestInOutProtoMaker(pd::OpProto* proto, pd::OpAttrChecker* op_checker)
: OpProtoAndCheckerMaker(proto, op_checker) {
AddInput("input", "input of test op");
AddInput("input", "input of test op");
}
};

TEST(ProtoMaker, DuplicatedInOut) {
pd::OpProto op_proto;
pd::OpAttrChecker op_checker;
auto proto_maker = TestInOutProtoMaker(&op_proto, &op_checker);
ASSERT_THROW(proto_maker.Validate(), paddle::framework::EnforceNotMet);
}