Design doc for operator attribute #2606

reyoung · 2017-06-26T09:36:54Z

The design doc for operator attribute. Maybe here is better for review.

* Add background.

reyoung · 2017-06-27T07:00:47Z

This is a subset of PR #2555, we split #2555 into many parts for review.

jacquesqiao · 2017-06-27T07:35:11Z

doc/design/attribute.md

+```cpp
+class OperatorBase {
+ public:
+  Error InitializeAttribute(const AttributeReader& attrs) = 0;


jacquesqiao · 2017-06-27T07:37:05Z

doc/design/attribute.md

+  explicit AttributeReader(const AttributeMap& attrs) : attrs_(attrs) {}
+
+  template <typename T>
+  Error __must_check Get(const std::string& attributeName, T* attr) const;


what will attr point to if Error happens?

The attr is always pointing to an outside variable. The pointer is not modified in Get method.

oh, so the value T* attr point to just not change, is that right?

The pointer attr is will never be changed inside Get method. The value *attr will not be changed if an error occurred. The value *attr will be set to correct value when there is no error.

jacquesqiao · 2017-06-27T07:40:40Z

doc/design/attribute.md

+ public:
+  Error InitializeAttribute(const AttributeReader& attrs) {
+    auto err = attrs.Get<float>("scale", &scale_);
+    if (!err.isOK() && err != "Attribute Not Found") {


What will happen if err == "Attribute Not Found" ?

If the error is Attribute Not Found, then the default value is used.

The default value is set in private data member, like float scale_{1.0};.

I think If the error is Attribute Not Found, then the default value is used. should be added to comment.
Another thing is that remind the user to set a default value for their attributes.

jacquesqiao

LGTM！

wangkuiyi

It seems that the writing could be shortened. Here is my trial:

An operator could have attributes. For example, CosineOp could have a float typed attribute scale, which changes the output range from [-1,1] to [-scale,scale].

Attributes is defined by a name and a type. An instance of an attribute has a value of that type.

As part of the network description, attribute need to be serialized. So we need a protobuf message that describes an attribute, say AttributeProto.

An operator could list its attributes in a data member std::map<string/*name*/, AttributeProto*>.

wangkuiyi · 2017-06-28T01:15:01Z

doc/design/attribute.md

@@ -0,0 +1,125 @@
+# Design Doc about operator attribute
+
+## background


wangkuiyi · 2017-06-28T01:15:14Z

doc/design/attribute.md

@@ -0,0 +1,125 @@
+# Design Doc about operator attribute


Design Doc: Operator Attributes

wangkuiyi · 2017-06-28T01:18:56Z

doc/design/attribute.md

+
+## background
+
+In a neural network, each operator could contain some configurable attributes. For example, a cosine similarity operator may contain an attribute named `scale`. The default cosine similarity returns a value in range [-1.0, 1.0]. But the user can set range scale manually, e.g., user set `scale=5.0`, then that cosine operator will return a value in the range [-5.0, 5.0].


In a neural network, each operator could contain some configurable attributes.

==>

Operators have attributes.

wangkuiyi · 2017-06-28T01:19:16Z

doc/design/attribute.md

+
+In a neural network, each operator could contain some configurable attributes. For example, a cosine similarity operator may contain an attribute named `scale`. The default cosine similarity returns a value in range [-1.0, 1.0]. But the user can set range scale manually, e.g., user set `scale=5.0`, then that cosine operator will return a value in the range [-5.0, 5.0].
+
+The configurable attributes could be various types. Some operators need `float` value to configure; some need `string` value.  We need a data structure to represent different types.


be of various types

wangkuiyi · 2017-06-28T01:20:41Z

doc/design/attribute.md

+
+In a neural network, each operator could contain some configurable attributes. For example, a cosine similarity operator may contain an attribute named `scale`. The default cosine similarity returns a value in range [-1.0, 1.0]. But the user can set range scale manually, e.g., user set `scale=5.0`, then that cosine operator will return a value in the range [-5.0, 5.0].
+
+The configurable attributes could be various types. Some operators need `float` value to configure; some need `string` value.  We need a data structure to represent different types.


The configurable attributes could be various types. Some operators need float value to configure; some need string value. We need a data structure to represent different types.

==>

Each attributes has a name and a type. We are going to describe them in a protobuf message.

wangkuiyi · 2017-06-28T01:22:20Z

doc/design/attribute.md

+
+The configurable attributes could be various types. Some operators need `float` value to configure; some need `string` value.  We need a data structure to represent different types.
+
+Each operator contains different configurable attributes. The names of attributes are not same.  We need an associate map from attribute name to attribute value for `Operator`.


Each operator contains different configurable attributes. The names of attributes are not same. We need an associate map from attribute name to attribute value for Operator.

==>

We plan to list attributes of an operator in an associate map: map<string, Attribute*>.

In protobuf, the type of Attributes is a map<string, Attribute> attrs; But in each op class, it stores simple strong typed values as member data.

class CosineOp { public: void Init(const AttributeReader& reader) { scale_ = reader.Get<float>("scale") } private: float scale_; };

If we extract scale from protobuf map every time when we use it, it is very slow.

QiJune · 2017-06-28T02:59:55Z

doc/design/attribute.md

+```
+
+There are two methods in `AttributeReader`: `Get` and `GetArray`. `GetArray` is used for `ListValue`, and `Get` is used for the rests. The user should invoke either of them when he wants to get an Attribute value from `AttributeMap`.
+


The ListValue message is defined as:

message ListValue { repeated int32 ints = 1; repeated float floats = 2; repeated string strings = 3; }

And GetArray method only has one template parameter T. So, how the GetArray get ListValue with various type.

Only one of ListValue field should be set. The protobuf 3 currently cannot make repeated field inside a oneof. So just make all ListValue a single message.

The tensorflow has a similar implementation

* Simplify background description. * Not using Error as return value, use PADDLE_ENFORCE

…ute_design_for_op

jacquesqiao · 2017-06-29T09:27:39Z

paddle/framework/attribute_reader.h

+};
+
+/// Implementation of Contain
+namespace details {


why named details ?

Because we need define some function that should not be used by the user but can only be defined in the header file.

So mark them in namespace details could tell the user not to use them directly.

jacquesqiao · 2017-06-29T09:48:36Z

paddle/framework/attribute_reader.h

+   * mismatch or not contains that name.
+   */
+  template <typename T>
+  bool Contain(const std::string& name) const;


It seems that most lib name this Contians
https://docs.oracle.com/javase/7/docs/api/java/util/List.html#contains(java.lang.Object)

jacquesqiao · 2017-06-29T10:45:45Z

paddle/framework/attribute_reader_test.cc

+  reader.GetArray("listInt", &actual);
+  reader.GetArray("listStr", &actualStr);
+  ASSERT_EQ(expected, actual);
+  ASSERT_EQ(expectedStr, actualStr)


need ";" at the end of this line

I found it. Done.

Add copy right and comments Update design doc

jacquesqiao · 2017-06-29T12:34:22Z

paddle/framework/attribute_reader.h

+    std::copy(field.begin(), field.end(), std::back_inserter(*vec));     \
+  }
+
+ATTR_GETARRAY_IMPL(int, ints);


to be with the same name style with ATTR_READER_GETVALUE_IMPL and ATTR_READER_ISTYPE_IMPL

ATTR_GETARRAY_IMPL => ATTR_READER_GETARRAY_IMPL

jacquesqiao · 2017-06-30T00:10:18Z

paddle/framework/attribute_reader.h

+   * throw
+   */
+  template <typename T>
+  T Get(const std::string& name) const;


just discuss, do we need a

T Get(const std::string& name, const T& default) const;

to set a default value

I see the design doc, people can use Contains to set a default value. So this is not a problem.

wangkuiyi · 2017-06-30T00:47:44Z

doc/design/attribute.md

+    }
+
+    oneof value {
+        ListValue list = 1;


The oneof directive doesn't seem help much here -- it can locate a field in value but it cannot help us locate a field in ListValue. If our C++ code would still have to check which field is the one in ListValue, it would be easier just use the old plain syntax:

syntax = "proto3"; message Attribute { enum Type { INTS = 0; FLOATS = 1; STRINGS = 2; INT = 3; FLOAT = 4; STRING = 5; } Type type = 1; repeated int32 ints = 2; repeated float floats = 3; repeated string strings = 4; int32 int = 5; float float = 6; string string = 7; }

syntax = "proto3"; message Attribute { repeated int32 ints = 1; repeated float floats = 2; repeated string strings = 3; optional int32 int = 4; optional float float = 5; optinoal string string = 6; }

Maybe the Type is not useful, because we could use optional field and has_xxx() to detect AttributeType.

wangkuiyi · 2017-06-30T00:55:20Z

doc/design/attribute.md

+
+There are two frameworks implement `Attribute` concept in `protobuf`. They are [`caffe2`](https://github.com/caffe2/caffe2/blob/master/caffe2/proto/caffe2.proto#L98) and [`tensorflow`](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/framework/attr_value.proto#L16).
+
+* Caffe2 uses `proto2` syntax. It treats all attributes as a list, and each attribute contains a `name`. Each time caffe2 read an attribute is searching a variable in a list. It is slow if the number of attributes is large. Caffe2 also mark all field as `optional`. It doesn't ensure `one of` attribute value is set.


Surprise to know that Caffe2 searches attributes in a protobuf list. The right way is to load from protobuf field repeated Attrbribute attrs = x; into C++ data structure map<string, Attribute> and search in the C++ data structure.

It's my mistake, Caffe2 is loading all attribute into memory first.

wangkuiyi · 2017-06-30T00:59:08Z

doc/design/attribute.md

+}
+```
+
+In `OperatorDescription` message, there should be a field like this:


OperatorDescription => OperatorDesc ?

wangkuiyi · 2017-06-30T01:01:21Z

doc/design/attribute.md

+
+```cpp
+using AttributeMap = google::protobuf::Map<std::string, Attribute>;
+class AttributeReader {


AttributeReader => Attributes

This is not a reader, it holds and searches attributes as well.

wangkuiyi · 2017-06-30T01:07:22Z

doc/design/attribute.md

+  T Get(const std::string& name) const;
+
+  // Get attribute with a array of type T, which name is `name`
+  template <typename T>


What if an attribute was marked int in the protobuf message, but the user tries to retrieve its string value? How can we check and find such kind of errors?

Because we use optional, we can detect which type is set in protobuf, and assert fail when type mismatch.

wangkuiyi · 2017-06-30T01:16:16Z

doc/design/attribute.md

+  // Is that `name` attribute with type T in map or not.
+  // T could be int, float, string and std::vector of them
+  template <typename T>
+  bool Contains(const std::string& name) const;


Contains => IsType

For example,

if (IsType<std::vector<string>>("name")) { ... }

Actually, maybe it would be easier to have Has and Type instead

class Attributes { public: bool Has(const std::string& name) const { return attrs_.find(name) != attrs_.end(); } const std::type_info& Type() const(const std::string& name) const { PADDLE_ENFORCE(Has(name)); switch (attrs_[name].type()) { case paddle::framework::Attribute::INTS: return typeid(int); ... } } };

wangkuiyi · 2017-06-30T01:19:01Z

doc/design/attribute.md

+class CosineOp : public OperatorBase {
+ public:
+  void InitializeAttribute(const AttributeReader& attrs) {
+    if (attrs.Contains<float>("scale")) {


This doesn't look like a smart idea -- every time we change an attribute in the private: section, we must also change this function accordingly.

wangkuiyi · 2017-06-30T01:20:38Z

doc/design/attribute.md

+};
+```
+
+`InitializeAttribute` will be invoked by `CreateOperator`. Since `InitializeAttribute ` could throw an EnforceNotMet, a `unique_ptr` is used to make code exception-safe.


It seems that this document is not complete. I cannot find the part describing macros that define attributes of an operator?

wangkuiyi · 2017-06-30T01:21:54Z

doc/design/attribute.md

+```
+
+`InitializeAttribute` will be invoked by `CreateOperator`. Since `InitializeAttribute ` could throw an EnforceNotMet, a `unique_ptr` is used to make code exception-safe.
+


Also, I am curious about how the Python API can fill in the protobuf field OperatorDesc::attrs, so that the C++ code could load.

jacquesqiao

we will discuss more about operator attr and registry

reyoung · 2017-08-02T03:04:36Z

Closed because it has been done.

reyoung added 3 commits June 26, 2017 16:55

Init commit for attribute design.

cdd28f7

* Add background.

Adding protobuf implementation

87e3820

Add implementation in CPP

581ce7d

reyoung requested review from wangkuiyi, jacquesqiao, JiayiFeng and QiJune June 26, 2017 09:36

reyoung closed this Jun 26, 2017

reyoung reopened this Jun 27, 2017

jacquesqiao reviewed Jun 27, 2017

View reviewed changes

reyoung added 2 commits June 27, 2017 15:41

Typo, base class should be virtual.

306dcfe

Add comments

0d9b9d3

jacquesqiao added this to Doing in PaddlePaddle Refactoring: Phase 1 Jun 27, 2017

jacquesqiao approved these changes Jun 27, 2017

View reviewed changes

wangkuiyi requested changes Jun 28, 2017

View reviewed changes

QiJune reviewed Jun 28, 2017

View reviewed changes

reyoung force-pushed the feature/attribute_design_for_op branch 2 times, most recently from 0865ca4 to 84843d5 Compare June 28, 2017 10:14

Update Attribute Design

e3a63d7

* Simplify background description. * Not using Error as return value, use PADDLE_ENFORCE

reyoung force-pushed the feature/attribute_design_for_op branch from 84843d5 to e3a63d7 Compare June 28, 2017 10:16

reyoung added 7 commits June 29, 2017 13:15

Merge branch 'develop' of github.com:baidu/Paddle into feature/attrib…

ba54a0c

…ute_design_for_op

Merge branch 'feature/protobuf_lib' into feature/attribute_design_for_op

18dd0ad

Implement Attribute

908c8c1

Merge branch 'feature/protobuf_lib' into feature/attribute_design_for_op

b901e3b

Change AttributeReader's header

7250a92

Fix TravisCI

3090785

Merge branch 'feature/protobuf_lib' into feature/attribute_design_for_op

2bde865

reyoung added 2 commits June 29, 2017 16:18

Add GetArray

b90a3a6

Merge branch 'develop' of github.com:baidu/Paddle into feature/attrib…

d21d486

…ute_design_for_op

jacquesqiao mentioned this pull request Jun 29, 2017

operator design #2555

Closed

reyoung force-pushed the feature/attribute_design_for_op branch from 679bf73 to 9bcd74c Compare June 29, 2017 09:10

jacquesqiao reviewed Jun 29, 2017

View reviewed changes

jacquesqiao mentioned this pull request Jun 29, 2017

basic impl of operator #2667

Closed

jacquesqiao reviewed Jun 29, 2017

View reviewed changes

Simplify API of AttributeReader

911113d

Add copy right and comments Update design doc

reyoung force-pushed the feature/attribute_design_for_op branch from 9bcd74c to 911113d Compare June 29, 2017 10:49

reyoung added 3 commits June 29, 2017 18:56

Rename Contain -> Contains

1f35526

Simplify Attribute Doc

224c6a4

Add comments in code

1895f06

jacquesqiao reviewed Jun 29, 2017

View reviewed changes

reyoung mentioned this pull request Jun 29, 2017

Feature/op desc proto #2675

Closed

jacquesqiao reviewed Jun 30, 2017

View reviewed changes

wangkuiyi approved these changes Jun 30, 2017

View reviewed changes

wangkuiyi requested changes Jun 30, 2017

View reviewed changes

jacquesqiao requested changes Jun 30, 2017

View reviewed changes

reyoung closed this Aug 2, 2017

reyoung removed this from Doing in PaddlePaddle Refactoring: Phase 1 Aug 2, 2017

reyoung deleted the feature/attribute_design_for_op branch October 28, 2017 22:19

heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021

fix quant export_model (PaddlePaddle#2606)

0ed43ce

		@@ -0,0 +1,125 @@
		# Design Doc about operator attribute

		## background

		```

		There are two methods in `AttributeReader`: `Get` and `GetArray`. `GetArray` is used for `ListValue`, and `Get` is used for the rests. The user should invoke either of them when he wants to get an Attribute value from `AttributeMap`.


		There are two frameworks implement `Attribute` concept in `protobuf`. They are [`caffe2`](https://github.com/caffe2/caffe2/blob/master/caffe2/proto/caffe2.proto#L98) and [`tensorflow`](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/framework/attr_value.proto#L16).

		* Caffe2 uses `proto2` syntax. It treats all attributes as a list, and each attribute contains a `name`. Each time caffe2 read an attribute is searching a variable in a list. It is slow if the number of attributes is large. Caffe2 also mark all field as `optional`. It doesn't ensure `one of` attribute value is set.

		```

		`InitializeAttribute` will be invoked by `CreateOperator`. Since `InitializeAttribute ` could throw an EnforceNotMet, a `unique_ptr` is used to make code exception-safe.

Design doc for operator attribute #2606

Design doc for operator attribute #2606

Conversation

reyoung commented Jun 26, 2017 • edited

reyoung commented Jun 27, 2017

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

jacquesqiao left a comment

Choose a reason for hiding this comment

wangkuiyi left a comment • edited

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

reyoung Jun 29, 2017 • edited

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

wangkuiyi Jun 30, 2017 • edited

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

jacquesqiao left a comment

Choose a reason for hiding this comment

reyoung commented Aug 2, 2017

reyoung commented Jun 26, 2017 •

edited

wangkuiyi left a comment •

edited

reyoung Jun 29, 2017 •

edited

wangkuiyi Jun 30, 2017 •

edited