RFC: Kernel and Op Implementation and Registration API #133

sjamesr · 2019-08-14T22:06:39Z

Comment period is open until 2019-08-28

Kernel and Op Implementation and Registration API

Status	Proposed
Author(s)	James Ring (sjr@google.com), Anna Revinskaya (annarev@google.com)
Sponsor	Günhan Gülsoy (gunan@google.com)
Updated	2019-08-14

Objective

Tensorflow (TF) currently provides a C++ API for implementing kernels and ops. The Voltron project aims to create a modular/plugin-based TF implementation with API and ABI surfaces. Plugins will be able to create and register custom kernel and op implementations.

In order to provide a stable ABI, the Voltron team has chosen to provide C APIs to plugin authors. This document introduces the C API for op and kernel registration. For authors who wish to continue using C++ to interface with TensorFlow, an ABI-stable C++ header-only API is provided.

alextp

Some key concerns are still missing from this proposal.

For example, we need to handle forward compatibility (is there a way for a single kernel to work across versions of TF which are from before it was built?) similar to how we discussed it in the filesystem API RFC.

This proposal would also be much easier to read if it had proposed code files for a full simple example op registration and kernel definition, so we can properly understand the consequences of this API (including error handling, etc).

alextp · 2019-08-15T21:42:32Z

rfcs/20190814-kernel-and-op-registration.md

+
+// Bitcast_Create, Bitcast_Compute and Bitcast_Delete actually implement the
+// kernel. See the section below for discussion on kernel implementation.
+static void* Bitcast_Create(TF_OpKernelConstruction* context) {


In TF today op kernel construction can fail (and if often does, as it validates attributes etc). This should have a status somewhere.

OpKernelConstruction itself contains a status which can be set with TF_OpKernelConstruction_Failure. I'll update the doc.

alextp · 2019-08-15T21:43:08Z

rfcs/20190814-kernel-and-op-registration.md

+  return (void*) k;
+}
+
+static void* Bitcast_Compute(void* k, TF_OpKernelContext* context) {


What is the return value here? Outputs are not supposed to be allocated and then returned (they are allocated via the context) so I don't know why this is not void.

This is a typo, well spotted :)

alextp · 2019-08-15T21:43:43Z

rfcs/20190814-kernel-and-op-registration.md

+  void (*compute_func)(void*, TF_OpKernelContext*),
+  void (*delete_func)(void*));
+
+void TF_RegisterKernelBuilder(const char* name, TF_KernelBuilder* builder,


Why two functions instead of one function?

The kernel builder may be customized with other calls, such as TF_KernelBuilder_TypeConstraint and TF_KernelBuilder_HostMemory, I'll add these to the doc.

alextp · 2019-08-15T21:46:00Z

rfcs/20190814-kernel-and-op-registration.md

+    int64_t cost_per_unit,
+    void (*fn)(int64_t, int64_t));
+
+TF_CAPI_EXPORT extern void TF_ThreadPool_ParallelForWithWorkerId(


One thing which is not obvious when reading this proposal is that in C++ we need to wrap almost every call to a method in the context with the op_requires_ok macro to terminate execution in case of an error.

The same will need to be done here as all of these can fail, so I think we need to be explicit about the code the user is expected to write.

Context is just used to call accessors, for e.g.:
auto thread_pool = context->device()->tensorflow_cpu_worker_threads()->workers;

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/kernels/matrix_diag_op.cc#L323

We could modify ThreadPool interface though to also return a Status. Then, we can also take TF_Status output parameter in C API.

I added a section below called "Getting Status when using device APIs". Basically, since the status is not on the surface of APIs that we are wrapping (we could add it to ThreadPool, but probably not to Eigen::StreamInterface), I propose to have a separate function call to get status.

Let me know if this sounds good.

karllessard · 2019-08-27T17:11:50Z

I don't know if this is in the scope of this RFC but how will be registered ops be exposed through the C API? Right now, both Ops defs and API defs are returned as proto buffers, will that remain the same?

Since at some points we want to move out the different language bindings out of the core repository (e.g. Java), for me it does not make sense to keep the per-language API defs in there. What would be the best strategy then?

@sjamesr, @annarev : any reply for this? thanks

annarev · 2019-09-08T23:22:26Z

@karllessard sorry I missed your question earlier. See replies inline.

I don't know if this is in the scope of this RFC but how will be registered ops be exposed through the C API? Right now, both Ops defs and API defs are returned as proto buffers, will that remain the same?

Yes, the way Op defs and API defs are returned will remain the same. These accessors are already a part of our C API. But let us know if you think some part of the API won't work well for some languages.

Since at some points we want to move out the different language bindings out of the core repository (e.g. Java), for me it does not make sense to keep the per-language API defs in there. What would be the best strategy then?

I think language bindings can already be removed with current API. You can keep ApiDef files with the language bindings (they are just read when generating language APIs and aren't used anywhere else). For example, Go API is generated based on a set of directories that contain ApiDef files: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/go/genop/main.go#L39 that are added using calls to TF_ApiDefMapPut (https://github.com/tensorflow/tensorflow/blob/master/tensorflow/go/genop/internal/api_def_map.go#L91). So, I would think we can move language API designed in this way to a separate repo. However, I haven't checked how Java APIs are generated.

karllessard · 2019-09-09T02:18:13Z

However, I haven't checked how Java APIs are generated.

The Java API works pretty much the same, it has a op generator written in C++ that uses directly TF internal classes to read the API defs found in this directory merged with the base one.

But what I'm wondering is that if the op kernels are now implemented and registered outside the core, those API defs cannot reside in the core anymore. Having them in the language bindings would mean that the owners of a plugin must go in each client library (Python, Java, Go...) to add the definition of their op kernels in there as well, which does not sounds right neither.

So then, maybe what would be best is if the API defs are registered by the plugins using similar mechanisms than the ops (e.g. via TF_NewOpDefinitionBuilder, or via a new TF_NewApiDefinitionBuilder method?), and we should let diligent plugin maintainers the ability to provide a API def per language.

What do you think?

annarev · 2019-09-11T19:04:09Z

Good point. We definitely need a way to register an ApiDef for an op.

theadactyl · 2019-11-19T22:14:41Z

@sjamesr what's the status of this revision?

seanpmorgan · 2019-12-20T15:08:58Z

@sjamesr @gunan @annarev This is the most important change we are looking forward to for TF-Addons as we get a ~few issues per week on ABI issues. Would it be possible to get a status update on this & expected timeline. It would be very beneficial for us to know how much work we should put into workarounds given that this is on the horizon.

byronyi · 2019-12-22T15:14:14Z

What’s the current ABI in TF core wheels?

bhack · 2020-04-14T16:05:37Z

Comment period is open until 2019-08-28. /cc @ematejska

I removed device section of this doc. We never made a final decision on the device solution and then I moved to work on something else. If we come up with a final decision for device, we can create another RFC (I removed my name from authors as well since my only contribution was device API. ). The rest of the doc can be merged.

annarev · 2020-06-03T00:59:46Z

@theadactyl , @seanpmorgan , @byronyi Really sorry for not providing any updates earlier. We really haven't had much work on this after initial Kernel API implementation by @sjamesr : https://github.com/tensorflow/tensorflow/blob/master/tensorflow/c/kernels.h . Since then, James moved to work on another project and I was focusing on other tasks in TensorFlow.

It seems best to merge this RFC since this doc has been pending for a while and initial implementation was submitted. I removed the device RFC part, since it wasn't finalized. Also, I will keep in mind that we need to support ApiDefs and look into a separate RFC for it when I get time.

For now, this RFC as it stands can be merged.

byronyi · 2020-06-03T04:12:08Z

@annarev Thanks for rolling things forward.

Btw, would you mind to share with the community the current status of the "Voltron" project?

annarev · 2020-06-04T17:03:21Z

I think @mihaimaruseac completed the filesystem part (https://github.com/tensorflow/community/blob/master/rfcs/20190506-filesystem-plugin-modular-tensorflow.md). Unfortunately, we got caught up in other work and didn't get to spend as much time working on other tasks listed in the doc.

@gunan Adding Gunhan in case he has more details

mihaimaruseac · 2020-06-04T17:08:52Z

Only the POSIX part. Windows should come by end of this quarter and GCS and S3 are part of a GSoC project. If there are no more delays (like there have been in the past), by TF 2.4 we should be able to convert filesystems to the Voltron world

add draft kernel RFC

b6ea8f4

sjamesr requested review from ewilderj, goldiegadde, martinwicke and theadactyl as code owners August 14, 2019 22:06

googlebot added the cla: yes label Aug 14, 2019

ewilderj self-assigned this Aug 14, 2019

ewilderj added this to Needs attention in RFC management via automation Aug 14, 2019

ewilderj added the RFC: Proposed RFC Design Document label Aug 14, 2019

ewilderj changed the title ~~add draft kernel RFC~~ RFC: Kernel and Op Implementation and Registration API Aug 14, 2019

ewilderj moved this from Needs attention to Open reviews in RFC management Aug 14, 2019

Update 20190814-kernel-and-op-registration.md

19ad326

alextp suggested changes Aug 15, 2019

View reviewed changes

Update 20190814-kernel-and-op-registration.md

1c87177

karllessard mentioned this pull request Sep 9, 2019

[WIP] Add core-api modules using JavaCPP karllessard/tensorflow-java-sandbox#2

Closed

karllessard mentioned this pull request Oct 10, 2019

Import Java operators generator from main repo tensorflow/java#2

Merged

ewilderj moved this from Open reviews to Awaiting Committee Notes in RFC management Oct 11, 2019

ewilderj moved this from Awaiting Committee Notes to In Revision in RFC management Oct 11, 2019

seanpmorgan mentioned this pull request Nov 7, 2019

GCloud tf-gpu Image and tfa compatability? tensorflow/addons#676

Closed

bhack mentioned this pull request Nov 19, 2019

Create rfc-admin-guide.md #165

Merged

seanpmorgan mentioned this pull request Dec 20, 2019

File not found during import tensorflow/addons#792

Closed

seanpmorgan mentioned this pull request Jan 10, 2020

Lazy Load Custom Ops tensorflow/addons#855

Closed

seanpmorgan mentioned this pull request Jan 31, 2020

Custom Op Linux ABI Incompatibility: Undefined Symbol tensorflow/addons#987

Closed

seanpmorgan mentioned this pull request Mar 13, 2020

image.rotate Segfault with TensorFlow 2.2.0rc0 and tf-nightly tensorflow/addons#1298

Closed

gabrieldemarmiesse mentioned this pull request Mar 16, 2020

Make a better UX around the issue of tensorflow ABI instability. tensorflow/addons#1317

Closed

penpornk mentioned this pull request May 14, 2020

RFC: TensorFlow on DirectML #243

Merged

annarev requested a review from ematejska as a code owner June 3, 2020 00:51

Changed "Updated" date

a926a55

Moving to Accepted

135293e

ematejska approved these changes Jun 4, 2020

View reviewed changes

ematejska added RFC: Accepted RFC Design Document: Accepted by Review and removed RFC: Proposed RFC Design Document labels Jun 4, 2020

ematejska moved this from In Revision to Accepted RFCs in RFC management Jun 4, 2020

ematejska merged commit 5001b24 into tensorflow:master Jun 4, 2020

seanpmorgan mentioned this pull request Jul 14, 2020

Package Addons on conda-forge tensorflow/addons#201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Kernel and Op Implementation and Registration API #133

RFC: Kernel and Op Implementation and Registration API #133

sjamesr commented Aug 14, 2019 •

edited by ewilderj

Loading

alextp left a comment

alextp Aug 15, 2019

sjamesr Aug 15, 2019

alextp Aug 15, 2019

sjamesr Aug 15, 2019 •

edited

Loading

alextp Aug 15, 2019

sjamesr Aug 15, 2019

alextp Aug 15, 2019

annarev Aug 21, 2019

annarev Aug 22, 2019

karllessard commented Aug 27, 2019 •

edited

Loading

annarev commented Sep 8, 2019

karllessard commented Sep 9, 2019

annarev commented Sep 11, 2019

theadactyl commented Nov 19, 2019

seanpmorgan commented Dec 20, 2019

byronyi commented Dec 22, 2019

bhack commented Apr 14, 2020

annarev commented Jun 3, 2020

byronyi commented Jun 3, 2020

annarev commented Jun 4, 2020

mihaimaruseac commented Jun 4, 2020

RFC: Kernel and Op Implementation and Registration API #133

RFC: Kernel and Op Implementation and Registration API #133

Conversation

sjamesr commented Aug 14, 2019 • edited by ewilderj Loading

Kernel and Op Implementation and Registration API

Objective

alextp 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

sjamesr Aug 15, 2019 • edited Loading

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

karllessard commented Aug 27, 2019 • edited Loading

annarev commented Sep 8, 2019

karllessard commented Sep 9, 2019

annarev commented Sep 11, 2019

theadactyl commented Nov 19, 2019

seanpmorgan commented Dec 20, 2019

byronyi commented Dec 22, 2019

bhack commented Apr 14, 2020

annarev commented Jun 3, 2020

byronyi commented Jun 3, 2020

annarev commented Jun 4, 2020

mihaimaruseac commented Jun 4, 2020

sjamesr commented Aug 14, 2019 •

edited by ewilderj

Loading

sjamesr Aug 15, 2019 •

edited

Loading

karllessard commented Aug 27, 2019 •

edited

Loading