refine sync_with_cpp for insert_op #9747
Conversation
| if len(self.ops) != len(ops_in_cpp) and start_index == 0 and len( | ||
| self.ops) == end_index: | ||
| self.ops.clear() | ||
| for index in range(len(ops_in_cpp)): |
There was a problem hiding this comment.
For simplicity, can we only keep this and remove all above sync operations? It's slower but very easy to understand.
There was a problem hiding this comment.
Sorry, I couldn't remove all above sync operators. I tried it, but many unit tests fail. The reason I guess is:
# sync ops append to the head of cpp_ops
for index in range((start_index - 1 - 1), -1, -1):
op_desc = ops_in_cpp[index]
op = Operator(self, op_desc)
self.ops.appendleft(op)
The reverse order of range((start_index - 1 - 1), -1, -1) and appendleft make the order of python_ops different from cpp_ops.
There was a problem hiding this comment.
That's wired. The above code still tries to make the order of self.ops the same with cpp_ops. We can merge this if it's currently a blocking feature and add some comments here.
|
I have also add insert_op in https://github.com/PaddlePaddle/Paddle/pull/9714/files#diff-d8a7116f8835661cd9a974472e9e9f2dR834, after change block.ops from deque to list https://github.com/PaddlePaddle/Paddle/pull/9714/files#diff-d8a7116f8835661cd9a974472e9e9f2dR662, the problem is quite simple. The main problem is that ops in Python should have the same behavior with it in CPP, deque make things complex. |
|
@jacquesqiao Could you please create a PR and I can test it? |
|
@luotao1 can you please update this PR to include latest changes? |
|
@typhoonzero Yes, I will follow the way like @jacquesqiao 's |
|
close due to #9816 |
related #9629.
When use
insert_opmethod for stage 1, there is error insync_with_cpp. The reason is thatsync_with_cpponly considerremove_op,append_op,prepend_op,remove_varnow, but don't considerinsert_op.Thus, this PR consider
insert_opinsync_with_cpp, and add a unit test to check thesync_with_cppafter callingappend_op,prepend_opandinsert_op.