Skip to content
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

RFC: Best practices for custom operations in TensorFlow #126

Merged
merged 11 commits into from
Oct 11, 2019

Conversation

alextp
Copy link
Contributor

@alextp alextp commented Jul 26, 2019

Review period is open through 2019-08-09

Best practices for custom operations in TensorFlow

Status Accepted
Author(s) Alexandre Passos (apassos@google.com
Sponsor Karmel Allison (karmel@google.com)
Updated 2019-06-10

For most of TF’s history, it was very expensive for third-party packages or libraries to release their own tf operations. This created pressure to put ops in tf core or in tf contrib, which created some uncertainty around support stories and backwards compatibility.

Around (but technically not a part of) TF 2.0, however, TensorFlow supports a straightforward way for third-party packages to build and deploy their own custom TF ops. To maintain a healthy ecosystem, we recommend best practices in this RFC.

@ewilderj ewilderj changed the title Custom op rfc RFC: Best practices for custom operations in TensorFlow Jul 26, 2019
@ewilderj ewilderj added 2.0 TensorFlow 2.0 development RFC: Proposed RFC Design Document labels Jul 26, 2019
@awav
Copy link

awav commented Jul 28, 2019

@alextp, That's fantastic! Is there a plan to add XLA op as an example as well?

their op names are globally unique across the entire TF ecosystem. To do so,
prepend the package’s name to the op’s name and separate with a ‘>’. An op named
“MatMul” inside the “tensorflow_addons” package should be named “Addons>MatMul”,
for example.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tensorflow/sig-addons-maintainers Seems that we need to rename all C++ ops? cc @seanpmorgan

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Since an op’s name uniquely identifies it, different TF packages should ensure
their op names are globally unique across the entire TF ecosystem. To do so,
prepend the package’s name to the op’s name and separate with a ‘>’. An op named
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make tf.load_op_library throw a warning or even an error if there is no package name specified? AFAIK only custom-op repos utilize this function

@seanpmorgan
Copy link
Member

So I took a stab at implementing this for Addons... but currently it'll throw an "Invalid Op Name" because of the >: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/framework/op_def_util.cc#L254

What will the behavior be once the package separating character is accepted? At the moment the camel case op name is lower-cased and separated by _ in the python object's method. For example SkipGramGenerateCandidates becomes skip_gram_generate_candidates.

obj.addons_skip_gram_generate_candidates seems a little verbose seeing as the obj is already loaded using tf.load_library from a known shared_object.

stories and backwards compatibility.

Around (but technically not a part of) TF 2.0, however, TensorFlow supports [a
straightforward way for third-party package to build and deploy their own custom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/package/packages/

rfcs/20190726-custom-ops.md Show resolved Hide resolved

| Status | Proposed |
:-------------- |:---------------------------------------------------- |
| **Author(s)** | Alexandre Passos (apassos@google.com |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing )

- each downstream package can choose its own backward and forward compatibility
processes, allowing fine-grained trade-offs between velocity and stability

## Out-of-tree ops must be namespaced
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The namespace problem is similar to the naming discussion in the Standardizing Composite Operations In TensorFlow RFC.

Any thought around making the naming scheme more unified?

change Status to Accepted
@ewilderj
Copy link
Contributor

Ready to merge the RFC but I never heard back after the design meeting. Are there any notes to share? Was this design accepted?

@martinwicke
Copy link
Member

Yes, this can be merged. Comments have already been integrated as far as I can tell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 TensorFlow 2.0 development cla: yes RFC: Accepted RFC Design Document: Accepted by Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants