Skip to content

SIG Addons: Procedure and RFC template for Addons migrations #241

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions sigs/addons/MIGRATION_RFC_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Migrate XXXXX from TensorFlow Addons to TensorFlow Core

| Status | Proposed (Waiting for Sponsor) |
| :---------- | :------------------------------------------------------------------------------------------------- |
| **RFC #** | TBD after PR | |
| **Authors** | XXXXXXXXXX |
| **Sponsor** | XXXXXXXXXX |
| **Updated** | YYYY-MM-DD |
| **Sponsorship Deadline** | YYYY-MM-DD (45 Days after submission) |

## Rationale for Migration
* What are the use cases for the addon?
* OSS usage, H5 Index, etc.

## Historical Information
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how it is easy to have a quick query but for stability we could ask how many bugs was handled in the last (x) month(s).

* Have there been signifiant issues reported to Addons that need to be adressed?
* When was it implemented in Addons?

## Implementation Details
* Link to implementation in Addons:
* Does this include custom-op kernels?
* Are they CPU/GPU/TPU compatible?
* What is the pytest coverage of the addon?

## Changes to Implementation (If Needed)
```
Code snippet(s) showing proposed code changes
```
* Discussion on the rationale for changes

## Transition Plan
* Proposed landing place in tf-core
* Will there be any changes to the API? (e.g. additional parameters or renaming)
* Deprecation plan for Addons

## Relevant GitHub Issues

## Questions and Discussion Topics

## Final Decision
TBD
33 changes: 33 additions & 0 deletions sigs/addons/MIGRATION_TO_CORE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Migration From TF-Addons To TensorFlow Core

### In-Progress & Previous Migrations:
https://github.com/tensorflow/addons/projects/2/

### Process
Copy link
Contributor

@bhack bhack May 5, 2020

Choose a reason for hiding this comment

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

I think this could be quite complicated for cases like ImageProjectiveTransformV2.
As we could have an upstream request where the sponsor is already in the TF core team I don't think this double pass is plausible for this specific case.

EDIT:
Here I meant that if there upstream proposer and the sponsor overlaps (in the case the promotion activity it is started by a TF team member) I don't think that we will like to have this double pass evaluation (on the SIG side and then here with the RFC). So I suppose that we want to define a fast track process in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be helping external contribution for the migration only, not internal contribution for the migration

Copy link
Contributor

Choose a reason for hiding this comment

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

@tanzhenyu Do you mean that you want a different process for Google internals?

1. Create an issue in TensorFlow Addons for a candidate that you think should be
migrated.
2. The SIG will evaluate the request and add it to the `Potential Candidates` section
of our GitHub project.
3. If it's agreed that a migration makes sense, an RFC needs to be written to discuss
the move with a larger community audience.
* If the transition will impact tf-core and Keras then submit the RFC to
[TensorFlow Community](https://github.com/tensorflow/community)
* Additions which only subclass Keras APIs should submit their migration proposals to
[Keras Governance](https://github.com/keras-team/governance)
Copy link
Contributor

Choose a reason for hiding this comment

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

The new Keras website is online. I don't know if we need to duplicate/fork this template also in Keras governance and toadd something to the Keras SIG new webpage.
We are in the special case here where a SIG is an upstream of another SIG.

Copy link
Contributor

Choose a reason for hiding this comment

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


4. A sponsor from the TF/Keras team must agree to shepard the transition and maintain
the new API in tensorflow/tensorflow.
* If no sponsor is obtained after 45 days the RFC will be rejected and will remain
Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok to put a number here but I think that we need a realistic evaluation by the TF team. Is 45 days something realistic for the internal process?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, would like to align here but 45 days ensures we overlap with a TFA monthly meeting and lets us get help from TF internal team members to help find sponsors. @theadactyl @ewilderj is there anyway we could make 45 days the standard amount of time for RFC sponsorship? I imagine this would be useful for multiple SIGs that have monthly meetings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is the same if we could extend the standard to 45 days is ok

as part of Addons.
5. If a sponsor is obtained, and the RFC is approved, a pull request must move the
addon along with proper tests.
6. After merging, the addition will be replaced with an alias to the core function
if possible. If an alias is not possible (e.g. large parameter changes), then a deprecation
warning will be added and will be removed from TFA after 2 releases.


### Criteria for Migration RFC
* The addition is widely used throughout the community, or has high academic significance.
* Metrics must be reported in the RFC (OSS usage, H5 index, etc.)
* The addition is unlikely to have API changes as time progresses
* The addition is well written / tested