Sarkars/builder - #176
Sarkars/builder#176sayantan-nervana wants to merge 38 commits into
Conversation
Split conversions into .cc and .h to add header guards So that it can be included in multiple places (builder and builder1)
aprocter
left a comment
aprocter
left a comment
There was a problem hiding this comment.
Good stuff! Just some suggestions.
| // Given the input shapes and a list of TF _Arg nodes, create corresponding | ||
| // nGraph parameters. Also populate the ng_op_map | ||
| Status GetInputParams(const std::vector<TensorShape>&, vector<const Node*>, | ||
| vector<shared_ptr<ng::op::Parameter>>&); |
There was a problem hiding this comment.
I think TF style favors pointers over non-const references (not 100% sure though)
| // Given a TF node, return its corresponding TranslateOp function and required | ||
| // input indexes. A wrapper for TRANSLATE_OP_MAP and INPUT_INDEX_MAP | ||
| Status GetOpTranslationRequirements(const Node*, Builder1::TranslatorFn&, | ||
| vector<int>&); |
There was a problem hiding this comment.
I think TF style favors pointers over non-const references (not 100% sure though)
There was a problem hiding this comment.
https://google.github.io/styleguide/cppguide.html#Reference_Arguments
It seems to prefer a const T&, and never a raw T&. It prefers T* over T& for 'return' values.
But, our code makes a lot of use of T& for returning. For example TranslateGraph itself uses std::shared_ptrngraph::Function& to return the created ng_function.
The 'cons' of not using T& as stated in the style guide is:
References can be confusing, as they have value syntax but pointer semantics.
I am not sure if addressing that con is worth the effort of overhauling all places where we have already used T& for returning values
There was a problem hiding this comment.
You're right---we didn't really fully absorb that this was a Google style preference until we had written a fair chunk of the bridge code already, so a lot of our code is using return-via-reference. I guess it's a question of whether we need to conform to Google style here. I'll leave that question up to @avijit-nervana .
|
I think most of Adam's review comments are addressed or incorporated. The two things remaining are:
Any other thing that I missed in the above summary? |
|
Comments below.
From: Sayantan Sarkar <notifications@github.com>
Reply-To: NervanaSystems/ngraph-tf <reply@reply.github.com>
Date: Monday, September 3, 2018 at 1:05 PM
To: NervanaSystems/ngraph-tf <ngraph-tf@noreply.github.com>
Cc: "Chakraborty, Avijit" <avijit.chakraborty@intel.com>, Mention <mention@noreply.github.com>
Subject: Re: [NervanaSystems/ngraph-tf] Sarkars/builder (#176)
I think most of Adam's review comments are addressed or incorporated. The two things remaining are:
1. T& vs T* for return. TF style prefers T* for return/changeable variables. references are preferred with consts. But in out code, we use TranslateGraph that uses non-const T&.
I prefer T* for return as it’s explicit from the function signature. I used to believe that T& is safer but … but sure what’s the best practice these days.
1. Adding MakeNGraphNode as suggested by Adam in Slack. Should we do this in a new PR?
I forgot which slack message. Could you please tag me in that one? I will review and let me know
(Tomorrow -as I need to drive to LA now)
Any other thing that I missed in the above summary?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#176 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AdGBtSXx2vn4E4PLZn4NPqErQAsbJ7Zzks5uXYtigaJpZM4WSXgC>.
|
|
Ok, will make the T& -> T* changes. Also tagged you in slack |
| int32 max_arg_index = -1; | ||
| std::vector<const Node*> arg_nodes; | ||
|
|
||
| for (auto node : m_tf_graph.nodes()) { |
There was a problem hiding this comment.
We can loop over m_tf_params as we have already classified the nodes into params, retvals and ops. Might not need the arg_nodes vector, can use m_tf_params
There was a problem hiding this comment.
Going by the GetInputParams() function, max_arg_index = m_tf_params.size()-1 .. In that case we dont need this loop. And, If the _Arg indexes are not predictable, i.e. in sequence (0 to n-1) then GetInputParams() would need changes, ng_parameter_list[index] = ng_param; index might be greater than size.
| // vector of generated nGraph nodes. Since we process nodes in toposort | ||
| // order, it is guaranteed when processing a TF node, its parents | ||
| // will already have been processed and safely stowed in ng_op_map | ||
| std::unordered_map<std::string, vector<shared_ptr<ng::Node>>> m_ng_op_map; |
There was a problem hiding this comment.
Google Style Guide suggests adding trailing _ to all private data members of a class. nGraph core follows the style adopted here.
https://google.github.io/styleguide/cppguide.html#Variable_Names
@avijit-nervana which one is recommended? Actually we can do both, by adding _ to all the variables.
There was a problem hiding this comment.
The m_ means data member, and s_ means static members. According to Adam, ngraph follows m_ and s_, so I have that format. Not sure if we stick to (m_, s_) or trailing _
There was a problem hiding this comment.
@sayantan-nervana When you get a chance - please resolve the conflict.
4 participants
Please take a look at the builder refactoring change.
The killswitch is a primeval C style macro in encapsulate_op.cc. Maybe there is a better/preferable method. But I used it instead of if(flag) because, when we enable the newbuilder at a later stage, we can just delete the stuff in the #if-#else en masse and not have to modify any more code.
Currently, the old builder is in use, so nothing should fall apart.
#define BUILDER1 // (un)comment this line to (de)activate old builder
#ifdef BUILDER1
newbuilder
#else
oldbuilder
#endif
I will continue adding more TranslateOps functions in the meanwhile
Since its a big change, I am sure there are style issues, questionable design/syntax choices, outright unoptimised chunks and sneaky leaks. Your feedback is very important and appreciated.