Rnn make stepnet member #3469
Rnn make stepnet member #3469
Conversation
… rnn-make-stepnet-member
|
fix: #3457 |
paddle/operators/recurrent_op.h
Outdated
| static const rnn::ArgumentName kArgName; | ||
|
|
||
| private: | ||
| RecurrentAlgorithm alg_; | ||
| std::shared_ptr<NetOp> stepnet_; |
There was a problem hiding this comment.
Why would we need to define stepnet_ as a std::shared_ptr? Can we define
class RecurrentOp ... {
private:
NetOp step_;There was a problem hiding this comment.
In pybind.cc, a std::shared_ptr is passed to RecurrentOp as an argument, so here is a shared pointer too.
There was a problem hiding this comment.
Why do we need shared_ptr in our Python binding?
There was a problem hiding this comment.
just like NetOp, RNNOp has its own python binding to expose some methods, such as set_stepnet.
In pybind.cc, all the operators created by user are handled by shared_ptr, maybe easier to be referenced across different levels, when user create a Operator, a shared_ptr is returned, it is free to use the pointer, or pass the pointer into multiple NetOp (add an op into a net).
Operators will live across the whole execution period, so a shared_ptr seems reasonable.
There was a problem hiding this comment.
Do you mean something like here:
py::class_<operators::NetOp, std::shared_ptr<operators::NetOp>> net(m, "Net");It seem that calling Python new Net() returns a shared_ptr<operator::NetOp> -- however, when will the reference count become 0 and the C++ instance get freed?
paddle/operators/recurrent_op.h
Outdated
| @@ -33,7 +34,12 @@ class RecurrentAlgorithm { | |||
| void Run(const framework::Scope& scope, | |||
| const platform::DeviceContext& dev_ctx) const; | |||
|
|
|||
| void Init(std::unique_ptr<rnn::Argument> arg) { arg_ = std::move(arg); } | |||
| void Init(std::unique_ptr<rnn::Argument> arg, | |||
There was a problem hiding this comment.
Why unique_ptr here? Could we just do
class RecurrentOp ... {
private:
rnn::Argument arg_;There was a problem hiding this comment.
the rnn::Argument is a argument that should be passed to RecurrentAlgorithm, a unique pointer may be lighter to pass, and std::move is used here, finally only RecurrentAlgorithm owns this memory.
There was a problem hiding this comment.
I don't get it. By defining arg_ a data member of RecurrentOp, we can of course call RecurrentAlgorithm(&arg_);, could we?
… rnn-make-stepnet-member
fix: #3457