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

Add take_while experimental dataset op #24645

Merged
merged 18 commits into from
Jan 16, 2019
Merged

Add take_while experimental dataset op #24645

merged 18 commits into from
Jan 16, 2019

Conversation

Squadrick
Copy link
Member

Addresses #24105

@Squadrick
Copy link
Member Author

Still need to add test cases, but any changes required in the current commits?

@jsimsa @mrry Most other dataset op that take in a function as a parameter, check for short circuits using ComputeShortCircuitIndices, will I need to do that here as well?

Also, right now take_while is state-less, similar to map. Do I need to also build the scan equivalent for take_while, i.e. take_while with a state?

@jsimsa jsimsa self-requested a review December 31, 2018 20:30
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.

Overall, this looks promising. I left (mostly minor) comments throughout the PR.

As for your questions, 1) implementing the short-circuit path is optional and 2) take_while should be stateless. If a user needs a stateful version, they can build that out of existing transformations (scan + take_while + map).

@ymodak ymodak self-assigned this Jan 1, 2019
@ymodak ymodak added the awaiting review Pull request awaiting review label Jan 1, 2019
@ymodak ymodak added this to Assigned Reviewer in PR Queue via automation Jan 1, 2019
Remove preserve_cardinality
Update license year
Propagate predicate's error to the caller
Inline _transformation_name
@Squadrick
Copy link
Member Author

I've made the required changes. Adding tests in the next couple of hours.

@ymodak ymodak added the size:L CL Change Size: Large label Jan 2, 2019
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Jan 2, 2019
@Squadrick
Copy link
Member Author

@jsimsa I've added tests and made the changes mentioned. Take a look, and let me know if further changes are required.

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 @Squadrick. Left a couple more small comments. I am going to trigger presubmit tests which might require additional fixes.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Jan 2, 2019
@Squadrick
Copy link
Member Author

Squadrick commented Jan 4, 2019

@jsimsa Made the changes and fixed whatever was failing on the tests. I wasn't exactly sure what the api_def should look like for this, so I copied and edited api_def_FilterDataset, so can you take a look and tell me whether it's right or wrong?

Also, what's the difference between base_api and python_api?

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.

a couple more minor comments

`take_while` ShortCircuit tests are parameterized
Fix errors in `BUILD` files using `buildifier`
LoopIteratorPredicate takes `vector<Tensor>&`
@Squadrick
Copy link
Member Author

@jsimsa Made the changes you asked and fixed whatever was causing the CI to fail. I think it should be good to go unless I've overlooked something.

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jan 7, 2019
@ymodak ymodak removed the kokoro:run label Jan 10, 2019
@jsimsa
Copy link
Contributor

jsimsa commented Jan 10, 2019

@Squadrick one of the test failures tensorflow/tools/api/tests:api_compatibility_test is in fact related:

can you run the following:

    $ bazel build tensorflow/tools/api/tests:api_compatibility_test
    $ bazel-bin/tensorflow/tools/api/tests/api_compatibility_test \
          --update_goldens True

and update the PR

@Squadrick
Copy link
Member Author

Squadrick commented Jan 11, 2019

@jsimsa I pulled master and ran the commands. This is what my git status looks like now.

On branch take-while
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   tensorflow/tools/api/golden/v2/tensorflow.data.experimental.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-baseline-classifier.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-baseline-estimator.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-baseline-regressor.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-boosted-trees-classifier.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-boosted-trees-regressor.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-checkpoint-saver-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-checkpoint-saver-listener.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-d-n-n-classifier.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-d-n-n-estimator.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-d-n-n-linear-combined-classifier.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-d-n-n-linear-combined-estimator.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-d-n-n-linear-combined-regressor.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-d-n-n-regressor.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-estimator.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-feed-fn-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-final-ops-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-global-step-waiter-hook.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-linear-classifier.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-linear-estimator.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-linear-regressor.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-logging-tensor-hook.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.-mode-keys.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-nan-loss-during-training-error.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-nan-tensor-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-profiler-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-second-or-step-timer.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-session-run-args.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-session-run-context.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-session-run-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-session-run-values.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-step-counter-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-stop-at-step-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.-summary-saver-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.experimental.-in-memory-evaluator-hook.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.experimental.-linear-s-d-c-a.pbtxt
	deleted:    tensorflow/tools/api/golden/v2/tensorflow.estimator.experimental.pbtxt
	modified:   tensorflow/tools/api/golden/v2/tensorflow.estimator.pbtxt

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	tensorflow/tools/api/golden/v2/tensorflow.estimator.inputs.pbtxt

no changes added to commit (use "git add" and/or "git commit -a")

I also got this warning:

WARNING:tensorflow:Golden file update requested!
All test failures have been skipped, see the logs for detected diffs.
This test is now going to write new golden files.
Make sure to package the updates together with your change.

You will need an explicit API approval. This may take longer than a normal
review.

And this seems to be the diff:

+   member_method {
+     name: "take_while"
+     argspec: "args=[\'predicate\'], varargs=None, keywords=None, defaults=None"
+   }

I have no idea how to proceed. How do I go about getting API approval?

@jsimsa
Copy link
Contributor

jsimsa commented Jan 11, 2019

The only file pbtxt file you should add to your PR is tensorflow/tools/api/golden/v2/tensorflow.data.experimental.pbtxt. Ignore all the tensorflow/tools/api/golden/v2/tensorflow.estimator.* (I am not sure why they showed up).

I will take care of the API approval (it happens during the internal review of the PR).

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jan 11, 2019
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jan 11, 2019
@ymodak ymodak added the ready to pull PR ready for merge process label Jan 12, 2019
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jan 14, 2019
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.

The internal review surfaced a couple of issues.

@Squadrick
Copy link
Member Author

@jsimsa Running the update script didn't change any of the v1 scripts.

In tensorflow/api_template.__init__.py, on line 56, there's _compat.enable_v2_behavior(), this sets _force_enable in tensorflow/python/tf2.py to True, and every time tf2.enabled() is called it returns True. So setting TF2_BEHAVIOR=0 has no effect on which TensorFlow version to run. tensorflow/api_template.__init__.py is one of the srcs for api_compatibility_test, so I had to manually remove @test_util.run_v1_only('b/120545219') in api_compatibility_tests.py on line 317 and 336, to get v1 golden API files to update.

Is there something I'm overlooking?

@Squadrick
Copy link
Member Author

@jsimsa Pushed the requested changes.

@jsimsa
Copy link
Contributor

jsimsa commented Jan 16, 2019

@Squadrick thanks, I will resume the internal review.

@tensorflow-copybara tensorflow-copybara merged commit ef15adf into tensorflow:master Jan 16, 2019
PR Queue automation moved this from Reviewer Requested Changes to Merged Jan 16, 2019
tensorflow-copybara pushed a commit that referenced this pull request Jan 16, 2019
@Squadrick Squadrick deleted the take-while branch January 16, 2019 22:53
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:L CL Change Size: Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

7 participants