Let OpProto support multiple and temporary #2860
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
* Each input/output of Paddle's Op could be a list. Add multiple mark to OpProto. Also add a `input_format`/`output_format` attribute if that Op has multiple input or output. The format of that attribute please reference the comments in `op_proto.proto` * Add temporary mark, because some output of an Op is not used by user but used by other op for faster computation. Explicitly mark which output is temporary could let future memory/computation optimization. * Add generated field to AttrProto.
reyoung force-pushed the feature/op_proto_modified branch from 1e8cc1e to 81b3437 Compare 
Superjomn reviewed Jul 14, 2017
| The all input variables of this op is six, and they are segmented into three | ||
| inputs. | ||
| The first input is input[0:4], second is input[4:5], third is input[5:6]. |
There was a problem hiding this comment.
just 3 variable length inputs? why 3, no information about the op's input definition.
There was a problem hiding this comment.
That information is stored in OpProto because it is shared between all instances of the same types.
This PR does not touch C++ OperatorBase interface, but it is better for OperatorBase can get Input/Output information for that type.
Superjomn reviewed Jul 14, 2017
paddle/framework/op_registry_test.cc Outdated
| : OpProtoAndCheckerMaker(proto, op_checker) { | ||
| AddInput("input", "input of cosine op"); | ||
| AddOutput("output", "output of cosine op"); | ||
| AddInput("input", "input of cosine op", /*multiple*/ true); |
There was a problem hiding this comment.
offer a AddInputs to repalce AddInput("input", true) ?
two methods to add inputs:
AddInputto add a normal inputAddInputsto add a variable length input
Superjomn reviewed Jul 14, 2017
| private: | ||
| void SetHasMultiple(const std::string& in_out, bool* flag) { | ||
| if (!*flag) { | ||
| AddAttr<std::vector<int>>(in_out + "_format", |
There was a problem hiding this comment.
each variable length input will has a input format ?
There was a problem hiding this comment.
The input_format is an attribute for that Op.
* It is more readable to invoke `AddInputs` not `AddInput(multiple=true)`.
jacquesqiao reviewed Jul 14, 2017
| // } | ||
| // } | ||
| // | ||
| optional bool multiple = 3 [default=false]; |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
OpProto. Also add a
input_format/output_formatattribute if thatOp has multiple input or output. The format of that attribute please
reference the comments in
op_proto.protobut used by other op for faster computation. Explicitly mark which
output is temporary could let future memory/computation optimization.