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

[NODEREF] Introduce named attribute system. #1618

Merged
merged 2 commits into from Aug 20, 2018
Merged

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented Aug 18, 2018

So far we introduced Map<string, NodeRef> for general attribute storage. As we plan to upgrade the high-level IR, we would like to introduce more structure into it, specifically

  • We would like to have something like dmlc::Parameter that declares the default value and attribute value property
  • We also want to take benefit of the TVM's node system(directly access from python side).

This PR introduces attrs.h to solve the problem. The declaration API is relatively similar to dmlc::Parameter, and the Attrs is backed by the Node system. The general idea is that we declare the attribute in the C++, and it can be made accessible from the python side. We also introduce a DictAttrsNode that is backed by a dictionary, to mimic the python dict based object

Features

  • Allow automatically set of default value when initialization value is not provided
  • Provide bound checking and doc generation as in dmlc::Parameter
  • Node system: field directly accessible from python side, serializable

C++ Declaration

struct TestAttrs : public AttrsNode<TestAttrs> {
  int axis;
  std::string name;
  Array<Expr> padding;
   TVM_DECLARE_ATTRS(TestAttrs, "attrs.TestAttrs") {
    TVM_ATTR_FIELD(axis)
        .set_default(10)
        .set_lower_bound(1)
        .set_upper_bound(10)
        .describe("axis field");
    TVM_ATTR_FIELD(name)
        .describe("name");
    TVM_ATTR_FIELD(padding)
        .describe("padding of input")
        .set_default(Array<Expr>({0, 0}));
  }
};

Python Side Access

def test_make_attrs():
     x = tvm.make.node("attrs.TestAttrs", name="xx", padding=(3,4))
    assert x.name == "xx"
    assert x.padding[0].value == 3
    assert x.padding[1].value == 4
    assert x.axis == 10
     
    dattr = tvm.make.node("DictAttrs", x=1, y=10, name="xyz", padding=(0,0))
    assert dattr.x.value == 1

@tqchen
Copy link
Member Author

tqchen commented Aug 18, 2018

@jroesch @yzhliu @merrymercy please review

Copy link
Member

@jroesch jroesch left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, but we sat together and talked design and implementation, maybe good to get review from others?

* \file tvm/attrs.h
* \brief TVM attribute module
*
* This module enables declaration named attributes
Copy link
Member

Choose a reason for hiding this comment

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

This module enables declaration named attributes

should be:

This module enables declaration of named attributes

* \brief TVM attribute module
*
* This module enables declaration named attributes
* and support default value setup and bound checking.
Copy link
Member

Choose a reason for hiding this comment

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

and support default value setup and bound checking.

should be:

which support default value setup and bound checking.

/*!
* \brief Declare an attribute function.
* \param ClassName The name of the class.
* \param TypeKey The type key to be used in the tvm node system.
Copy link
Member

Choose a reason for hiding this comment

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

The type key to be used by the TVM node system.

__fvisit__(#FieldName, &FieldName)


/*! \brief Error throwed during attribute checking */
Copy link
Member

Choose a reason for hiding this comment

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

Error thrown during attribute checking.

/*! \brief name of the field */
std::string name;
/*! \brief type of the field in string format */
std::string type_info;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we store the type in string format? is this the string key?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used in docstring, clarified in the comment

TVM_DECLARE_NODE_TYPE_INFO(DictAttrsNode, BaseAttrsNode);
};

// Namespace fof detail implementationsx
Copy link
Member

Choose a reason for hiding this comment

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

The english here is unclear.

Namespace containing detail implementations?

}
return *this;
}
// set upper bound checks the upper bound
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could we make these full sentences ?

template<typename FFind>
class AttrInitVisitor {
public:
// hit count on visiting
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain this more in depth?

Returns
-------
node : Node
The corresponding created DSL Node
Copy link
Member

Choose a reason for hiding this comment

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

This is grammatically strange, maybe The corresponding DLS node.?

@@ -223,6 +223,10 @@ class ExtTypeVTable {
class TVMPODValue_ {
public:
operator double() const {
// Allow automatic conversion from int to float
if (type_code_ == kDLInt) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this to make the attributes more flexible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, for example, user can do add_one(x, 1), and in the backend we can directly fetch out a float, for functions that want to do so

@tqchen
Copy link
Member Author

tqchen commented Aug 20, 2018

@jroesch review comments address, please take another look

@jroesch
Copy link
Member

jroesch commented Aug 20, 2018

👍

@tqchen tqchen merged commit 20c495e into apache:master Aug 20, 2018
@tqchen
Copy link
Member Author

tqchen commented Aug 20, 2018

Thanks @jroesch this is now merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants