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

Python 3.7 fixes #20766

Closed
wants to merge 1 commit into
base: master
from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+82 −83
Diff settings

Always

Just for now

Copy path View file
@@ -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,

This comment has been minimized.

@asimshankar

asimshankar Jul 13, 2018

Contributor

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?

This comment has been minimized.

@daa

daa Jul 19, 2018

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()

This comment has been minimized.

@asimshankar

asimshankar Jul 20, 2018

Contributor

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

This comment has been minimized.

@daa

daa Jul 20, 2018

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.

This comment has been minimized.

@ErikDeBruijn

ErikDeBruijn Jul 21, 2018

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.

This comment has been minimized.

@asimshankar

asimshankar Jul 24, 2018

Contributor

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

// We use TF_SessionRun_wrapper instead of TF_SessionRun
)

This comment has been minimized.

@daa

daa Jul 24, 2018

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.

This comment has been minimized.

@asimshankar

asimshankar Jul 24, 2018

Contributor

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?

This comment has been minimized.

@daa

daa Jul 24, 2018

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

tensorflow::gtl::FlatMap<string, tensorflow::uint64>* remote_contexts) {
for (int i = 0; i < remote_workers.size(); i++) {
const string& remote_worker = remote_workers[i];
@@ -128,7 +128,7 @@ tensorflow::Status CreateRemoteContexts(
*request.mutable_server_def() = server_def;
request.mutable_server_def()->set_job_name(parsed_name.job);
request.mutable_server_def()->set_task_index(parsed_name.task);
request.set_async(async);
request.set_async(is_async);
auto* eager_client = remote_eager_workers->GetClient(remote_worker);
if (eager_client == nullptr) {
return tensorflow::errors::Internal(
@@ -203,7 +203,7 @@ tensorflow::Status NewRemoteAwareTFE_Context(const TFE_ContextOptions* opts,
tensorflow::gtl::FlatMap<string, tensorflow::uint64> remote_contexts;
LOG_AND_RETURN_IF_ERROR(CreateRemoteContexts(
remote_workers, rendezvous_id, opts->server_def,
remote_eager_workers.get(), opts->async, &remote_contexts));
remote_eager_workers.get(), opts->is_async, &remote_contexts));

tensorflow::RemoteRendezvous* r =
grpc_server->worker_env()->rendezvous_mgr->Find(rendezvous_id);
@@ -222,7 +222,7 @@ tensorflow::Status NewRemoteAwareTFE_Context(const TFE_ContextOptions* opts,

auto* device_mgr = grpc_server->worker_env()->device_mgr;
*ctx = new TFE_Context(opts->session_options.options, opts->policy,
opts->async, device_mgr, r, std::move(server),
opts->is_async, device_mgr, r, std::move(server),
std::move(remote_eager_workers),
std::move(remote_device_mgr), remote_contexts);

@@ -241,8 +241,8 @@ void TFE_ContextOptionsSetConfig(TFE_ContextOptions* options, const void* proto,
}

void TFE_ContextOptionsSetAsync(TFE_ContextOptions* options,
unsigned char async) {
options->async = async;
unsigned char is_async) {
options->is_async = is_async;
}
void TFE_ContextOptionsSetDevicePlacementPolicy(
TFE_ContextOptions* options, TFE_ContextDevicePlacementPolicy policy) {
@@ -259,9 +259,9 @@ TF_CAPI_EXPORT extern void TFE_ContextOptionsSetServerDef(
}

TF_CAPI_EXPORT extern void TFE_ContextSetAsyncForThread(TFE_Context* ctx,
unsigned char async,
unsigned char is_async,
TF_Status* status) {
status->status = ctx->context.SetAsyncForThread(async);
status->status = ctx->context.SetAsyncForThread(is_async);
}

void TFE_DeleteContextOptions(TFE_ContextOptions* options) { delete options; }
@@ -285,7 +285,7 @@ TFE_Context* TFE_NewContext(const TFE_ContextOptions* opts, TF_Status* status) {
new tensorflow::IntraProcessRendezvous(device_mgr.get());

return new TFE_Context(opts->session_options.options, opts->policy,
opts->async, std::move(device_mgr), r);
opts->is_async, std::move(device_mgr), r);
}

void TFE_DeleteContext(TFE_Context* ctx, TF_Status* status) { delete ctx; }
@@ -308,7 +308,7 @@ void TFE_ContextSetThreadLocalDevicePlacementPolicy(
}

// Note: this function looks up a thread local policy. So it should be called in
// the appropriate client thread. In particular, in async mode, it may not be
// the appropriate client thread. In particular, in asynchronous mode, it may not be
// safe to call this function from the async EagerExecutor threads.
extern TFE_ContextDevicePlacementPolicy TFE_ContextGetDevicePlacementPolicy(
TFE_Context* ctx) {
Copy path View file
@@ -76,7 +76,7 @@ typedef enum TFE_ContextDevicePlacementPolicy {
// Sets the default execution mode (sync/async). Note that this can be
// overridden per thread using TFE_ContextSetAsyncForThread.
TF_CAPI_EXPORT extern void TFE_ContextOptionsSetAsync(TFE_ContextOptions*,
unsigned char async);
unsigned char is_async);

TF_CAPI_EXPORT extern void TFE_ContextOptionsSetDevicePlacementPolicy(
TFE_ContextOptions*, TFE_ContextDevicePlacementPolicy);
@@ -125,7 +125,7 @@ TFE_ContextGetDevicePlacementPolicy(TFE_Context*);

// Overrides the execution mode (sync/async) for the current thread.
TF_CAPI_EXPORT extern void TFE_ContextSetAsyncForThread(TFE_Context*,
unsigned char async,
unsigned char is_async,
TF_Status* status);

// Causes the calling thread to block till all ops dispatched in async mode
@@ -184,7 +184,7 @@ TF_CAPI_EXPORT extern TF_Tensor* TFE_TensorHandleResolve(TFE_TensorHandle* h,
// that shares the underlying buffer. Otherwise, it currently requires at least
// one of the source or destination devices to be CPU (i.e., for the source or
// destination tensor to be placed in host memory).
// If async execution is enabled, the copy may be enqueued and the call will
// If asynchronous execution is enabled, the copy may be enqueued and the call will
// return "non-ready" handle. Else, this function returns after the copy has
// been done.
TF_CAPI_EXPORT extern TFE_TensorHandle* TFE_TensorHandleCopyToDevice(
@@ -341,7 +341,7 @@ TF_CAPI_EXPORT extern void TFE_OpSetAttrFunctionList(TFE_Op* op,
// the size of 'retvals' is less than the number of outputs. This call sets
// *num_retvals to the number of outputs.
//
// If async execution is enabled, the call may simply enqueue the execution
// If asynchronous execution is enabled, the call may simply enqueue the execution
// and return "non-ready" handles in `retvals`. Note that any handles contained
// in 'op' should not be mutated till the kernel execution actually finishes.
//
@@ -374,7 +374,7 @@ TF_CAPI_EXPORT extern void TFE_ContextDisableRunMetadata(TFE_Context* ctx);
// Populates the passed-in buffer with a serialized RunMetadata protocol buffer
// containing any run metadata information accumulated so far and clears this
// information.
// If async mode is enabled, this call blocks till all currently pending ops are
// If asynchronous mode is enabled, this call blocks till all currently pending ops are
// done.
TF_CAPI_EXPORT extern void TFE_ContextExportRunMetadata(TFE_Context* ctx,
TF_Buffer* buf,
@@ -56,26 +56,25 @@ limitations under the License.

struct TFE_ContextOptions {
TF_SessionOptions session_options;
// true if async execution is enabled.
bool async = false;
bool is_async = false;
TFE_ContextDevicePlacementPolicy policy{TFE_DEVICE_PLACEMENT_SILENT};
tensorflow::ServerDef server_def;
};

struct TFE_Context {
explicit TFE_Context(const tensorflow::SessionOptions& opts,
TFE_ContextDevicePlacementPolicy default_policy,
bool async,
bool is_async,
std::unique_ptr<tensorflow::DeviceMgr> device_mgr,
tensorflow::Rendezvous* rendezvous)
: context(opts,
static_cast<tensorflow::ContextDevicePlacementPolicy>(
default_policy),
async, std::move(device_mgr), rendezvous) {}
is_async, std::move(device_mgr), rendezvous) {}

explicit TFE_Context(
const tensorflow::SessionOptions& opts,
TFE_ContextDevicePlacementPolicy default_policy, bool async,
TFE_ContextDevicePlacementPolicy default_policy, bool is_async,
tensorflow::DeviceMgr* local_device_mgr,
tensorflow::Rendezvous* rendezvous,
std::unique_ptr<tensorflow::ServerInterface> server,
@@ -86,7 +85,7 @@ struct TFE_Context {
: context(opts,
static_cast<tensorflow::ContextDevicePlacementPolicy>(
default_policy),
async, local_device_mgr, rendezvous, std::move(server),
is_async, local_device_mgr, rendezvous, std::move(server),
std::move(remote_eager_workers), std::move(remote_device_mgr),
remote_contexts) {}

Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.