-
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
add remove_op, remove_var in Python end #9816
Conversation
@@ -13,11 +13,10 @@ See the License for the specific language governing permissions and | |||
limitations under the License. */ | |||
|
|||
#include "paddle/fluid/framework/block_desc.h" | |||
#include <queue> |
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.
This line should be above those "" includes.
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.
This is the result of clang-format. When I move line16 above line 15, the clang-format will change to this version.
The reason is that block_desc.h
is Related header, but <queue>
is C++ library. The order is: Related header, C library, C++ library, other libraries' .h, your project's .h. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
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.
I see, thank you.
program.sync_with_cpp() | ||
|
||
# remove op1, its input var2 and output var3 will be removed at the same time, | ||
# but its input var1 and output var4 will not be removed since they are used for op2. | ||
block.remove_op(1, 2) | ||
program.sync_with_cpp() |
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.
Does sync_with_cpp
need to be updated to a simpler version that clear all ops and add back the descs?
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.
Discussed with @jacquesqiao, this simpler version is so dangerous since it will clear all ops.
If we create an op1 first, then call insert_op
, and then using op1 will fail. The reason is that all the instances in Python end are removed, they are changed to C++ instances instead.
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.
That make sense, I see sync_with_cpp
have already deleted those clear
operations.
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
remove_op
,remove_var
in Python end.remove_op
only remove the operator, andremove_var
only remove the variable.