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

Tests for BatchDatasetOp #27876

Merged
merged 3 commits into from May 3, 2019
Merged

Tests for BatchDatasetOp #27876

merged 3 commits into from May 3, 2019

Conversation

feihugis
Copy link
Member

This PR enables AsGraphDefInternal to work with two versions of BatchDatasetOp and adds tests for BatchDatasetOp.

cc: @jsimsa

jsimsa
jsimsa requested changes Apr 16, 2019
View reviewed changes
tensorflow/core/kernels/data/batch_dataset_op_test.cc Outdated
DatasetOpsTestBase::CreateTensor<int64>(TensorShape({}), {2}),
/*drop_remainder*/
DatasetOpsTestBase::CreateTensor<bool>(TensorShape({}), {false}),
/*op_version*/ 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

you do not have to write for op_version 1 as it is not used anywhere

tensorflow/core/kernels/data/batch_dataset_op.cc Outdated
@@ -114,8 +114,14 @@ class BatchDatasetOp : public UnaryDatasetOpKernel {
TF_RETURN_IF_ERROR(b->AddScalar(batch_size_, &batch_size));
Node* drop_remainder = nullptr;
TF_RETURN_IF_ERROR(b->AddScalar(drop_remainder_, &drop_remainder));
TF_RETURN_IF_ERROR(b->AddDataset(
this, {input_graph_node, batch_size, drop_remainder}, output));
if (type_string() == "BatchDataset") {
Copy link
Contributor

Choose a reason for hiding this comment

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

as per my comment in the test file, please undo this change

@feihugis feihugis force-pushed the Test_BatchDataset branch 2 times, most recently from b22ebf9 to bed7023 Compare April 16, 2019 03:27
@feihugis
Copy link
Member Author

Got it. I rewrite this PR:

  • dac2bf1: Minor updates of the comments in dataset.h file

  • bed7023: add the tests for BatchDatasetOp (remove the parts for op_version 1)

jsimsa
jsimsa previously approved these changes Apr 16, 2019
View reviewed changes
@gbaned gbaned added the size:L CL Change Size: Large label Apr 16, 2019
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Apr 16, 2019
@gbaned gbaned added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 17, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 17, 2019
@feihugis
Copy link
Member Author

feihugis commented Apr 17, 2019 •
edited

The failure log for the test Ubuntu CC can't be accessed. Could you please help paste the log here?

@feihugis feihugis changed the title Enable AsGraphDefInternal to work with two versions of BatchDatasetOp and add tests for BatchDatasetOp Tests for BatchDatasetOp Apr 19, 2019
@feihugis
Copy link
Member Author

The Ubuntu CC test runs successfully on my laptop. @gbaned Could you please help re-trigger the test?

@gbaned gbaned added the kokoro:force-run Tests on submitted change label Apr 23, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Apr 23, 2019
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Apr 23, 2019
@feihugis
Copy link
Member Author

Thanks @gbaned for re-triggering the tests.

Recently, some changes (0c23c60#diff-79a93a3e76fded5f3522ad99b27976fd) were added to BatchDatasetV2, which causes the test failures. Will revise this PR accordingly.

@gbaned
Copy link
Contributor

gbaned commented Apr 25, 2019

@feihugis Any update please? Thanks!

@feihugis
Copy link
Member Author

feihugis commented Apr 25, 2019 •
edited

@gbaned Sorry for the delay as a conference these two days. Will fix the issue today.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Apr 25, 2019
@feihugis
Copy link
Member Author

Recently, some changes (0c23c60#diff-79a93a3e76fded5f3522ad99b27976fd) were added to BatchDatasetV2, which causes the test failures. Will revise this PR accordingly.

@jsimsa @gbaned The new attr parallel_copy is added to BatchDatasetOpTest via 6e7399a. Could you please have a look at the change when you get a chance?

@feihugis
Copy link
Member Author

feihugis commented May 1, 2019

Recently, some changes (0c23c60#diff-79a93a3e76fded5f3522ad99b27976fd) were added to BatchDatasetV2, which causes the test failures. Will revise this PR accordingly.

@jsimsa BatchDatasetV2 got a new attribute (parallel_copy), so I revise this PR accordingly by adding parallel_copy to BatchDatasetOpTest via 6e7399a. Could you please review the change when you have time?

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer May 1, 2019
@tensorflow-bot tensorflow-bot bot added the kokoro:force-run Tests on submitted change label May 1, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 1, 2019
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels May 3, 2019
pull bot pushed a commit to testkevinbonz/tensorflow that referenced this pull request May 3, 2019
@tensorflower-gardener
Merge pull request tensorflow#27876 from feihugis:Test_BatchDataset
2c8a182 
PiperOrigin-RevId: 246524973
@tensorflow-copybara tensorflow-copybara merged commit 6e7399a into tensorflow:master May 3, 2019
PR Queue automation moved this from Approved by Reviewer to Merged May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsimsa jsimsa jsimsa approved these changes

Assignees

@gbaned gbaned

Labels
cla: yes ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@feihugis @gbaned @jsimsa @googlebot @kokoro-team @tensorflow-copybara @rthadur