TensorFlow Transformer Part-2 #9

phi-dbq · 2017-09-19T20:56:58Z

Utilities to infer tensor and operation names: as_tensor_name and as_op_name.
They implement the following functionalities.
{tf.Tensor, tf.Operation, tf.Tensor name, tf.Operation name} |-> name of a tf.Operation
{tf.Tensor, tf.Operation, tf.Tensor name, tf.Operation name} |-> name of a tf.Tensor

These two methods differ from existing ones in that if the input is already a string type, they will simply go and infer the name of the graph element (either tensor or operation).
These are useful when working outside the context of a tf.Graph object.
Existing methods such: tensor_name, op_name, get_tensor and get_op all require a tf.Graph object as input.

codecov-io · 2017-09-19T22:07:51Z

Codecov Report

Merging #9 into tf-transformer-part1 will increase coverage by 0.47%.
The diff coverage is 100%.

@@                   Coverage Diff                    @@
##           tf-transformer-part1       #9      +/-   ##
========================================================
+ Coverage                 82.66%   83.13%   +0.47%     
========================================================
  Files                        24       24              
  Lines                      1281     1293      +12     
  Branches                      5        5              
========================================================
+ Hits                       1059     1075      +16     
+ Misses                      222      218       -4

Impacted Files	Coverage Δ
python/sparkdl/graph/builder.py	`93.69% <100%> (ø)`	⬆️
python/sparkdl/graph/tensorframes_udf.py	`90% <100%> (ø)`	⬆️
python/sparkdl/graph/utils.py	`100% <100%> (+4.93%)`	⬆️
python/sparkdl/transformers/keras_image.py	`100% <100%> (ø)`	⬆️
python/sparkdl/transformers/tf_image.py	`95.49% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8531ec...63967b4. Read the comment docs.

sueann

Could you add a description to the PR so I can understand why this change is needed?

sueann · 2017-09-21T21:13:22Z

python/tests/tests.py

@@ -29,6 +29,9 @@
 from pyspark.sql import SQLContext
 from pyspark.sql import SparkSession

+class PythonUnitTestCase(unittest.TestCase):
+    # Just the plain test unittest.TestCase, but won't have to do import check


do you mean the unittest2 check above? if so make that explicit.

sueann · 2017-09-21T21:15:51Z

python/tests/graph/test_utils.py

+        tnsr = tf.constant(1427.08, name=op_name)
+        graph = tnsr.graph
+
+        self.assertEqual(op_name, tfx.as_op_name(tnsr))


what's the difference between as_op_name and op_name ?

op_name takes a graph and a tfobj_or_name argument, and check if the latter is part of the former.
as_op_name returns a valid tensor name if the input is a string. Otherwise, it uses the graph of the tfobj and call op_name.
Thus op_name is a bit more strict than as_op_name.

sueann · 2017-09-21T21:20:53Z

python/tests/graph/test_utils.py

+from ..tests import PythonUnitTestCase
+
+class TFeXtensionGraphUtilsTest(PythonUnitTestCase):
+


each test function should test a particular aspect of a particular function in the tfx module, so it's easy to understand what exactly is being tested, and thus if a test fails it's immediately obvious what exactly failed. could you restructure so that is true? e.g. a few I see embedded in the existing tests below are:

def test_op_name(): # test that op_name from an op name equals input def test_op_name_from_tensor(): # test that op_name from a tensor is correct ...

If there is shared set-up code, you can make helper functions for them.

pending on the result on test refactoring in PR-1

Using the following YAPF style ======================================================== based_on_style = pep8 ALIGN_CLOSING_BRACKET_WITH_VISUAL_INDENT=True BLANK_LINE_BEFORE_NESTED_CLASS_OR_DEF=False COLUMN_LIMIT=100 SPACE_BETWEEN_ENDING_COMMA_AND_CLOSING_BRACKET=False SPLIT_ARGUMENTS_WHEN_COMMA_TERMINATED=True SPLIT_BEFORE_FIRST_ARGUMENT=False SPLIT_BEFORE_NAMED_ASSIGNS=False SPLIT_PENALTY_AFTER_OPENING_BRACKET=30 USE_TABS=False ========================================================

sueann · 2017-09-26T21:21:12Z

Can you say in the PR description which PR this is used in? Trying to understand how it is used (outside of tests).

sueann

i think i understand op_name and as_op_name better now. will we break a lot of things if we changed this function to be def op_name(tfobj_or_name, graph=None) and rolled in as_op_name into it? Same for tensor_name.

then we have only one function, with the assumption that if the graph is given, we'll verify that the tensor / op name is legit.

having all these functions that are almost the same is confusing for the user.

phi-dbq

Consolidated a bunch of (as_|get_)?(tensor|op)(_name)? functions as per the comments.

sueann

looks pretty good. thanks for consolidating the op_name tensor_name functions! mostly small comments below.

sueann · 2017-09-29T23:02:07Z

python/sparkdl/graph/utils.py

    """
    Derive tf.Tensor name from an op/tensor name.
-    We do not check if the tensor exist (as no graph parameter is passed in).
+    If the input is a name, we do not check if the tensor exist


"If the input is a TensorFlow object, or the graph is given, we also check that the tensor exists in the associated graph."

sueann · 2017-09-29T23:02:19Z

python/sparkdl/graph/utils.py

    tnsr = graph.get_tensor_by_name(_tensor_name)
    assert tnsr is not None, \
        'cannot locate tensor {} in current graph'.format(_tensor_name)
    return tnsr

-def as_tensor_name(name):
+def tensor_name(tfobj_or_name, graph=None):
    """
    Derive tf.Tensor name from an op/tensor name.


Derive tf.Tensor name from a op/tensor name or object.

sueann · 2017-09-29T23:03:10Z

python/sparkdl/graph/utils.py

    :param tfobj_or_name: either a tf.Tensor, tf.Operation or a name to either
    """
-    graph = validated_graph(graph)
-    return get_op(graph, tfobj_or_name).name
+    # If `graph` is provided, directly get the graph operation


no need for comment

sueann · 2017-09-29T23:03:34Z

python/sparkdl/graph/utils.py

+    # If `graph` is provided, directly get the graph operation
+    if graph is not None:
+        return get_tensor(tfobj_or_name, graph).name
+    # If `graph` is absent, check if other cases


no need for comment

sueann · 2017-09-29T23:04:19Z

python/sparkdl/graph/utils.py

    """
-    Get the name of a tf.Tensor
+    Derive tf.Operation name from an op/tensor name.


"name or object."

sueann · 2017-09-29T23:16:27Z

python/tests/graph/test_utils.py

+
+class TFeXtensionGraphUtilsTest(PythonUnitTestCase):
+    @parameterized.expand(_gen_graph_elems_names)
+    def test_valid_graph_element_names(self, data, description):


test_valid_tensor_op_string_inputs is more clear.

sueann · 2017-09-29T23:22:49Z

python/tests/graph/test_utils.py

+    yield TestCase(data=(tnsr_name, tfx.tensor_name(tnsr, graph)),
+                   description='get tensor name from tensor (with graph)')
+    yield TestCase(data=(tnsr, tfx.get_tensor(tnsr, graph)),
+                   description='get tensor from the same tensor (with graph)')


the four cases (this and below) require graph as input, so we don't need to add "(with graph)" in the description. having it makes it seem like there should be "(no graph)" versions.

sueann · 2017-09-29T23:24:06Z

python/tests/graph/test_utils.py

+    tnsr = tf.constant(1427.08, name=op_name)
+    graph = tnsr.graph
+
+    yield TestCase(data=(op_name, tfx.op_name(tnsr)),


since there are so many test cases here, let's add comments in between sections, e.g.

# Tests for op_name yield .. ... # Tests for tensor_name ... # Tests for get_op ... # Tests for get_tensor ... # ...

sueann · 2017-09-29T23:28:05Z

python/tests/graph/test_utils.py

+                   description='get tensor from the same tensor (with graph)')
+    yield TestCase(data=(tnsr.op, tfx.get_op(tnsr, graph)),
+                   description='get op from tensor (with graph)')
+    yield TestCase(data=(graph, tfx.get_op(tnsr, graph).graph),


why would we be worried about this not being true? is there some complex logic in get_op we can mess up?

One basic contract for the get_op and get_tensor functions is that the returned tensor or operation should belong to the graph that we pass as the second argument.
Otherwise, there could be subtle errors that are hard to debug. For example, we might feed data to a tensor/placeholder on the wrong graph.

That's a statement about handling bad input. It'd make more sense to test that it raises error if the graph and the op are not compatible, no?

That sounds like a statement about handling bad input. We should also test that these raise errors if the graph and the op are not compatible.

sueann · 2017-09-29T23:29:00Z

python/tests/graph/test_utils.py

+
+    yield TestCase(data=(op_name, tfx.op_name(tnsr)),
+                   description='get op name from tensor (no graph)')
+    yield TestCase(data=(op_name, tfx.op_name(tnsr, graph)),


we should have tests for getting op_name and tesor_name from op objects as well

1. docs in graph/utils.py

phi-dbq · 2017-10-03T01:29:25Z

@sueann I updated the PR regarding the last round of comments.

sueann · 2017-10-04T00:30:57Z

python/tests/graph/test_utils.py

+                   description='get tensor from the same tensor (with graph)')
+    yield TestCase(data=(tnsr.op, tfx.get_op(tnsr, graph)),
+                   description='get op from tensor (with graph)')
+    yield TestCase(data=(graph, tfx.get_op(tnsr, graph).graph),


That's a statement about handling bad input. It'd make more sense to test that it raises error if the graph and the op are not compatible, no?

sueann · 2017-10-04T00:36:06Z

python/tests/graph/test_utils.py

    for wrong_val in [7, 1.2, tf.Graph()]:
        yield TestCase(data=wrong_val, description='wrong type {}'.format(type(wrong_val)))


-def _gen_graph_elems():
+def _gen_valid_tensor_op_objects():


now it seems to be testing all input combos. if so, rename to _gen_valid_tensor_op_input_combos

sueann

just a few comments

sueann · 2017-10-04T00:36:22Z

python/tests/graph/test_utils.py

+                   description='get tensor from the same tensor (with graph)')
+    yield TestCase(data=(tnsr.op, tfx.get_op(tnsr, graph)),
+                   description='get op from tensor (with graph)')
+    yield TestCase(data=(graph, tfx.get_op(tnsr, graph).graph),


That sounds like a statement about handling bad input. We should also test that these raise errors if the graph and the op are not compatible.

sueann

almost there! just a few nits. thanks!

sueann · 2017-10-04T17:38:12Z

python/sparkdl/graph/utils.py

@@ -71,8 +72,7 @@ def get_op(tfobj_or_name, graph):
        raise TypeError('invalid op request for [type {}] {}'.format(type(name), name))
    _op_name = op_name(name, graph=None)
    op = graph.get_operation_by_name(_op_name)
-    assert op is not None, \
-        'cannot locate op {} in current graph'.format(_op_name)
+    assert isinstance(op, tf.Operation), 'expect tf.Operation, but got {}'.format(type(op))


I think the previous error message was more clear. the current one sounds a little bit like you expected the user to give us the operation object, not our internal invocation of get_operation_by_name.

sueann · 2017-10-04T17:41:28Z

python/sparkdl/graph/utils.py

+
+
+def _assert_same_graph(tfobj, graph):
+    if graph is None or not hasattr(tfobj, 'graph'):


simpler (no need for the extra return statement):

if graph is not None and hasattr(tfobj, 'graph'): assert ...

sueann · 2017-10-04T17:43:51Z

python/tests/graph/test_utils.py

+
+    # Test get_tensor and get_op returns tensor or op contained in the same graph
+    yield TestCase(data=lambda: tfx.get_op(tnsr, other_graph),
+                   description='test graph from getting op fron tensor')


'test get_op from tensor with wrong graph' is more clear (similarly for other testcases in this method)

sueann · 2017-10-04T17:46:09Z

python/tests/graph/test_utils.py

+    other_graph = tf.Graph()
+    op_name = tnsr.op.name
+
+    # Test get_tensor and get_op returns tensor or op contained in the same graph


'Test get_tensor and get_op with non-associated tensor/op and graph inputs'

sueann · 2017-10-04T17:49:11Z

python/tests/graph/test_utils.py

+    yield TestCase(data=(tnsr.op, tfx.get_op(op_name, graph)),
+                   description='test op from op name')
+
+    # Test get_tensor and get_op returns tensor or op contained in the same graph


these tests don't seem necessary now that we have the asserts you've most recently added (we invoke the same function calls in other tests and they will fail if the asserts fail). let's remove them to save on testing and reading time. i like this new way of having the asserts in the methods much better than the tests here, since what we're checking here are really implementation details.

phi-dbq · 2017-10-04T20:09:47Z

python/tests/graph/test_utils.py

@@ -128,24 +129,6 @@ def _gen_valid_tensor_op_input_combos():
    yield TestCase(data=(tnsr.op, tfx.get_op(op_name, graph)),
                   description='test op from op name')

-    # Test get_tensor and get_op returns tensor or op contained in the same graph


Removed these tests as they are covered by other test cases.

phi-dbq

Updated regarding latest PR comments.

phi-dbq · 2017-10-04T20:10:30Z

python/tests/graph/test_utils.py

@@ -47,23 +47,24 @@ def _gen_invalid_tensor_or_op_with_graph_pairing():
    other_graph = tf.Graph()
    op_name = tnsr.op.name

-    # Test get_tensor and get_op returns tensor or op contained in the same graph
+    # Test get_tensor and get_op with non-associated tensor/op and graph inputs
+    _comm_suffix = ' with non-associated tensor/op and graph inputs'


Renamed the description to clarify the intention of each test case.

' with wrong graph' seems good enough here and more readable.

just to be clear, my comment was for _comm_suffix. the long way to say it for the comment is good since there are now two ways the same thing is described to the user - so it should decrease uncertainty if one of them doesn't quite make sense to the reader.

phi-dbq · 2017-10-04T20:10:38Z

python/sparkdl/graph/utils.py

@@ -216,7 +218,6 @@ def strip_and_freeze_until(fetches, graph, sess=None, return_graph=False):


 def _assert_same_graph(tfobj, graph):
-    if graph is None or not hasattr(tfobj, 'graph'):


Removed the first return statement.

phi-dbq · 2017-10-04T20:12:12Z

python/sparkdl/graph/utils.py

@@ -72,12 +72,13 @@ def get_op(tfobj_or_name, graph):
        raise TypeError('invalid op request for [type {}] {}'.format(type(name), name))
    _op_name = op_name(name, graph=None)
    op = graph.get_operation_by_name(_op_name)
-    assert isinstance(op, tf.Operation), 'expect tf.Operation, but got {}'.format(type(op))
+    err_msg = 'cannot locate op {} in the current graph, got [type {}] {}'
+    assert isinstance(op, tf.Operation), err_msg.format(_op_name, type(op), op)


Changed error message to better align to the intended usage of the function rather than focusing locally on this assertion statement.

sueann

lgtm. a few nits for you to fix, but will approve (no further review required).

sueann · 2017-10-04T22:02:19Z

python/tests/graph/test_utils.py

@@ -47,23 +47,24 @@ def _gen_invalid_tensor_or_op_with_graph_pairing():
    other_graph = tf.Graph()
    op_name = tnsr.op.name

-    # Test get_tensor and get_op returns tensor or op contained in the same graph
+    # Test get_tensor and get_op with non-associated tensor/op and graph inputs
+    _comm_suffix = ' with non-associated tensor/op and graph inputs'


' with wrong graph' seems good enough here and more readable.

sueann · 2017-10-04T22:02:28Z

python/tests/graph/test_utils.py

    yield TestCase(data=lambda: tfx.get_op(tnsr, other_graph),
-                   description='test graph from getting op fron tensor')
+                   description='test get_op with from tensor' + _comm_suffix)


remove "with"

sueann · 2017-10-04T22:02:34Z

python/tests/graph/test_utils.py

    yield TestCase(data=lambda: tfx.get_op(tnsr.name, other_graph),
-                   description='test graph from getting op fron tensor name')
+                   description='test get_op fron tensor name' + _comm_suffix)


fron -> from

* intro: TFInputGraph * tests * Merge branch 'tf-transformer-part1' into tf-transformer-part3 * and so there is no helper classes * and into more pieces * class & docs * update docs * refactoring tfx API * update tfx utils usage * one way to build these tests * tests refactored * test cases in a single class THis will make things easier when we want to extend other base class functions. * shuffle things around Signed-off-by: Philip Yang <philip.yang@databricks.com> * docs mostly * yapf'd * consolidate tempdir creation * (wip) PR comments * more tests * change test generator module name * TFTransformer Part-3 Test Refactor (#14) * profiling * tests * renamed test * removed original tests * removed the profiler utils * fixes indents * imports * added some tests * added test * fix test * one more test * PR comments * TensorFlow Transformer Part-4 (#11) * flat param API impl * support input graph scenarios * (WIP) new interface implementation * docs and cleanup * using tensorflow API instead of our utilities * automatic type conversion * cleanup * PR comments 1. Move `InputGraph` to its module. * (WIP) address comments * (WIP) respond to PR comments * test refactor * (wip) consolidating params * rebase upstream * import params fix * (wip) TFInputGraph impl * (wip) moving to new API * (wip) enable saved_model tests * (wip) enable checkpoint test * (wip) enable multiple tensor tests * enable all tests * optimize graph for inference * allows setting TFInputGraph * utilize test_input_graph for transformer tests * enable all tests Signed-off-by: Philip Yang <philip.yang@databricks.com> * input graph * docs * tensor tests * tensor test update * TFTransformer Part-4 Test Refactor (#15) * adding new tests * remove original test design * cleanup * deleting original testing ideas * PR comments

phi-dbq added 2 commits September 19, 2017 13:55

update utils

0c8c219

tests

522279a

sueann suggested changes Sep 21, 2017

View reviewed changes

phi-dbq added 2 commits September 21, 2017 14:47

Merge branch 'tf-transformer-part1' into tf-transformer-part2

467480e

phi-dbq force-pushed the tf-transformer-part1 branch from 7287ab7 to 4f11374 Compare September 21, 2017 22:00

phi-dbq added 2 commits September 21, 2017 15:06

Merge branch 'tf-transformer-part1' into tf-transformer-part2

27f0617

Merge branch 'tf-transformer-part1' into tf-transformer-part2

93e659d

sueann suggested changes Sep 28, 2017

View reviewed changes

phi-dbq force-pushed the tf-transformer-part1 branch from 7ae89e4 to 76e9fb9 Compare September 28, 2017 21:51

Merge branch 'tf-transformer-part1' into tf-transformer-part2

f8d7930

phi-dbq force-pushed the tf-transformer-part2 branch from 06280b2 to 202e7ea Compare September 29, 2017 07:19

phi-dbq commented Sep 29, 2017

View reviewed changes

phi-dbq added 2 commits September 29, 2017 09:02

refactoring tfx API

aef1661

test refactoring

ead1ed6

phi-dbq force-pushed the tf-transformer-part2 branch from 202e7ea to ead1ed6 Compare September 29, 2017 16:48

sueann suggested changes Sep 29, 2017

View reviewed changes

phi-dbq added 4 commits October 2, 2017 13:48

PR comments

055ce14

1. docs in graph/utils.py

(wip) utils test

2d48b32

a few more tests for utils

742cdaf

test update cont'd

f0912fb

Merge branch 'tf-transformer-part1' into tf-transformer-part2

3c849f2

sueann reviewed Oct 4, 2017

View reviewed changes

phi-dbq force-pushed the tf-transformer-part2 branch from c53f0a7 to 26c8f24 Compare October 4, 2017 01:45

PR comments

d729528

phi-dbq force-pushed the tf-transformer-part2 branch from 26c8f24 to d729528 Compare October 4, 2017 01:48

sueann suggested changes Oct 4, 2017

View reviewed changes

PR comments

0c2eda1

phi-dbq force-pushed the tf-transformer-part2 branch from 6947e62 to 0c2eda1 Compare October 4, 2017 20:05

phi-dbq commented Oct 4, 2017

View reviewed changes

sueann approved these changes Oct 4, 2017

View reviewed changes

PR comments

63967b4

phi-dbq force-pushed the tf-transformer-part2 branch from e033795 to 63967b4 Compare October 5, 2017 00:05

sueann mentioned this pull request Oct 5, 2017

[New Feature] What would it take to generalize to non-image data? databricks/spark-deep-learning#54

Open

phi-dbq added this to the release 0.2 milestone Oct 12, 2017

phi-dbq mentioned this pull request Nov 18, 2017

[Holistic] TFTransformer databricks/spark-deep-learning#78

Closed

phi-dbq merged commit cf856db into tf-transformer-part1 Nov 22, 2017

		from ..tests import PythonUnitTestCase

		class TFeXtensionGraphUtilsTest(PythonUnitTestCase):



		def _assert_same_graph(tfobj, graph):
		if graph is None or not hasattr(tfobj, 'graph'):

		@@ -216,7 +218,6 @@ def strip_and_freeze_until(fetches, graph, sess=None, return_graph=False):


		def _assert_same_graph(tfobj, graph):
		if graph is None or not hasattr(tfobj, 'graph'):

TensorFlow Transformer Part-2 #9

TensorFlow Transformer Part-2 #9

Uh oh!

Conversation

phi-dbq commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sueann left a comment

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!

sueann commented Sep 26, 2017

Uh oh!

sueann left a comment

Choose a reason for hiding this comment

Uh oh!

phi-dbq left a comment

Choose a reason for hiding this comment

Uh oh!

sueann left a comment

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!

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!

phi-dbq commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sueann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sueann left a comment

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

phi-dbq commented Sep 19, 2017 •

edited

Loading

codecov-io commented Sep 19, 2017 •

edited

Loading