[Dy2stat] Add Basic Support for Grammar 'return' #25176
Merged
Conversation
This file contains 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
|
Thanks for your contribution! |
liym27
reviewed
Jun 28, 2020
| return self.count_return[func_node] | ||
|
|
||
| def set_func_return_count(self, func_node, count): | ||
| self.count_return[func_node] = count |
There was a problem hiding this comment.
This function set_func_return_count doesn't seem to be called.
There was a problem hiding this comment.
Yes, I found it. I will remove in my next PR which supports various-length return. Indeed it is already deleted in my working station.
zhhsplendid
added a commit
that referenced
this pull request
Jun 30, 2020
Support Various-Length Return Grammar in Dy2stat. This PR is a follow-up of #25176 . The basic idea is putting no-value placeholder variables at `return` statement to make all `return` statement have same length, after that the static graph can have fixed fetch output (code at return_transformer.py). Then remove those no-value placeholder when we finally return dygraph result (code at partial_program.py). However, various length return in Bert model is still not supported. The dy2stat can change the code as I wish but some ops which check shape at compile time (e.g. Reshape, MatMul) will throw error because of the no-value-placeholder may not have the required shape. Is this a matter? To me, those no-value placeholder will be replaced as really values meeting shape requirements at run time, so I think the solution should be some way to do the compile-time checking. By the way, every time when we have dynamic shape, it often causes problem in dy2stat. We should find a way to handle it in the future. Fixing various return in Bert is my TODO thing and I will also find some other existing models for verification.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR types
New features
PR changes
Others
Describe
This PR added basic support for 'return' grammar in dy2stat. It supports the control flow of 'return'.
The basics idea is using a return value variable to store the early return statements and boolean state variables with if-else to skip the statements after the return statements.
This PR is very basic support. There are some corner cases I didn't develop/test. For example, 'return None', 'return different length of variables', 'return non-tensor and tensor together', 'no return statement'. These corner cases will be done in my next PRs. Target date is this week.
Note:
for the unit test, I changed test_program_translator.py because the StaticCode of
dyfunc_with_if_elsewill change. To guarantee the correctness ofdyfunc_with_if_else, I also run it inTestRecursiveReturnin test_return.py.I commented the early return code in bert_dygraph_model.py because 'return different length of variables' is unsupported now. I also know that there are some other models used early return and we didn't enable it in the unit test. I will add support for it in next PRs and then re-enable those tests.