[draft] add registry for Op, OpProto and OpAttrChecker #2739

JiayiFeng · 2017-07-04T12:49:18Z

This is a draft to show our design about how to implement registry mechanisms of Op, OpProto and OpAttrChecker.

For convenience, a small part of attr_checker.h is written with pseudocode. It is indicated by comments ==pseudocode begin== and ==pseudocode end==.

Superjomn · 2017-07-05T01:46:12Z

paddle/framework/op_registry.h

+};
+
+class OpRegistry {
+  typedef std::function<OperatorBase*(OpDesc& op_desc)> OpCreator;


const OpDesc& ?

In TypedAttrChecker::operator(), we check attributes and complete default value, so the OpDesc will be mutated in inner checkers.

so, OpDesc& -> OpDesc* better ?

Superjomn · 2017-07-05T01:47:12Z

paddle/framework/op_registry.h

+    ProtoMakerType(&op_proto, &op_checker);
+  }
+
+  static OpBase* CreateOp(const std::string& op_type, OpDesc op_desc) const {


const OpDesc& ?

As mentioned above~

Superjomn · 2017-07-05T01:50:11Z

paddle/framework/op_registry.h

+  }
+};
+
+#define REGISTER_OP(op_class, op_maker_class, op_type)             \


op_class -> __op_class ? other arguments are the same
a different name method to indicate these arguments are used in macro.

Got it. Thanks!

Superjomn · 2017-07-05T01:51:46Z

paddle/framework/op_registry.h

+  }
+};
+
+REGISTER_OP(CosineOp, CosineOpProtoAndCheckerMaker, "cos");


cos -> cos_sim or cossim ?

… dev_registry

reyoung · 2017-07-05T03:48:59Z

paddle/framework/attr_checker.h

+  bool operator()(OpDesc& op_desc) const {
+    //========= pseudocode begin ==========//
+    // check whether attr need default value;
+    if (!op_disc.has_fild(attr_name_)) {


op_disc -> op_desc?

reyoung · 2017-07-05T04:50:08Z

paddle/framework/attr_checker.h

+class LargerThanChecker {
+ public:
+  LargerThanChecker(T lower_bound) : lower_bound_(lower_bound) {}
+  bool operator()(const T& value) const { return value > lower_bound_; }


Maybe bool is not enough to give the user a details information.

There can be two options:

Use PADDLE_ENFORCE and return void.

Use std::string as return value. An empty string represents OK, others represent Errors.

Also, I think the interface of this function could be

(T* attr, bool alreadySet) -> void

And then the Default can be a function, too. Like

[](T* attr, bool alreadySet) { if (!alreadySet) { // not set by user *attr = xxx; // set by us. } }

and Required can be a function, too.

[](T* attr, bool alreadySet) { if (!alreadySet) { // not set by user return "error!!"; } }

reyoung · 2017-07-05T05:02:54Z

paddle/framework/attr_checker.h

+ private:
+  std::string attr_name_;
+  std::vector<ValueChecker> value_checkers_;
+  bool has_default_;


Not only Default is needed, but also Required is needed too.

reyoung · 2017-07-05T05:31:12Z

paddle/framework/op_registry.h

+   private:                                                        \
+    const static OpRegisterHelper<#op_class, #op_maker_class> reg; \
+  };                                                               \
+  const Register op_class##Register::reg(#op_type);


Maybe we should move paddle/util/InitFunction here.

reyoung · 2017-07-05T06:47:43Z

既然我们从protobuf解析出Attribute的用来传递给C++的OpBase。并且在解析完毕传递之前，需要做一些约束检查。

那么，我们为啥不先将Protobuf中的所有Attributes解析成标准C++类型，以便之后进一步处理。而约束检查函数都以C++类型为输入类型就好。

可能的C++类型为:

// boost::variant can be replaced by our implementaiton
using Attribute = boost::variant<boost::blank,  int, float, std::string, std::vector<int>, std::vector<float>, std::vector<std::string>>;
using AttributeMap = std::unordered_map<std::string, Attribute>;
using AttributeConstraint = std::function<void(Attribute* attr)>;

那么，在注册OpCreator的时候，注册的函数可以为:

std::unordered_map<string, std::vector<AttributeConstraint>> attr_constraints_;
auto creator = [] (const OpDesc& desc) {
  AttributeMap attrs = ParseProtoAttrs(desc.attrs());
  for (auto it = attr_constraints_.begin(); it != attr_constraints_.end(); ++it) {
     auto attr_name = it->first;
     auto& constraints = it->second;
     for (auto& cons : constraints) {
        cons(&attrs[attr_name]);  // if attr_name is not exist, a boost::variant<blank> will be created.
     }
  }

  return new OpBase(desc.inputs(), desc.outputs(), attrs);  // just pass attrs to OpBase.
}

这样做的好处是:

解析检查参数的过程注册在了OpCreator中，对于Op的Developer和框架的使用者，都是透明的。
最开始就讲protobuf的Attribute转换成C++类型，后续的读取，查找操作都可以保证时间复杂度很小。
简化了AttributeConstraint或者Checker的设计，将其变成一个很简单的，可以读写Attribute函数。

这个实现依赖了boost::variant, 不过我们可以给用户传一个Attributes的接口，进而隔绝Op开发者直接使用boost::variant。譬如

class Attributes {
public:
  // also, maybe argument use `AttributeMap` is better than `const T&`, because compiler might RVO
  Attributes(AttributeMap attrs): attrs_(attrs) {}

  template<typename T>
  T Get(const std::string& name) const;

private:
  AttributeMap attrs_;
};
class OpBase {
public:
  // In Op, user can only use Attributes::Get method.
  // We can implement Attribute by boost::variant firstly, then change to our implementation,
  // if we think boost::variant is heavy.
  OpBase(std::unique_ptr<Attributes> attrs) {}
};

JiayiFeng · 2017-07-05T09:29:57Z

仔细想了一下 @reyoung 提出的建议。和我目前的代码实现的主要差异在于：

Attribute从Protobuf解析出来后会以boost::variant封装，以兼容多种可能的类型。在我的实现中，Attribute先从boost::variant中取出，转化成确定类型的值，再接受对应类型的各项checker的检查。在 @reyoung 的建议中，checker写成boost的visitor的形式，因此可以直接接受boost::variant类型的Attribute，无需确定类型。

两种方法最后的代码实现应该没有什么大的差别，就是底层checker是写成模板类还是boost visitor的区别。

除此之外， @reyoung 还建议将default的设置也抽取出来做成一个checker，以及用PADDLE_ENFORCE报错，这对我很有启发性，我稍后修改一下

…Map` is a unordered_map of <string, Attribute>, and `Attribute` is a boost::variant object to hold multiple types of attribute value. 2. Use `PADDLE_ENFORCE` to print checkers' fail message. 3. Abstract default value operations to a new function: `DefaultChecker`.

reyoung · 2017-07-06T04:42:26Z

paddle/framework/attr_checker.h

+    }
+    T& attr_value = boost::get<T>(attr);
+    for (const auto& checker : value_checkers_) {
+      checker(attr_value);


The advantage of this design is checker contains a template parameter T.

QiJune · 2017-07-06T04:43:00Z

paddle/framework/attr_checker.h

+ private:
+  std::string attr_name_;
+  std::vector<ValueChecker> value_checkers_;
+  std::vector<ValueChecker> default_checker_;


default_checker_ is a little confusing, maybe default_setter_ would be better.

reyoung · 2017-07-06T04:45:49Z

paddle/framework/attr_checker.h

+  }
+
+  void operator()(AttributeMap& attr_map) const {
+    Attribute& attr = attr_map.at(attr_name_);


.at will not construct a default Attribute.

See

http://en.cppreference.com/w/cpp/container/map/operator_at

http://en.cppreference.com/w/cpp/container/map/at

reyoung · 2017-07-06T04:47:44Z

paddle/framework/attr_checker.h

+// an attribute can have more than one limits
+template <typename T>
+class TypedAttrChecker {
+  typedef std::function<void(const T&)> ValueChecker;


Maybe not important, but using is better than typedef.

http://blog.csdn.net/big_yellow_duck/article/details/52224068

reyoung · 2017-07-06T04:49:37Z

paddle/framework/attr_checker.h

+ public:
+  TypedAttrChecker(const std::string& attr_name) : attr_name_(attr_name) {}
+
+  TypedAttrChecker& LargerThan(const T& lower_bound) {


Maybe we should provide a base API named

AddChecker(ValueChecker checker) { checkers_.push_back(checker); return *this; }

Other API will invoke AddChecker

Maybe AddCustomChecker has done the same thing?

TypedAttrChecker& AddCustomChecker(const ValueChecker& checker) { value_checkers_.push_back(checker); return *this; }

reyoung · 2017-07-06T04:51:13Z

paddle/framework/attr_checker.h

+    return *(checker.target<TypedAttrChecker<T>>())
+  }
+
+  void Check(AttributeMap& attr_map) {


Maybe Check could mark to const?

reyoung · 2017-07-06T04:51:44Z

paddle/framework/attr_checker.h

+  }
+
+  void Check(AttributeMap& attr_map) {
+    for (const auto& checker : attr_checkers_) {


Also, const is not need if Check is marked as const

ZZ

QiJune · 2017-07-06T10:06:12Z

paddle/framework/op_registry.h

+   private:                                                            \
+    const static OpRegisterHelper<#__op_class, #__op_maker_class> reg; \
+  };                                                                   \
+  const Register __op_class##Register::reg(#__op_type);


What's the meaning of this line? const Register?

miss type... It should be OpRegisterHelper<#__op_class, #__op_maker_class>

1. Complete the development of interfaces between OpRegistry and Protobuf. 2. Add unit test for op_registry.h

jacquesqiao

LGTM

JiayiFeng added 3 commits July 3, 2017 19:17

init op_registry.h

dd7fcb4

dev op_registry.h

4b991f1

add 'attr_checker.h', which is a draft of op attribute checker.

c4cc5e0

JiayiFeng requested review from Superjomn, reyoung, wangkuiyi, jacquesqiao, dzhwinter, luotao1, qingqing01 and QiJune July 4, 2017 12:52

Superjomn reviewed Jul 5, 2017

View reviewed changes

JiayiFeng added 2 commits July 5, 2017 10:42

rename some macro parameters

dd2479e

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

2c37e68

… dev_registry

reyoung reviewed Jul 5, 2017

View reviewed changes

reyoung reviewed Jul 6, 2017

View reviewed changes

QiJune reviewed Jul 6, 2017

View reviewed changes

reyoung reviewed Jul 6, 2017

View reviewed changes

rename DefaultChecker to DefaultValueSetter

a9923fd

ZZ

QiJune reviewed Jul 6, 2017

View reviewed changes

JiayiFeng added 4 commits July 6, 2017 21:05

Finish op_registry

4eff5b6

1. Complete the development of interfaces between OpRegistry and Protobuf. 2. Add unit test for op_registry.h

Add demo and test of custome checker

fd46805

fix merge conflict

c3c82e6

fix merge conflict

28e7cb3

jacquesqiao approved these changes Jul 7, 2017

View reviewed changes

jacquesqiao merged commit 1d2ef1d into PaddlePaddle:develop Jul 7, 2017

JiayiFeng deleted the dev_registry branch July 7, 2017 06:21

reyoung added this to Done in PaddlePaddle Refactoring: Phase 1 Aug 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[draft] add registry for Op, OpProto and OpAttrChecker #2739

[draft] add registry for Op, OpProto and OpAttrChecker #2739

JiayiFeng commented Jul 4, 2017 •

edited

Loading

Superjomn Jul 5, 2017

JiayiFeng Jul 5, 2017

Superjomn Jul 6, 2017

Superjomn Jul 5, 2017

JiayiFeng Jul 5, 2017

Superjomn Jul 5, 2017

JiayiFeng Jul 5, 2017

Superjomn Jul 5, 2017

reyoung Jul 5, 2017

reyoung Jul 5, 2017

reyoung Jul 5, 2017

reyoung Jul 5, 2017

reyoung Jul 5, 2017

reyoung commented Jul 5, 2017 •

edited

Loading

JiayiFeng commented Jul 5, 2017 •

edited

Loading

reyoung Jul 6, 2017

QiJune Jul 6, 2017

JiayiFeng Jul 6, 2017

reyoung Jul 6, 2017

JiayiFeng Jul 6, 2017

reyoung Jul 6, 2017

reyoung Jul 6, 2017

JiayiFeng Jul 6, 2017 •

edited

Loading

reyoung Jul 6, 2017

reyoung Jul 6, 2017

JiayiFeng Jul 6, 2017

QiJune Jul 6, 2017

JiayiFeng Jul 6, 2017

jacquesqiao left a comment

[draft] add registry for Op, OpProto and OpAttrChecker #2739

[draft] add registry for Op, OpProto and OpAttrChecker #2739

Conversation

JiayiFeng commented Jul 4, 2017 • edited Loading

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

reyoung commented Jul 5, 2017 • edited Loading

JiayiFeng commented Jul 5, 2017 • edited Loading

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

JiayiFeng Jul 6, 2017 • edited Loading

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

jacquesqiao left a comment

Choose a reason for hiding this comment

JiayiFeng commented Jul 4, 2017 •

edited

Loading

reyoung commented Jul 5, 2017 •

edited

Loading

JiayiFeng commented Jul 5, 2017 •

edited

Loading

JiayiFeng Jul 6, 2017 •

edited

Loading