Python 3.7 fixes #20766

MatthiasWinkelmann · 2018-07-13T04:51:30Z

Reserved word: `async`

"async" became a reserved word in Python 3.7. I replaced with "is_async", which was already used for such flags in other places. An alternative suggestion, if the is_ construct is deemed too ugly, might be to use the full term, "asynchronous".

Python C API changes

Python C API now returns const types for `PyUnicode_AsUTF8AndSize``` and similar. Fixed analogous to protocolbuffers/protobuf@0a59054.

Fixes #20690

"async" became a reserved word. Replaced with "is_+async". Python C API now returns const for PyUnicode_AsUTF8AndSize and similar

asimshankar

Thanks for the PR. Some questions/comments:

asimshankar · 2018-07-13T16:00:32Z

tensorflow/c/eager/c_api.cc

@@ -111,7 +111,7 @@ tensorflow::Status GetAllRemoteDevices(
 tensorflow::Status CreateRemoteContexts(
    const std::vector<string>& remote_workers, int64 rendezvous_id,
    const tensorflow::ServerDef& server_def,
-    tensorflow::eager::EagerClientCache* remote_eager_workers, bool async,
+    tensorflow::eager::EagerClientCache* remote_eager_workers, bool is_async,


If async is a reserved word in Python 3.7, I don't quite follow why all these C files have to change :)
(c_api.cc, c_api.h, context.cc, context.h etc.)

Let's revert the changes to the C files?

c_api.cc and c_api.h have to change because pywrap_tensorflow_internal.py is generated from them, however I found that change should be rather minimal and touch only TFE_ContextOptionsSetAsync() and TFE_ContextSetAsyncForThread()

Shouldn't this be handled in pywrap_tfe.i instead of changing the C code?

You are right, this may be handled in pywrap_tfe.i but from my small investigation I could solve this issue only with some sort of hack - define macro to redefine variable name, i.e. put

#define async is_async

into pywrap_tfe.i. %rename function didn't help because it works only with declarations but not with arguments.

I managed to fix the syntax error by changing line 114 and 115, but in addition I needed to also change line 150 and 151 in the same way. That also contains the reserved word async.

Would writing a wrapper function in the SWIG file work?
(Not for the same reason, but we hide TF_SessionRun from Python and instead use TF_SessionRun_wrapper in

tensorflow/tensorflow/python/client/tf_session.i

Line 482 in b8a9d16

// We use TF_SessionRun_wrapper instead of TF_SessionRun

)

I thought of wrapping and I think it should work and it is more direct way than defining macro. But it will require watching for changes in tensorflow c api - watching for updated signatures of wrapped functions and for appearing async parameter in functions.

Isn't it just 2 functions that need to be wrapped?
The concerns around watching for updated signatures or changing signatures or new functions apply even if one were to change the C code, no?

You are right, there are only 2 functions and one should watch for new signatures in case of updated C code anyway.

asimshankar · 2018-07-13T16:02:32Z

tensorflow/python/eager/pywrap_tfe_src.cc

@@ -216,7 +216,7 @@ bool ParseStringValue(const string& key, PyObject* py_value, TF_Status* status,
 #if PY_MAJOR_VERSION >= 3
  if (PyUnicode_Check(py_value)) {
    Py_ssize_t size = 0;
-    char* buf = PyUnicode_AsUTF8AndSize(py_value, &size);
+    char* buf = const_cast<char*>(PyUnicode_AsUTF8AndSize(py_value, &size));


I'd suggest we avoid the const_cast and instead make buf a const char*:

const char* buf = PyUnicode_AsUTF8AndSize(py_value, &size);

asimshankar · 2018-07-13T16:04:48Z

tensorflow/python/lib/core/ndarray_tensor.cc

@@ -154,7 +154,7 @@ Status PyBytesArrayMap(PyArrayObject* array, F f) {
    if (PyUnicode_Check(item.get())) {
 #if PY_VERSION_HEX >= 0x03030000
      // Accept unicode by converting to UTF-8 bytes.
-      ptr = PyUnicode_AsUTF8AndSize(item.get(), &len);
+      ptr = const_cast<char*>(PyUnicode_AsUTF8AndSize(item.get(), &len));


Same comment here. Might have to change the f provided at the call sites to use a const char* instead, but seems like that should be easy.

asimshankar · 2018-07-13T16:05:19Z

tensorflow/python/lib/core/py_func.cc

@@ -352,7 +352,7 @@ Status ConvertNdarrayToTensor(PyObject* obj, Tensor* ret) {
        Py_ssize_t el_size;
        if (PyBytes_AsStringAndSize(input_data[i], &el, &el_size) == -1) {
 #if PY_MAJOR_VERSION >= 3
-          el = PyUnicode_AsUTF8AndSize(input_data[i], &el_size);
+          el = const_cast<char*>(PyUnicode_AsUTF8AndSize(input_data[i], &el_size));


Avoid const_cast here too?

alextp · 2018-07-13T18:44:36Z

tensorflow/contrib/distribute/python/mirrored_strategy.py

@@ -193,7 +193,7 @@ def distribute_dataset(self, dataset_fn):
        self._prefetch_on_device)

  def _broadcast(self, tensor, destinations):
-    # TODO(josh11b): In eager mode, use one thread per device, or async mode.


This change is also unnecessary as it's in a comment

alextp · 2018-07-13T18:45:39Z

This change only changes C++ files and one python comment to not use a python keyword. So nothing in here is necessary, so I'm closing this PR.

asimshankar · 2018-07-13T18:55:34Z

@alextp - We'd still want to address the change in the return type of PyUnicode_AsUTF8AndSize in 3.7. So reopening the PR.

@MatthiasWinkelmann : Can you reduce the PR to handle just that change?

TheAifam5 · 2018-07-16T09:41:46Z

Any update ? :D

Edit:
If someone does not want to wait...

Patch _pywrap_tensorflow_internal.pyd (its PE file) strings in Lib\site-packages\tensorflow\python using https://hexed.it/
python36 -> python37 (single match)
64_90 -> 64_92 (multiple matches)

Patch build_info.py strings in Lib\site-packages\tensorflow\python\platform
64_90 -> 64_92 (single match)
9.0 -> 9.2 (single match)

Patch pywrap_tensorflow_internal.py in Lib\site-packages\tensorflow\python:
async) -> is_async) (should be only 4 matches)

Yea i know, it's dirty way but works :D

asimshankar · 2018-07-31T06:53:59Z

Closing due to inactivity, and since #21202 seems like it subsumes this.

…char*" instead of a "char*". (See https://docs.python.org/3/whatsnew/3.7.html#c-api-changes) There are additional changes needed for Python 3.7 compatibility, this change just pulls out one of them (and subsumes a related attempt in #21202 and #20766) Helps with #20517 PiperOrigin-RevId: 207008013

Python 3.7 fixes

7000c3e

"async" became a reserved word. Replaced with "is_+async". Python C API now returns const for PyUnicode_AsUTF8AndSize and similar

googlebot added the cla: yes label Jul 13, 2018

tensorflowbutler assigned qlzh727 Jul 13, 2018

qlzh727 requested review from akshaym, alextp and asimshankar and removed request for akshaym July 13, 2018 14:46

qlzh727 added the awaiting review Pull request awaiting review label Jul 13, 2018

asimshankar suggested changes Jul 13, 2018

View reviewed changes

alextp reviewed Jul 13, 2018

View reviewed changes

alextp closed this Jul 13, 2018

asimshankar reopened this Jul 13, 2018

tensorflowbutler removed the awaiting review Pull request awaiting review label Jul 14, 2018

alextp added the kokoro:force-run Tests on submitted change label Jul 24, 2018

kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 24, 2018

bstriner mentioned this pull request Jul 28, 2018

Quick Fix for Python 3.7 #21202

Merged

asimshankar closed this Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python 3.7 fixes #20766

Python 3.7 fixes #20766

Python 3.7 fixes #20766

Python 3.7 fixes #20766

Conversation

Reserved word: async

Python C API changes

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

Choose a reason for hiding this comment

Reserved word: `async`