From cafd74791618db3264d7a3e85e48d6e29540dbff Mon Sep 17 00:00:00 2001 From: Alex Passos Date: Mon, 16 Mar 2020 10:25:50 -0700 Subject: [PATCH 1/8] experimental API policy --- governance/api-reviews.md | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index 61dc3e64e..7aa1bb357 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -200,3 +200,56 @@ When adding new ops, look for: it CPU, GPU, XLA, TPU, etc) and prefer to have at least a plan for writing device-agnostic code - should the python layer for this operation support raggedtensor/sparsetensor? + +## Experimental APIs + +Experimental APIs are APIs which have the word 'experimental' somewhere in their +name; for example `tf.experimental.foo`, or `tf.foo.experimental.Bar`, or +`tf.foo(experimental_bar=True)` or `tf.Foo().experimental_bar()`. + +Experimental APIs are APIs intended to be added to TensorFlow as-is, but which +we reserve the right to change in backwards-incompatible ways if we have +to. + +No temporary APIs should be added to experimental (i.e. "we just need this until +certain bugfix or certain new feature becomes available" is not a valid reason +to add an API with experimental in the name.) + +No API with known deficiencies should be added to experimental. Experimental +APIs should, to the best of our knowledge, not be expected to change in a known +way (no argument with a known bad name, etc). + +The same amount of due diligence required for a real API is required for an +experimental API: this means tests, benchmarks, documentation, end-to-end +examples, etc + +Experimental APIs are not a license to break users. This means: + 1. we do not remove experimental APIs which are widely used without an effort + to help migrate users away + 2. experimental APIs are not removed without warning and don't have + backwards-incompatible changes made to them without warning (the warning can be + a deprecation on version 2.x and removal on 2.x+ 1, but plain removal on 2.x + with no notice on 2.x-1 is not ok) + +Small changes which are mentioned in relnotes and have obvious fixes might be +made (for example if adding a new argument to a long argument list and we +believe there are no pass-by-position users we might allow the new argument to +be added to the middle and not the end of the parameter list). + +Large backwards-incompatible changes to experimental APIs still require an +`experimental_foo_v2` or similar backwards-compatible evolution plan to avoid +breaking users of the existing experimental API. + +No API endpoint should stay in experimental forever. If a particular +experimental API hasn't had major changes in two minor releases we should remove +the experimental annotation from the API name or delete it. If we do want to +delete it we need to have a deprecation plan that can migrate all users to some +other API endpoint or composition of existing APIs. + +When removing the experimental annotation we should, if at all possible, allow +escape routes to not break existing code. This means toplevel symbols +`tf.experimental.foo` and methods like `tf.Class.experimental_foo` should get a +deprecation warning on 2.x before deletion on 2.x+1; we should use the +doc_controls decorators to not pollute API docs with deprecated "graduated" +experimental APIs. For experimental function arguments we should consider +catching **kwargs to raise the proper warnings for at least one version. From 8165cb915fe2baa56b921b1415a4d008b8ce9bfa Mon Sep 17 00:00:00 2001 From: Alex Passos Date: Mon, 16 Mar 2020 10:30:07 -0700 Subject: [PATCH 2/8] clarifying known deficiency --- governance/api-reviews.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index 7aa1bb357..23f5727e4 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -217,7 +217,10 @@ to add an API with experimental in the name.) No API with known deficiencies should be added to experimental. Experimental APIs should, to the best of our knowledge, not be expected to change in a known -way (no argument with a known bad name, etc). +way (no argument with a known bad name, etc). Experimental can, however, be used +for APIs which are a work-in-progress: it's fine to add experimental methods to +a base class even if those methods are only implemented on some subclasses as +long as we expect all classes to eventually implement those. The same amount of due diligence required for a real API is required for an experimental API: this means tests, benchmarks, documentation, end-to-end From edb029227c0e889220bc4ef1f8a4dd4196dbb335 Mon Sep 17 00:00:00 2001 From: Alexandre Passos Date: Fri, 20 Mar 2020 10:28:23 -0700 Subject: [PATCH 3/8] Update governance/api-reviews.md Co-Authored-By: Martin Wicke <577277+martinwicke@users.noreply.github.com> --- governance/api-reviews.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index 7aa1bb357..7838417f6 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -228,7 +228,7 @@ Experimental APIs are not a license to break users. This means: to help migrate users away 2. experimental APIs are not removed without warning and don't have backwards-incompatible changes made to them without warning (the warning can be - a deprecation on version 2.x and removal on 2.x+ 1, but plain removal on 2.x + a deprecation on version 2.x and removal on 2.x+1, but plain removal on 2.x with no notice on 2.x-1 is not ok) Small changes which are mentioned in relnotes and have obvious fixes might be From a75ea94de8319ca594a18034469eaa4c62fc0e81 Mon Sep 17 00:00:00 2001 From: Alexandre Passos Date: Fri, 20 Mar 2020 10:28:32 -0700 Subject: [PATCH 4/8] Update governance/api-reviews.md Co-Authored-By: Martin Wicke <577277+martinwicke@users.noreply.github.com> --- governance/api-reviews.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index 7838417f6..57ca22bc9 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -252,4 +252,4 @@ escape routes to not break existing code. This means toplevel symbols deprecation warning on 2.x before deletion on 2.x+1; we should use the doc_controls decorators to not pollute API docs with deprecated "graduated" experimental APIs. For experimental function arguments we should consider -catching **kwargs to raise the proper warnings for at least one version. +catching `**kwargs` to raise the proper warnings for at least one version. From 935e14d9c49b3f55381fc4ea213e87fd234c632e Mon Sep 17 00:00:00 2001 From: Alex Passos Date: Fri, 20 Mar 2020 13:10:11 -0700 Subject: [PATCH 5/8] clarifications --- governance/api-reviews.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index 70baf6d7c..de1f34c9e 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -205,7 +205,9 @@ When adding new ops, look for: Experimental APIs are APIs which have the word 'experimental' somewhere in their name; for example `tf.experimental.foo`, or `tf.foo.experimental.Bar`, or -`tf.foo(experimental_bar=True)` or `tf.Foo().experimental_bar()`. +`tf.foo(experimental_bar=True)` or `tf.Foo().experimental_bar()`. We generally +prefer experimental namespaces when possible, so prefer +`tf.foo.experimental.Bar` over `tf.foo.ExperimentalBar`. Experimental APIs are APIs intended to be added to TensorFlow as-is, but which we reserve the right to change in backwards-incompatible ways if we have @@ -236,7 +238,7 @@ Experimental APIs are not a license to break users. This means: Small changes which are mentioned in relnotes and have obvious fixes might be made (for example if adding a new argument to a long argument list and we -believe there are no pass-by-position users we might allow the new argument to +believe there are few pass-by-position users we might allow the new argument to be added to the middle and not the end of the parameter list). Large backwards-incompatible changes to experimental APIs still require an From bf57a9d649be6a718a10f4fb1a041f6facb86732 Mon Sep 17 00:00:00 2001 From: Alex Passos Date: Mon, 6 Apr 2020 09:13:02 -0700 Subject: [PATCH 6/8] clarify policy for stuff which keeps changing --- governance/api-reviews.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index de1f34c9e..563d015cb 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -249,7 +249,9 @@ No API endpoint should stay in experimental forever. If a particular experimental API hasn't had major changes in two minor releases we should remove the experimental annotation from the API name or delete it. If we do want to delete it we need to have a deprecation plan that can migrate all users to some -other API endpoint or composition of existing APIs. +other API endpoint or composition of existing APIs. In rare cases experimental +APIs can continue to be iterated on after many releases (see TPUStrategy); this +only applies for fairly large API surfaces. When removing the experimental annotation we should, if at all possible, allow escape routes to not break existing code. This means toplevel symbols @@ -257,4 +259,6 @@ escape routes to not break existing code. This means toplevel symbols deprecation warning on 2.x before deletion on 2.x+1; we should use the doc_controls decorators to not pollute API docs with deprecated "graduated" experimental APIs. For experimental function arguments we should consider -catching `**kwargs` to raise the proper warnings for at least one version. +catching `**kwargs` to raise the proper warnings for at least one version (note +though that `**kwargs` is generally discouraged from our APIs; we prefer +explicitly named keyword arguments if at all possible). From de161b6d1783c3d00ffa06550780d078d676a8d5 Mon Sep 17 00:00:00 2001 From: Alex Passos Date: Mon, 6 Apr 2020 09:14:13 -0700 Subject: [PATCH 7/8] reference experimental section from other places --- governance/api-reviews.md | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index 563d015cb..a30e1cf3a 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -24,8 +24,8 @@ We avoid backwards-incompatible API changes. We also avoid backwards-incompatible behavior changes, such as restricting the set of valid inputs to a function or extending the set of valid outputs of a function. Adding support for previously not supported behavior is okay, as are changes to -explicitly experimental APIs (within reason). When needing to provide a new or -different behavior, we strongly prefer a new version of the API over breaking +explicitly experimental APIs (see section below). When needing to provide a new +or different behavior, we strongly prefer a new version of the API over breaking backwards compatibility. Note that we are free to deprecate APIs; we just cannot break code which relies on their documented behavior. We need to worry about backward compatibility both of our python APIs and of the serialized GraphDefs, @@ -37,15 +37,6 @@ produced by currently correct python code without a three weeks notice. This comes up most frequently when adding new ops, but also applies to non-obvious things such as the graph emitted by gradients or pfor. -Including the name “experimental” in an API endpoint allows you to break -backwards compatibility in the future, as noted above. However, we still prefer -to mark the experimental API as deprecated for one release before removing it in -the subsequent release. Please do not use the experimental namespace as an -escape hatch for bad code or functionality you -don’t-really-want-but-need-for-now; experimental APIs should be APIs that we -intend to make standard in the near future, but have some lingering questions to -resolve first. - ### Docstrings @@ -89,7 +80,7 @@ the next. We would like new experimental symbols to be things which will eventually end up in core TF as opposed to things we expect will be phased out with no clear replacement. The best expectation to have for an experimental endpoint is that the “experimental” will simply be removed. If you don’t believe -that’ll work, it should probably not be added in its current form. +that’ll work, it should probably not be added in its current form. ### Style From 8fb29b083648134c3049462bb38eb22ff78f1ec5 Mon Sep 17 00:00:00 2001 From: Alex Passos Date: Mon, 6 Apr 2020 13:18:41 -0700 Subject: [PATCH 8/8] clarify addons --- governance/api-reviews.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/governance/api-reviews.md b/governance/api-reviews.md index a30e1cf3a..78106b2c9 100644 --- a/governance/api-reviews.md +++ b/governance/api-reviews.md @@ -202,7 +202,10 @@ prefer experimental namespaces when possible, so prefer Experimental APIs are APIs intended to be added to TensorFlow as-is, but which we reserve the right to change in backwards-incompatible ways if we have -to. +to. This is different from apis in `tensorflow/addons`, many of which are not +necessarily intended to be added to core TF as they might have a more narrow use +case initially (if APIs in `tensorflow/addons` do become widely useful they can +"graduate" to core, either using experimental or not). No temporary APIs should be added to experimental (i.e. "we just need this until certain bugfix or certain new feature becomes available" is not a valid reason