improve py_func by boeddeker · Pull Request #15121 · tensorflow/tensorflow

boeddeker · 2017-12-05T10:08:33Z

See #14448
Improved py_func to accept nested structures as input and as output like in tf.data.Dataset.form_generator

relable inp to args and Tout to output_types
add arguments kwargs and output_shapes
allow args/kwargs/output_types/output_shapes to be a nested structure

Open questions:

allow output_types to be callable? (Dynamic output_types inference from args/kwargs)
(new) argument names
backward compatibility for old names

- relable inp to args and Tout to output_types - add arguments kwargs and output_shapes - allow args/kwargs/output_types/output_shapes to be a nested structure

tensorflow-jenkins · 2017-12-05T10:08:34Z

Can one of the admins verify this patch?

caisq · 2017-12-07T19:59:04Z

ping @alextp

alextp · 2017-12-07T22:25:01Z

+
+
+  """
+  from tensorflow.python.framework import tensor_shape


Imports should go at the top of the file

I will move it to the top in the next commit.

alextp · 2017-12-07T22:26:46Z



-def py_func(func, inp, Tout, stateful=True, name=None):
+def py_func(func, args=(), kwargs={}, output_types=None, output_shapes=None,


This change from imp to args, kwargs can break existing code, breaking our API compatibility guidelines we've had since tf 1.0.

So we need to find a way of doing what you want while not changing the signature of py_func.

When my suggestion does not move to contrib or 3-party and it is necessary to keep the API, my suggestion is (I do not know the rules for deprecated arguments in tf, I saw only some examples):
def py_func(func, inp, Tout=None, stateful=True, name=None, kwargs={}, Sout=None)

What I want with the changed signature is to allow keyword arguments and automatic set_shape like in tf.data.Dataset.from_generator.

Since in python it is common to use func(*args, **kwargs) I used that names. output_types and shape came from tf.data.Dataset.from_generator.

alextp · 2017-12-07T22:29:03Z

+      # If callable, assume same signature and call with tensors and get the shapes
+      output_shapes = output_shapes(*args, **kwargs)
+
+  flat_output_types = nest.flatten(output_types)


So my understanding is that the main change here is you call nest.flatten and nest.pack_sequence_as around the user code in a py_func. Everything else is debugging / diagnostics, right?

If so, I think this can live in utility code instead of in the tensorflow library proper. Maybe you want to add a contrib.py_func with this API or add this py func wrapper in a third-party library somewhere?

You are right. The main changed are allow kwargs as input and arbitrary returns with shapes (earlier only list/scalar without shape).
Another (not so important) point is to allow shape/type inference from the input.(Useful for decorators)
If you prefer contrib.py_func I can move the code to contrib.
If you prefer a third-party library, I will keep the code in a private repo.

When you need a use case inside of tf for this modification, look in tf.data.Dataset.from_generator.
The code there was the starting point.

I agree the dataset API for py_func is friendlier to use and I like exposing it. We could consider either tf.py_func_v2, or contrib.

@martinwicke for API review as to whether we prefer _v2 or contrib

martinwicke · 2017-12-08T00:17:07Z

I'd say contrib to start, so we can fix things if we find something wrong with the initial version.

boeddeker · 2017-12-08T08:40:13Z

Where should the code be? I do not know how contrib is organized.

alextp · 2017-12-10T21:20:49Z

contrib.framework seems to be the least-inappropriate existing package. Just move this definition to a file there and it should work I think

boeddeker · 2017-12-11T10:59:13Z

ok, I moved now the code to tensorflow/conrtib/framework/ops/session_ops.py

alextp

You also need to import the symbol from contrib/framework/init.py or it won't show up in the public package

boeddeker · 2017-12-11T16:09:27Z

sorry that I forgot the import, I fixed that.
There are maybe some more such bugs because I have not figured out how to install tensorflow from binary with git in editable mode.

alextp · 2017-12-11T16:13:25Z

Jenkins, test this please.

alextp · 2017-12-11T16:15:35Z

Let's see if the tests catch those.

…

On Mon, Dec 11, 2017 at 4:14 PM, Christoph Boeddeker < ***@***.***> wrote: sorry that I forgot the import, I fixed that. There are maybe some more such bugs because I have not figured out how to install tensorflow from binary with git in editable mode. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15121 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAATxdQHql0ysoJKd93tNISiQwOd918Kks5s_VTcgaJpZM4Q2CO2> .

-- - Alex

yifeif · 2017-12-19T22:45:26Z

@tensorflow-jenkins test this please

yifeif · 2017-12-21T00:06:01Z

@boeddeker do you mind fix the lint errors and test failures?

boeddeker · 2017-12-21T09:42:30Z

@yifeif Where do I find the lint errors and test failures?
Since I did not write a test, how could a test fail, that is related to this PR?
I only found one lint error.

alextp · 2017-12-21T15:24:19Z

Incidentally you should write a test, then.

…

On Thu, Dec 21, 2017 at 1:46 AM, Christoph Boeddeker < ***@***.***> wrote: @yifeif <https://github.com/yifeif> Where do I find the lint errors and test failures? Since I did not write a test, how could a test fail, that is related to this PR? I only found one lint error. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15121 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAATxbxk3jbO5J7-pdn8ClBY_MIth8MWks5tCijxgaJpZM4Q2CO2> .

-- - Alex

drpngx · 2017-12-25T23:40:52Z

+from tensorflow.python.framework import tensor_shape
+from tensorflow.python.util import nest
+
+from tensorflow.python.ops.script_ops import py_func as _py_func


Please use the hidden ops for that.

What do you mean with hidden ops? I used a renaming because of the name collision.

@drpngx this is fine -- it's a contrib-only extension of py_func, py_func still exists as a public symbol, so we don't need hidden_ops.

drpngx · 2017-12-26T19:20:02Z

This appears to make a lot of tests fail. Could you check?

boeddeker · 2017-12-27T10:59:05Z

I am not familiar with the import machinery in tensorflow, can someone explain, why the import does not work?
from tensorflow.contrib.framework.python.ops.script_ops import *

I will start to write tests, when the source code is formally correct for tensorflow.

martinwicke · 2017-12-27T17:57:00Z

The @@py_func callout needs to go into the docstring of tensorflow/contrib/framework/__init__.py, since it is there that remove_undocumented is called.

Otherwise I think this should work.

drpngx · 2017-12-27T22:48:09Z

@martinwicke Right, contrib, good point.

drpngx · 2017-12-28T00:03:08Z

@boeddeker you're probably missing some dependency.

FAIL: //tensorflow/python/kernel_tests:rnn_test (shard 10 of 10) (see /tmpfs/tmp/bazel/execroot/org_tensorflow/bazel-out/k8-py3-opt/testlogs/tensorflow/python/kernel_tests/rnn_test/shard_10_of_10/test.log)
INFO: From Testing //tensorflow/python/kernel_tests:rnn_test (shard 10 of 10):
==================== Test output for //tensorflow/examples/image_retraining:retrain_test:
Traceback (most recent call last):
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/examples/image_retraining/retrain_test.py", line 24, in <module>
    from tensorflow.examples.image_retraining import retrain
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/examples/image_retraining/retrain.py", line 117, in <module>
    from tensorflow.contrib.quantize.python import quant_ops
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/contrib/__init__.py", line 22, in <module>
    from tensorflow.contrib import bayesflow
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/contrib/bayesflow/__init__.py", line 24, in <module>
    from tensorflow.contrib.bayesflow.python.ops import csiszar_divergence
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/contrib/bayesflow/python/ops/csiszar_divergence.py", line 26, in <module>
    from tensorflow.contrib.bayesflow.python.ops.csiszar_divergence_impl import *
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/contrib/bayesflow/python/ops/csiszar_divergence_impl.py", line 43, in <module>
    from tensorflow.contrib import framework as contrib_framework
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/contrib/framework/__init__.py", line 93, in <module>
    from tensorflow.contrib.framework.python.ops import *
  File "/tmp/botexec/bazel-out/k8-py3-opt/bin/tensorflow/examples/image_retraining/retrain_test.runfiles/org_tensorflow/tensorflow/contrib/framework/python/ops/__init__.py", line 27, in <module>
    from tensorflow.contrib.framework.python.ops.script_ops import *
ModuleNotFoundError: No module named 'tensorflow.contrib.framework.python.ops.script_ops'
================================================================================```

yifeif · 2018-01-08T19:19:03Z

@tensorflow-jenkins test this please

boeddeker · 2018-01-09T09:16:29Z

Do I have to add the new file to tf_custom_op_py_library inside the BUILD file? (I did this in my last commit).

drpngx · 2018-01-09T18:22:35Z

Jenkins, test this please.

boeddeker · 2018-01-11T10:18:36Z

Sorry, but I have no bazel and did not know that I have to add dependencies.

yifeif · 2018-01-12T00:43:20Z

@tensorflow-jenkins test this please

improve py_func

c92cbe1

- relable inp to args and Tout to output_types - add arguments kwargs and output_shapes - allow args/kwargs/output_types/output_shapes to be a nested structure

googlebot added the cla: yes label Dec 5, 2017

caisq requested a review from alextp December 5, 2017 14:34

caisq self-assigned this Dec 5, 2017

caisq added the awaiting review Pull request awaiting review label Dec 5, 2017

alextp requested a review from akshayka December 7, 2017 22:24

alextp suggested changes Dec 7, 2017

View reviewed changes

move new py_func to tf.contrib.framework

b7d121c

alextp suggested changes Dec 11, 2017

View reviewed changes

add import for contrib.py_func

e41603a

alextp approved these changes Dec 11, 2017

View reviewed changes

alextp added awaiting testing (then merge) and removed awaiting review Pull request awaiting review labels Dec 11, 2017

boeddeker mentioned this pull request Dec 19, 2017

from_generator feedback and questions #13101

Closed

yifeif added the kokoro:force-run Tests on submitted change label Dec 19, 2017

kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 19, 2017

yifeif added the stat:awaiting response Status - Awaiting response from author label Dec 21, 2017

fix indent

fbc9e88

yifeif added kokoro:force-run Tests on submitted change and removed stat:awaiting response Status - Awaiting response from author labels Dec 21, 2017

kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 21, 2017

drpngx reviewed Dec 25, 2017

View reviewed changes

drpngx added the stat:awaiting response Status - Awaiting response from author label Dec 26, 2017

add missing @@

3ea90fc

yifeif added kokoro:force-run Tests on submitted change and removed stat:awaiting response Status - Awaiting response from author labels Jan 8, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 8, 2018

add new file to BUILD

a912bce

drpngx added the kokoro:force-run Tests on submitted change label Jan 9, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 9, 2018

add missing dep

a7656ce

correct dep order

93cc00c

yifeif added the kokoro:force-run Tests on submitted change label Jan 12, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 12, 2018

caisq merged commit ac8e673 into tensorflow:master Jan 13, 2018

jvishnuvardhan mentioned this pull request Jun 12, 2019

[Feature request] Improve syntax for accessing Python objects in dataset pipelines #14448

Closed



		def py_func(func, inp, Tout, stateful=True, name=None):
		def py_func(func, args=(), kwargs={}, output_types=None, output_shapes=None,

Conversation

boeddeker commented Dec 5, 2017

Uh oh!

tensorflow-jenkins commented Dec 5, 2017

Uh oh!

caisq commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinwicke commented Dec 8, 2017

Uh oh!

boeddeker commented Dec 8, 2017

Uh oh!

alextp commented Dec 10, 2017

Uh oh!

boeddeker commented Dec 11, 2017

Uh oh!

alextp left a comment

Choose a reason for hiding this comment

Uh oh!

boeddeker commented Dec 11, 2017

Uh oh!

alextp commented Dec 11, 2017

Uh oh!

alextp commented Dec 11, 2017 via email

Uh oh!

yifeif commented Dec 19, 2017

Uh oh!

yifeif commented Dec 21, 2017

Uh oh!

boeddeker commented Dec 21, 2017

Uh oh!

alextp commented Dec 21, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drpngx commented Dec 26, 2017

Uh oh!

boeddeker commented Dec 27, 2017

Uh oh!

martinwicke commented Dec 27, 2017

Uh oh!

drpngx commented Dec 27, 2017

Uh oh!

drpngx commented Dec 28, 2017

Uh oh!

yifeif commented Jan 8, 2018

Uh oh!

boeddeker commented Jan 9, 2018

Uh oh!

drpngx commented Jan 9, 2018

Uh oh!

boeddeker commented Jan 11, 2018

Uh oh!

yifeif commented Jan 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants