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.

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

[Grappler] Add dataset.shard(1, 0) to noop_elimination pass #47499

Merged
merged 1 commit into from Apr 6, 2021

Conversation

zhuzilin
Copy link
Contributor

@zhuzilin zhuzilin commented Mar 2, 2021

Thank you for your time on reviewing this PR. There are some addition to the test as ShardDataset is not a unary dataset op.

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Mar 2, 2021
@google-cla google-cla bot added the cla: yes label Mar 2, 2021
@rthadur rthadur requested a review from jsimsa March 3, 2021 04:38
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Mar 3, 2021
@@ -43,6 +43,23 @@ NodeDef *MakeUnaryNode(StringPiece node_type, int count, string input_node,
GetCommonAttributes(), graph);
}

NodeDef *MakeShardNode(int count, string input_node, MutableGraphView *graph) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of introducing MakeShardNode, generalize MakeUnaryNode to take a list of integers (as opposed to a single integer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -51,6 +68,23 @@ NodeDef *MakeUnaryNonConstNode(StringPiece node_type, string input_node,
GetCommonAttributes(), graph);
}

NodeDef *MakeShardNonConstNode(string input_node, MutableGraphView *graph) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of introducing MakeShardNonConstNode, generalize MakeNonConstNode to take a list of DTYPES to use to create placeholders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

@jsimsa jsimsa left a comment

Choose a reason for hiding this comment

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

Thanks. Left a couple of minor comments.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Mar 3, 2021
@gbaned
Copy link
Contributor

gbaned commented Mar 19, 2021

@zhuzilin Can you please check @jsimsa's comments and keep us posted ? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Mar 19, 2021
@zhuzilin
Copy link
Contributor Author

zhuzilin commented Apr 2, 2021

@jsimsa @gbaned Really sorry for the late update... was trapped in some other things lately... I've updated the test according to the review.

@zhuzilin zhuzilin requested a review from jsimsa April 2, 2021 03:08
@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Apr 4, 2021
@gbaned gbaned added the awaiting review Pull request awaiting review label Apr 5, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 5, 2021
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Apr 5, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 5, 2021
@gbaned gbaned removed the awaiting review Pull request awaiting review label Apr 6, 2021
@copybara-service copybara-service bot merged commit 0beed3f into tensorflow:master Apr 6, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants