Add option for build more python tests in Cmake #11853

raymondxyang · 2017-07-28T17:18:56Z

Add option tensorflow_BUILD_MORE_PYTHON_TESTS for testing some major packages under contrib directory
Tested, fixed and enabled some UTs on Windows/Linux

* Fix regex match for Windows * Fix compatibility issue with Python 3.x

…o windows-enable-testcases

* Clean code and resolve conflicts

tensorflow-jenkins · 2017-07-28T17:18:58Z

Can one of the admins verify this patch?

rmlarsen · 2017-08-08T18:56:48Z

@mrry could you take a look at this, please?

raymondxyang · 2017-08-08T19:09:58Z

Thanks @rmlarsen @mrry

mrry

Thanks for improving the test coverage! Some comments and questions about the changes follow....

mrry · 2017-08-08T21:27:48Z

tensorflow/contrib/cmake/tf_tests.cmake

@@ -148,14 +148,28 @@ if (tensorflow_BUILD_PYTHON_TESTS)
    "${tensorflow_source_dir}/tensorflow/python/training/*_test.py"
    "${tensorflow_source_dir}/tensorflow/contrib/data/*_test.py"
    "${tensorflow_source_dir}/tensorflow/contrib/factorization/*_test.py"
-    "${tensorflow_source_dir}/tensorflow/contrib/keras/python/keras/integration_test.py"


Was this test failing? It would be better to leave it here if not, so that it continues to be part of our presubmits...

AssertionError on local Windows build (GPU) recorded when committed. Passed latest Jenkins tensorflow/master build (CPU).. So I will add it back since Jenkins only run CPU build

mrry · 2017-08-08T21:30:18Z

tensorflow/contrib/keras/python/keras/utils/generic_utils.py

@@ -195,7 +195,7 @@ def func_dump(func):
  Returns:
      A tuple `(code, defaults, closure)`.
  """
-  code = marshal.dumps(func.__code__).decode('raw_unicode_escape')
+  code = marshal.dumps(func.__code__).replace(b'\\',b'/').decode('raw_unicode_escape')


What's going on here? It doesn't seem generally safe to do this, but I'm not familiar with this code...

mrry · 2017-08-08T21:31:59Z

tensorflow/contrib/learn/python/learn/estimators/estimator_test.py

-                constants.COLLECTION_DEF_KEY_FOR_INPUT_FEATURE_KEYS))
+        self.assertItemsEqual(
+          ['bogus_lookup', 'feature'],
+          [x.decode("utf-8") for x in graph.get_collection(


Does this work on Python 2.x with non-Unicode strings?

It might be better to use the compat.py conversion functions to canonicalize these to bytes (or str) objects....

mrry · 2017-08-08T21:32:45Z

tensorflow/contrib/learn/python/learn/utils/export_test.py

@@ -63,32 +63,33 @@ def _get_default_signature(self, export_meta_filename):
      signatures_any[0].Unpack(signatures)
      default_signature = signatures.default_signature
      return default_signature
-
+  


I think the linter will complain about the added space here and on L85... delete it, please.

mrry · 2017-08-08T21:33:16Z

tensorflow/contrib/learn/python/learn/utils/export_test.py

  def _assert_export(self, export_monitor, export_dir, expected_signature):
    self.assertTrue(gfile.Exists(export_dir))
    # Only the written checkpoints are exported.
    self.assertTrue(
-        saver.checkpoint_exists(export_dir + '00000001/export'),
+        saver.checkpoint_exists(os.path.join( export_dir, '00000001', 'export')),


Delete the space after join( here and on L73, L75, and L77.

mrry · 2017-08-08T21:33:51Z

tensorflow/contrib/learn/python/learn/utils/export_test.py

-    self.assertFalse(saver.checkpoint_exists(export_dir + '00000000/export'))
-    self.assertTrue(saver.checkpoint_exists(export_dir + '00000010/export'))
+    # Catch exception for non-existing path
+    # with self.assertRaises(errors.NotFoundError): 


Delete this commented-out line.

mrry · 2017-08-08T21:34:03Z

tensorflow/contrib/learn/python/learn/utils/export_test.py

    # Validate the signature
-    signature = self._get_default_signature(export_dir + '00000010/export.meta')
+    signature = self._get_default_signature(
+      os.path.join( export_dir, '00000010', 'export.meta'))


Delete the space after join(.

mrry · 2017-08-08T21:35:31Z

tensorflow/contrib/learn/python/learn/utils/gc_test.py

-                     compat.as_str_any(path.path))
+    # Modify the path object for RegEx match for Windows Paths
+    match = re.match("^" + compat.as_str_any(base_dir).replace('\\','/') + "/(\\d+)$",
+                    compat.as_str_any(path.path).replace('\\','/'))


This substitution doesn't seem safe on POSIX... what if there is a \ character in a path? Perhaps you should guard this with an if os.name == 'nt': block.

mrry · 2017-08-08T21:35:55Z

tensorflow/contrib/session_bundle/exporter.py

@@ -301,7 +301,8 @@ def export(self,
    if exports_to_keep:
      # create a simple parser that pulls the export_version from the directory.
      def parser(path):
-        match = re.match("^" + export_dir_base + "/(\\d{8})$", path.path)
+        match = re.match("^" + export_dir_base.replace('\\','/') + "/(\\d{8})$",
+                         path.path.replace('\\','/'))


Same comment as before... wrap this in an if os.name == 'nt': block.

All regex fixed.. thanks for the advice Derek

rmlarsen · 2017-08-09T16:57:45Z

@tensorflow-jenkins test this please

mrry

LGTM. Thanks for the contribution!

raymondxyang · 2017-08-10T00:40:56Z

Hi @mrry
As I check the jennkins i found failure message as below:

//tensorflow/contrib/keras:core_test                                     FAILED in 6.9s
  /var/lib/jenkins/workspace/tensorflow-pull-requests-cpu/bazel-ci_build-cache/.cache/bazel/_bazel_jenkins/eab0d61a99b6696edb3d2aff87b585e8/execroot/workspace/bazel-out/local-opt/testlogs/tensorflow/contrib/keras/core_test/test.log

Executed 1182 out of 1182 tests: 1181 tests pass and 1 fails locally.

Parameterized build ends with FAILURE at: Wed Aug  9 18:35:12 UTC 2017 (Elapsed time: 2116 s)
Build step 'Execute shell' marked build as failure
[Set GitHub commit status (universal)] ERROR on repos [] (sha:19cad03) with context:tensorflow-pull-requests-cpu
Unable to get pull request builder trigger!!
Setting status of 49e83b063f14167577fdd440950ecba22db8a36a to FAILURE with url https://ci.tensorflow.org/job/tensorflow-pull-requests-cpu/6245/ and message: 'FAILURE
 '
Using context: Linux CPU Tests
Finished: FAILURE

Not sure what happened.. and cannot re-produce on local/own jennkins bazel build.. would you help me look into the log or help me getting the detailed info?

rmlarsen · 2017-08-10T00:45:23Z

@tensorflow-jenkins test this please
@raymondxyang the failure you saw was most likely a tool failure unrelated to your PR.

raymondxyang · 2017-08-10T20:57:27Z

Hi @rmlarsen I've added a missing module related to the failure. May I have it re-tested by jenkins?
Thanks again for following up the request J

mrry · 2017-08-10T21:22:31Z

@tensorflow-jenkins test this please.

raymondxyang · 2017-08-11T00:24:48Z

The test tensorflow/python/training/server_lib_test.py started but never finished.. killed for over time limit. touched nothing related to it and it passed in the previous Windows run both locally and on the jenkins.. Can jenkins re-test it? Thanks J

rmlarsen · 2017-08-11T00:27:17Z

@tensorflow-jenkins test this please

rmlarsen · 2017-08-11T03:42:50Z

@tensorflow-jenkins test this please

rmlarsen · 2017-08-11T15:37:16Z

Thanks for the contribution!

raymondxyang added 13 commits July 25, 2017 15:44

Ignore Windows built project

3c17586

Fix deprecated methods in tf.contrib.python

7e62b71

Fix regex match for Windows build in contrib.keras

cef3aa0

Fix Regex match for Windows build in session_bundle

d067bd0

* Fix deprecated methods

8f6b79b

* Fix regex match for Windows * Fix compatibility issue with Python 3.x

Add missing ops into Windows build for test

70e3b84

Enabled more testcases for Windows build

bc4beca

Clean code and fix typo

d272789

Add conditional cmake mode for enabling more unit testcase

30a9bdc

Add Cmake mode for major Contrib packages

b2c7c3f

Add supplementary info in RAEDME for new cmake option

c15cc8e

Merge branch 'master' of https://github.com/tensorflow/tensorflow int…

a476e04

…o windows-enable-testcases

* Update tf_tests after testing with TF 1.3

3ef5a02

* Clean code and resolve conflicts

raymondxyang requested review from benoitsteiner and mrry as code owners July 28, 2017 17:18

googlebot added the cla: yes label Jul 28, 2017

benoitsteiner assigned mrry Aug 1, 2017

Merge branch 'master' into windows-enable-testcases

e87df4f

rmlarsen added the awaiting review Pull request awaiting review label Aug 8, 2017

mrry suggested changes Aug 8, 2017

View reviewed changes

rmlarsen added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Aug 9, 2017

raymondxyang added 2 commits August 9, 2017 09:42

Fix unsafe regex matches and format code

85ea0f7

Update exclude list after testing with latest master branch

49e83b0

mrry approved these changes Aug 9, 2017

View reviewed changes

Merge branch 'master' into windows-enable-testcases

2fbcfa2

Fix missing module

2729867

rmlarsen merged commit 9f81374 into tensorflow:master Aug 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option for build more python tests in Cmake #11853

Add option for build more python tests in Cmake #11853

raymondxyang commented Jul 28, 2017

tensorflow-jenkins commented Jul 28, 2017

rmlarsen commented Aug 8, 2017

raymondxyang commented Aug 8, 2017

mrry left a comment

mrry Aug 8, 2017

raymondxyang Aug 8, 2017

mrry Aug 8, 2017

mrry Aug 8, 2017

raymondxyang Aug 9, 2017

mrry Aug 8, 2017

mrry Aug 8, 2017

mrry Aug 8, 2017

mrry Aug 8, 2017

mrry Aug 8, 2017

mrry Aug 8, 2017

raymondxyang Aug 9, 2017

rmlarsen commented Aug 9, 2017

mrry left a comment

raymondxyang commented Aug 10, 2017

rmlarsen commented Aug 10, 2017

raymondxyang commented Aug 10, 2017

mrry commented Aug 10, 2017

raymondxyang commented Aug 11, 2017

rmlarsen commented Aug 11, 2017

rmlarsen commented Aug 11, 2017

rmlarsen commented Aug 11, 2017

Add option for build more python tests in Cmake #11853

Add option for build more python tests in Cmake #11853

Conversation

raymondxyang commented Jul 28, 2017

tensorflow-jenkins commented Jul 28, 2017

rmlarsen commented Aug 8, 2017

raymondxyang commented Aug 8, 2017

mrry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmlarsen commented Aug 9, 2017

mrry left a comment

Choose a reason for hiding this comment

raymondxyang commented Aug 10, 2017

rmlarsen commented Aug 10, 2017

raymondxyang commented Aug 10, 2017

mrry commented Aug 10, 2017

raymondxyang commented Aug 11, 2017

rmlarsen commented Aug 11, 2017

rmlarsen commented Aug 11, 2017

rmlarsen commented Aug 11, 2017