Add block.fwd_block_id#8489
Conversation
|
|
||
| int32_t Parent() const { return desc_->parent_idx(); } | ||
|
|
||
| int32_t ForwardBlockID() const { return desc_->forward_block_idx(); } |
There was a problem hiding this comment.
looks like this function is unused
There was a problem hiding this comment.
It is used in pybind to expose the forward block id to Python side.
paddle/fluid/framework/block_desc.cc
Outdated
| } | ||
| return it->second.get(); | ||
|
|
||
| BlockDesc *tmp = ParentBlock(); |
There was a problem hiding this comment.
In issue #8367, the search is described as BFS. But the code here is DFS. Please confirm.
python/paddle/v2/fluid/framework.py
Outdated
| if fwd_block is not None: | ||
| return fwd_block.var_recursive(name) | ||
| else: | ||
| raise |
There was a problem hiding this comment.
I am not familiar with the python syntax. What is being raised here?
There was a problem hiding this comment.
This statement will rethrow the exception.
python/paddle/v2/fluid/framework.py
Outdated
| try: | ||
| parent_block = self.program.block(self.parent_idx) | ||
| return parent_block.var_recursive(name) | ||
| except ValueError: |
There was a problem hiding this comment.
I feel the logic is not very straightforward. Probably change to
if self.forward_block_idx == -1:
raise
else
self.program.block(self.forward_block_idx).var_recursive(name)
python/paddle/v2/fluid/framework.py
Outdated
| else: | ||
| parent_block = self.program.block(self.parent_idx) | ||
| return parent_block.var_recursive(name) | ||
| # DFS |
There was a problem hiding this comment.
In issue #8367, the search is described as BFS. But the code here is DFS. Please confirm.
There was a problem hiding this comment.
Yes. I think the BFS should be the correct logic, however, DFS can be implemented easily. I will change the logic to BFS in following commits.
| # If the op has its own sub-block, deal with the sub-block first | ||
| if op.has_attr("sub_block"): | ||
| sub_block = program.block(op.block_attr("sub_block")) | ||
| grad_sub_block = program.create_block(parent_idx=sub_block.idx) |
There was a problem hiding this comment.
Was this a bug?
grad_sub_block = program.create_block(parent_idx=sub_block.idx)
=>
grad_sub_block = program.create_block(parent_idx=current_block.idx)
There was a problem hiding this comment.
We use parent to specify the forward block of the grad block before.
Since we separate the forward block and parent block, here we change the parent of grad block to its actual parent.
paddle/fluid/operators/while_op.cc
Outdated
| if (block_ins.find(input_name) != block_ins.end() || | ||
| fwd_block->FindVar(input_name) != nullptr) { | ||
| fwd_block->FindVar(input_name) != nullptr || | ||
| parent_block->FindVar(input_name) != nullptr) { |
There was a problem hiding this comment.
Why would while_op handle this separately? All the logic has been implemented in FindVar, so we only need grad_bock->FindVar(input_name) != nullptr?
There was a problem hiding this comment.
This logic is to calculate for each operator, which inputs are the OG that calculated in outside and pass into while_grad_op by its inputs. The grad_block always contains all variables that operators needed.
| } | ||
|
|
||
| BlockDesc *BlockDesc::ParentBlock() const { | ||
| if (this->desc_->parent_idx() == kNoneBlockIndex) { |
There was a problem hiding this comment.
This logic has been implemented in MutableBlock. Excellent.
… feature/add_fwd_block_id
…into feature/add_fwd_block_id
reyoung
force-pushed
the
feature/add_fwd_block_id
branch
from
7218c87 to
574bcda
Compare
|
|
||
| prog = self.program | ||
|
|
||
| while len(frontier) != 0: # BFS |
There was a problem hiding this comment.
Can we reuse BlockDesc::FindVarRecursive here?
There was a problem hiding this comment.
We need to find Variable in python, not in C++, so we cannot reuse that code.
4 participants
Fix #8367