-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
assign_value operator #7371
assign_value operator #7371
Conversation
paddle/operators/assign_value_op.cc
Outdated
} | ||
|
||
protected: | ||
framework::OpKernelType GetActualKernelType( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name should be GetExpectedKernelType
.
Related doc: https://github.com/PaddlePaddle/Paddle/blob/develop/doc/design/kernel_hint_design.md#final-choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
The interface changed before I submitted the PR.
framework::proto::DataType::FP32}); | ||
AddAttr<std::vector<float>>("fp32_values", "store the float values") | ||
.SetDefault({}); | ||
AddAttr<std::vector<int>>("int32_values", "store the int values") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Op's attribute will be stored into the protobuf. However, the size of a protobuf object is limited(64M). I'm afraid there might be some problems if the tensor to be assigned is huge.
An alternative way is writing the tensor values to a file in Python, then the assign_value_op
read the file and fill them into output variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If the arry is too big, we may have to use a separate file. But if many cases where only a small matrix is used, it's quite inconvenient to use a separate file to array the matrix.
ed42430
to
9108785
Compare
paddle/operators/assign_value_op.h
Outdated
break; | ||
} | ||
auto values = ctx.Attr<std::vector<T>>(value_name); | ||
Copy(dst, values.data(), sizeof(T) * values.size(), ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a uniform Copy interface of https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/tensor_util.h#L101
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
We need this operator to assign value to a tensor and the values are stored in the program so that they can be used independent of python.
9108785
to
25ecd20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We need this operator to assign value to a tensor and the values are stored in the program so that they can be used independent of python.