-
Notifications
You must be signed in to change notification settings - Fork 578
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: TensorFlow Canonical Type System #208
Changes from 19 commits
0395440
4e58ec3
4553626
001efcc
71a5164
0842a2f
e4fd7fa
4666892
de72ac5
e9720c1
e3224da
53680e5
7b21bf7
c951112
aae458f
f923705
dc924a7
5d4ce1d
fb3e2f2
c42f292
198a1d0
5bf02fb
2f4d14d
52d5617
d27dcb7
5bff3e7
f692cc0
1b20c1f
451b627
c3a1c2d
f551da1
cbe5eaf
b8aae73
e7b0fef
8797e73
882dbb6
97d5d8d
e86de57
fac8975
9c2f37f
8e47bc1
bc33a1a
af51680
c8816c4
327ceda
aeff124
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
# TensorFlow Canonical Type System | ||
|
||
| Status | Proposed | | ||
:-------------- |:---------------------------------------------------- | | ||
| **RFC #** | [208](https://github.com/tensorflow/community/pull/208) | ||
| **Author(s)** | Dan Moldovan (mdan@google.com) | | ||
| **Sponsor** | Gaurav Jain (gjn@google.com) | | ||
| **Updated** | 2020-02-19 | | ||
|
||
## Objective | ||
|
||
This RFC proposes a new TensorFlow module and namespace (`tf.types`) dedicated to storing implementation-free type definitions, similar to C++ header files. This module has no other dependencies inside TensorFlow, so any other internal module can depend on it to ensure interoperability without the risk of creating circular dependencies. These definitions can also be used by external users, for example in pytype annotations. | ||
The RFC focuses on the Python API, however the design should be reviewed with cross-language consistency in mind. | ||
## Motivation | ||
|
||
**Interoperability and composability**. A set of standard types that formalize an interface and decouples it from implementation ensures composability between components, especially when multiple implementations are involved. | ||
|
||
**Supports the [acyclic dependencies principle](https://en.wikipedia.org/wiki/Acyclic_dependencies_principle)**. In many instances, circular dependencies are caused between low-level complex components that need to compose (e.g. autograph needs to recognize datasets, and datasets need to use autograph). Interface extraction is a common pattern for breaking such cycles. | ||
|
||
**Supports pytype**. A set of static types that is consistent under Python’s `isinstance`/`issubclass` is required to support [PEP-484 type annotations](https://www.python.org/dev/peps/pep-0484/) in TensorFlow. This module can serve as the basis for that. | ||
|
||
**Helps formalize requirements for new APIs**. Having a formal, implementation-independent definition for things such as tensors, variables, iterables, iterators makes it easy to document and test compatibility between APIs. | ||
|
||
## User Benefit | ||
|
||
Application developers may use these canonical definitions for pytype annotations. | ||
|
||
Library developers can more easily define their API interfaces by referring to this namespace. | ||
|
||
Developers of modules internal to TensorFlow can use this module to avoid creating circular dependencies. | ||
|
||
## Design Proposal | ||
|
||
### The `tf.types` Namespace / Module | ||
All the declarations exposed under the `tf.types` namespace reside in the `python/types/*.py` module. These are [abstract base classes](https://docs.python.org/3.7/library/abc.html) with a bare minimum of method definitions and minimal or no implementation, which serve to formalize and document the contract of common types such as `Tensor`, `Variable`, etc. | ||
|
||
These definitions may be used as PEP 484 type hints, although in some cases they may be type- or shape- erased (for example, `tf.types.Tensor` may not necessarily be parametrized by `dtype` or `shape`). Note however that designs which parametrize on shape do exist, see for instance [tensorflow#31579](https://github.com/tensorflow/tensorflow/issues/31579). | ||
|
||
The type definitions are consistent with `isinstance` and `issubclass`. For example, `isinstance(tf.Tensor, tf.types.Tensor) == True`. | ||
|
||
### General Principles | ||
This module should not contain any implementation code. An advantage of that is that users exploring the implementation of specific types will not need to inspect this module. However, users who do not wish to inspect the code may visit the documentation of these generic types to better understand specifically what are the concrete subclasses of this type expected to do. | ||
|
||
The `tf.types` module may depend on external packages (such as `numpy`) _strictly for the purpose of defining type annotations and documentation_. No dependencies to other TensorFlow interfaces are allowed. Any dependencies on external packages which themselves depend on TensorFlow are expressly forbidden. | ||
|
||
Changes definitions inside `tf.types` must be approved by TensorFlow leads, and typically should be accompanied by an RFC. | ||
|
||
All type declarations are based on PEP-484 and related specifications, and defined using [typing](https://docs.python.org/3/library/typing.html), with the aim of being compatible with static type checkers like [pytype](https://github.com/google/pytype), [mypy](http://mypy-lang.org/), [pyre](https://pyre-check.org/). | ||
|
||
It is recommended that internal and external type annotations, `isinstance` and `issubclass` checks use these types, eventually deprecating helpers like `tf.is_tensor`. However, concrete types continue to exist - for example, variables are instances of `tf.Variable`, which is now a subclass of `tf.types.Variable`. | ||
|
||
Class type definitions define a minimum of abstract methods and properties which are required for pytype compatibility. | ||
|
||
### Support for `tf.function`'s `input_signature` | ||
The type system listed here can be expanded to allow input signatures using type annotations, see for instance [this thread](https://github.com/tensorflow/tensorflow/issues/31579). | ||
|
||
### Initial Type Hierarchy | ||
TensorFlow generally adopts an incremental development method. This RFC aims to remain consistent with that. | ||
|
||
Below are listed the major types presently used in TensorFlow. All types included in this list are subject to [normal compatibility rules](https://www.tensorflow.org/guide/versions), so they are unlikely to change in the future. It is therefore preferable to maintain a strict minimum of orthogonal declarations and carefully vet any additions. | ||
|
||
Most of these symbols will not be initially exported as public symbols. Only internal submodules will be able to use unexported types. The unexported types may be gradually exposed under `tf.types` or under `tf.types.experimental`. | ||
|
||
The initial type hierarchy is focused on V2 symbols. We expect to encounter places where these symbols would not be compatible with V1 code; in such cases, the V1 symbols will not be affected. | ||
|
||
#### Types created by this RFC | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are the specifics of each type out of scope for this RFC? i.e. which methods will be available on a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was hoping so, although I think we'll need to define them sooner than later if type annotations are to be useful. |
||
|
||
These types will be added with the initial creation of the `tf.types` namespace. | ||
|
||
* Core tensor types | ||
|
||
* `DType` | ||
* `Shape` | ||
* `Tensor` - generic dense tensor | ||
|
||
* `Symbol` - the regular graph tensor | ||
* `Value` - eager tensors | ||
|
||
* `TensorLike` - any type that can be implicitly converted to `Tensor` (see for example https://github.com/tensorflow/addons/blob/master/tensorflow_addons/utils/types.py) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. Since using ABC may have performance implications as you pointed out, we may need to think of a different mechanism, like a concrete There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that regardless of the implementation, the relationship between Personally, I'm fine with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My initial expectation is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a section about this - PTAL. It turns out that protocols do support static type checking, and can support isinstance in newer Python versions too. I'm not sure that |
||
|
||
* `Variable` | ||
|
||
#### Potential types for subsequent implementation | ||
|
||
These types are raised for discussion by this RFC, but are not part of the original implementation, unless they are strictly required for consistency (to be determined during the initial submission). | ||
|
||
Many of these are expected to be required when breaking the cyclic dependencies that currently exist between submodules. However, it is hoped that opening them up for discussion early can help create a more coherent type system. | ||
|
||
* Container types | ||
|
||
* `Composite` - low-level static structure (opaque to GraphDef/IR) | ||
* `Module` - builder for structures of `Variables` (invisible to GraphDef/IR) | ||
* `Optional` - basic programming construct | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this an alias to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add a clarification - |
||
* `List` - superclass for `TensorArray`, `Queue`, etc. (opaque to GraphDef/IR) | ||
|
||
* Higher-level types | ||
* `Dataset` - ETL pipeline | ||
* `Iterator` - basic stateful programming construct | ||
* `Iterable` - basic stateless programming construct | ||
* `Function` - basic programming construct | ||
* `Error` - superclass of all TF-specific errors | ||
|
||
* Distributed types | ||
* `DistributedDataset` - collective ETL | ||
* `DistributedIterator` - collective iterator | ||
|
||
* Low-level execution primitives | ||
* `Graph` - GraphDef/IR program | ||
* `FunctionGraph` - IR of a single concrete function | ||
|
||
### Alternatives Considered | ||
* N/A | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider the alternative where not everything is in the same module and we don't use the trick of making this:
I like the idea of having types in tensorflow, but this whole ABC black magic of avoiding dependecies is done because internally, Tensorflow has many, many circular dependencies (see here). In python, the more black magic we do, the more it will come back to bite us, see https://github.com/tensorflow/community/pull/208/files#r382246511 . Python types are not supposed to be used in this way and I'm afraid that we're going to have many problems in the future just for the sake of supporting circular dependencies in the TF codebase. Like for example some type system being confused about our types, or we'll realize the need to redeclare all the classes' interfaces in tf.types, breaking the DRY rule. The alternative is to declare types like In short, making Tensorflow types without any ABC tricks shouldn't create new circular dependencies, they'll just make the existing ones come to light. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I agree with the broader comment: we want to avoid, rather than facilitate, circular dependencies. This RFC specifically aims to support the extract interface pattern for removing such circular dependencies. That said, I fear the pattern is not a universal solution. For example, in the Keras instance you mentioned, the circular dependency cannot be broken by extracting types and requires moving some code around. I'm not sure I fully follow the By "ABC tricks" I suspect you are referring to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I'm referring to the trick of having all public classes of tensorflow subclass a class defined in tf.types, I think it's a brittle trick to enable the use of
I see, but what is the benefit of that? As a user, what bonus do I get by doing A side note for the user experience: A user will first try to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this as a discussion topic. I'm inclined to agree - we don't need both The Python types module uses a similar pattern. Related, if you have additional thoughts on how |
||
|
||
### Performance Implications | ||
* No performance implications expected. At most, we are adding a small number of levels to the class tree of some objects. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One caveat here is that >>> import abc
>>> class A(abc.ABC): pass
>>> class B: pass
>>> class SA(A): pass
>>> class SB(B): pass
>>> sa = SA()
>>> sb = SB()
>>> %timeit isinstance(sa, A)
237 ns ± 0.45 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
>>> %timeit isinstance(sb, B)
51.2 ns ± 0.245 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each) While this is of course an artificial benchmark, and a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very interesting. It sounds like we may want to avoid adding ABCMeta superclasses in instances when performance is critical, like eager tensors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One option is to define these types without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. Just to be clear, the types would be non-extensible using ABCs registration mechanism, and one could still extend using traditional subclassing. |
||
|
||
### Dependencies | ||
* None, by definition. | ||
|
||
### Engineering Impact | ||
* Engineering impact: Separate interfaces allow for faster loading times by reducing coupling between modules. | ||
* Maintenance: Minimal maintenance overhead since there is no functionality involved. The TensorFlow team and contributors will maintain the documentation up to date. Changes should be reviewed and approved by the TensorFlow team leads. | ||
|
||
### Platforms and Environments | ||
* Platforms: Python only, in the first stage. However, the type system should be aligned as much as possible with the core types in the TensorFlow runtime, and be language-independent as much as possible. | ||
* Execution environments: The type system is independent of platform. This also implies that no platform-specific types (such as `TPUTensor`) exist. | ||
|
||
### Best Practices | ||
* This set of type definitions support the acyclic dependencies principle, by requiring that implementations avoid lateral dependencies (e.g. with a linter rule). | ||
|
||
### Tutorials and Examples | ||
* As the design matures, we plan to showcase libraries that leverage this pattern. | ||
* Type annotations will be included in existing tutorials as definitions become final. | ||
|
||
### Compatibility | ||
* New minor version. Existing classes (`tf.Tensor`) will become subclasses of the new type interfaces. | ||
* Most subcomponents of TF (Lite, distributed, function, SavedModel) will depend on this new module, although their functionality is not impacted. | ||
* Libraries which depend on TensorFlow are encouraged to refer to `tf.types` definitions, rather than the concrete implementations for better future compatibility. | ||
|
||
### User Impact | ||
* Users will see a new `tf.types` module, that may be referenced from documentation and type annotations. | ||
|
||
|
||
## Questions and Discussion Topics | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would the new type system unified with https://www.tensorflow.org/api_docs/python/tf/TypeSpec? I think having both name space might confuse user about which to use, and we probably want to unify them into one if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, and I agree it would be great to either unify them, or think of ways to make the differences clearer and more intuitive - perhaps we should begin to refer to them as [TF] Python types and TF [native] types. |
||
* Single flat vs. hierarchical namespace - for example: `tf.types.distribute.Dataset`, or `tf.types.DistributedDataset`? | ||
* The inclusion of more specialized `Graph` types, such as `FuncGraph`, `CondBranchFuncGraph`, `WhileCondFuncGraph`, `WhileBodyFuncGraph`. It’s unclear where these should be defined, however internal submodules needs these subtypes to maintain acyclic dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should not be a requirement as this is not a requirements for python types to support
isinstance
. It also restricts us: we can't use anything in thetyping
module and protocols. Ex:I believe this RFC should include some examples of implementation to make things clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It seems like we need to think through the interaction between classical interfaces, protocols and how they play with injection mechanisms like
register_tensor_conversion_function
. Ideally we wouldn't need so many mechanisms for the same thing.Regarding protocols, I suspect @runtime_checkable can help. Although, it could come with a performance hit, as in the case of ABCMeta.
I'll look into elaborating this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a section proposing the use of protocols for custom Tensor types - I think that is in line with the use of
typing
that you suggested as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much, I'll take a look whenever I can.