Skip to content

[Grappler] Improve HoistCWiseUnaryChainsStage to support Reshape #42574

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
Merged
copybara-service merged 10 commits into from
May 12, 2021
Merged

[Grappler] Improve HoistCWiseUnaryChainsStage to support Reshape #42574

copybara-service merged 10 commits into from
May 12, 2021
+630 −57

Conversation

@xinan-jiang
Copy link
Contributor

@xinan-jiang xinan-jiang commented Aug 22, 2020

It continues the issue #40542

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Aug 22, 2020
@xinan-jiang
Copy link
Contributor Author

xinan-jiang commented Aug 22, 2020

@rthadur

@xinan-jiang xinan-jiang changed the title Pr/hoist concatenation [Grappler] Improve HoistCWiseUnaryChainsStage to support Reshape Aug 22, 2020
@gbaned gbaned self-assigned this Aug 23, 2020
@gbaned gbaned added the comp:grappler Grappler related issues label Aug 23, 2020
@gbaned gbaned requested a review from ezhulenev August 23, 2020 03:32
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 24, 2020
@rthadur rthadur removed the ready to pull PR ready for merge process label Aug 24, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 24, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 24, 2020
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Oct 6, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 6, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 6, 2020
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Nov 3, 2020
rmlarsen
rmlarsen reviewed Nov 5, 2020
View reviewed changes
tensorflow/core/grappler/optimizers/arithmetic_optimizer.cc Outdated

Status AddReshapes(const PortNodeMap& reshapes, NodeDef* root_node) {
for (auto link : reshapes) {
AddReshape(link.second, root_node, link.first);
Copy link
Member

@rmlarsen rmlarsen Nov 5, 2020

Choose a reason for hiding this comment

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

Check return status here. you can use the TF_RETURN_IF_ERROR macro.

Copy link
Contributor Author

@xinan-jiang xinan-jiang Nov 10, 2020

Choose a reason for hiding this comment

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

done

@gbaned gbaned removed the ready to pull PR ready for merge process label Nov 5, 2020
@gbaned
Copy link
Contributor

gbaned commented Nov 6, 2020

@xinan-jiang Can you please check @rmlarsen's comments and keep us posted ? Thanks!

@gbaned
Copy link
Contributor

gbaned commented Apr 15, 2021

@xinan-jiang Can you please check @rmlarsen's comment and resolve conflicts? Thanks!

@xinan-jiang
Copy link
Contributor Author

xinan-jiang commented Apr 16, 2021

@gbaned I am working on this.

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Apr 24, 2021
@xinan-jiang
Copy link
Contributor Author

xinan-jiang commented Apr 24, 2021

@rmlarsen @gbaned It fails with tf1.x "Concat" op, which is instead of "ConcatV2" in tf2.x. Now it could pass with "Concat" op.

@gbaned
Copy link
Contributor

gbaned commented Apr 26, 2021

@xinan-jiang Can you please resolve conflicts? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Apr 26, 2021
@xinan-jiang
Copy link
Contributor Author

xinan-jiang commented Apr 27, 2021

@rmlarsen please help to review my resolve for confilct with e98fc97

@xinan-jiang xinan-jiang force-pushed the pr/hoist-concatenation branch from f5def59 to 00383fb Compare April 27, 2021 08:14
@gbaned gbaned requested a review from rmlarsen April 27, 2021 14:21
@gbaned gbaned removed the stat:awaiting response Status - Awaiting response from author label Apr 27, 2021
@xinan-jiang
Copy link
Contributor Author

xinan-jiang commented Apr 29, 2021

@rmlarsen @gbaned help to review.

@gbaned gbaned requested review from rmlarsen and rthadur and removed request for rmlarsen April 29, 2021 14:50
@rthadur rthadur requested a review from ezhulenev April 29, 2021 16:17
@gbaned gbaned added the awaiting review Pull request awaiting review label Apr 30, 2021
@xinan-jiang
Copy link
Contributor Author

xinan-jiang commented May 6, 2021

@ezhulenev @rthadur help to review it.

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 6, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 6, 2021
@gbaned gbaned removed the awaiting review Pull request awaiting review label May 7, 2021
@xinan-jiang
Copy link
Contributor Author

xinan-jiang commented May 11, 2021

@gbaned I could not see the details of the fail test. Does it block the merge work?

@gbaned
Copy link
Contributor

gbaned commented May 12, 2021

@gbaned I could not see the details of the fail test. Does it block the merge work?

@xinan-jiang This is waiting for internal approval, once done it will be processed to merge. Thanks!

@copybara-service copybara-service bot merged commit 5b72bd0 into tensorflow:master May 12, 2021
@rmlarsen
Copy link
Member

rmlarsen commented May 20, 2021

I'm afraid this had to be rolled back since it broke tests in the lingvo project. It still seems like the rewrite violates some graph invariants.

	 [[node fprop/test_dtmodel/tower_0_0/Reshape_15 (defined at /third_party/py/lingvo/core/layers.py:2501) ]]```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rmlarsen rmlarsen rmlarsen approved these changes

@gbaned gbaned gbaned approved these changes

@rthadur rthadur Awaiting requested review from rthadur

@ezhulenev ezhulenev Awaiting requested review from ezhulenev

Assignees

@gbaned gbaned

Labels

cla: yes comp:grappler Grappler related issues ready to pull PR ready for merge process size:L CL Change Size: Large

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@xinan-jiang @gbaned @rmlarsen @ezhulenev @rthadur @googlebot @kokoro-team @tensorflowbutler