Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Dy2stat] Fix Memory Optimization in run_program_op and Add SimNet as Unit Test #25383

Merged
merged 7 commits into from
Jul 13, 2020
Merged

[Dy2stat] Fix Memory Optimization in run_program_op and Add SimNet as Unit Test #25383

merged 7 commits into from
Jul 13, 2020

Conversation

zhhsplendid
Copy link
Member

@zhhsplendid zhhsplendid commented Jul 6, 2020 •
edited
Loading

PR types

Others

PR changes

Others

Describe

Add Similarity Net as unit test. During the unit test, we found three problems:

  1. The run_program_op has memory optimization error when running dy2stat net multiple times.
  2. The support for SelectedRows can cause problem in dy2stat.
  3. The return grammar has problem.

This PR fixes the 1. problem but modify codes for the 2. 3. problems to make PR smaller. I will fix those two problems in the next PR(s)

@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 6, 2020

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 6, 2020

Thanks for your contribution!
Please add test = develop in your commit message to trigger CI to ensure your PR can be merged.
See Paddle CI Manual for details.

@paddle-bot-old
Copy link

paddle-bot-old bot commented Jul 6, 2020

Thanks for your contribution!
Please add test = develop in your commit message to trigger CI to ensure your PR can be merged.
See Paddle CI Manual for details.

@zhhsplendid zhhsplendid changed the title Code backup commit, test=develop Fix Memory Optimization in run_program_op and Add SimNet as Unit Test Jul 9, 2020
@zhhsplendid zhhsplendid changed the title Fix Memory Optimization in run_program_op and Add SimNet as Unit Test [Dy2stat] Fix Memory Optimization in run_program_op and Add SimNet as Unit Test Jul 9, 2020
liym27
liym27 previously approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@liym27 liym27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

chenwhql
chenwhql previously approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Aurelius84
Aurelius84 previously approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@Aurelius84 Aurelius84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the rest

paddle/fluid/operators/run_program_op.h
for (const framework::OpDesc *op : all_ops) {
if (!boost::algorithm::ends_with(op->Type(), "_grad")) {
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question: here we use _grad to determine whether a op is grad_op, but some op's grad_op is not with _grad suffix, such arrary_write's grad_op is array_read.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe also can use op_role?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for great suggestion. Done

@zhhsplendid zhhsplendid dismissed stale reviews from Aurelius84, chenwhql, and liym27 via 4797135 July 10, 2020 05:51
@zhhsplendid zhhsplendid merged commit f9ac5fb into PaddlePaddle:develop Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liym27 liym27 liym27 approved these changes

@chenwhql chenwhql chenwhql approved these changes

@Aurelius84 Aurelius84 Aurelius84 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zhhsplendid @Aurelius84 @chenwhql @liym27