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: TensorFlow Official Model Garden Redesign #130

Open
wants to merge 4 commits into
base: master
from

Conversation

@rachellj218
Copy link
Member

commented Aug 3, 2019

This RFC will be open for comment until Friday, August 16th, 2019.

TensorFlow Official Model Garden Redesign

Status Proposed
Author(s) Jing Li (jingli@google.com), Hongkun Yu (hongkuny@google.com), Xiaodan Song (xiaodansong@google.com)
Sponsor Edd Wilder-James (ewj@google.com)
Updated 2019-08-02

Objective

This document presents a proposal to redesign TensorFlow official model garden.
We aim to provide a central and reliable place to contain popular examples,
state-of-the-art models and tutorials to demonstrate the best practice in TF2.0
and illustrate real-world use cases.

@jackwish
Copy link

left a comment

I think this proposal is really cool as many TF users are suffering the existing model zoon :) Generally, will we maintain multi-branch (or tagging) to make the code/models consistent with specific TF version? For example, having branch r2.0 or v2.0.0 tracking TF branch/tag. (I know that TF will provide API stability though...)


## Motivation

The current [TF official model garden](https://github.com/tensorflow/models/tree/master/official)

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 5, 2019

Will the new model garden still lie in https://github.com/tensorflow/models/tree/master/official or as a "root" repo? Putting such important resources in a sub directory may make (new) users confused I guess.

This comment has been minimized.

Copy link
@jayfurmanek

jayfurmanek Aug 5, 2019

I have this same question.

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

The current plan is still to keep the model garden lie in the current directory. But we are going to work with TF Hub to provide a unified UI to provide both pretrained models and links to model codes, hopefully it will make it easier for users to find. Thanks!

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 12, 2019

I am not sure what others' thoughts, but to me, model garden directory is https://github.com/tensorflow/models/tree/master, while feeling hard to understand what the official sub directory means - because in current directory hierarchy, the MobileNets are published inresearch/slim where I also take them as official...

Anyway, UI and links will be great! Thank you for reply.

| Directory | Subdirectories | | Explainations |
:-------------- |:---------------------|:--|:------------------------------ |
| modeling | | | Common modeling libraries |
| | layers | | Common modules/layers, not built-in tensorflow layers yet.|

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 5, 2019

Putting layers here is dangerous to me. By not built-in tensorflow layers yet, I assume they are layers introduced by newly published papers. If we put the layers here, and the layers were included in tensorflow later, will we rewrite the related code in this model garden? If not, this will be another slim/contrib.layers of TF 1.x. Maybe to push the implementation in TF hard?

This comment has been minimized.

Copy link
@lc0

lc0 Aug 5, 2019

how are these layers different from tf-addons

This comment has been minimized.

Copy link
@seanpmorgan

seanpmorgan Aug 7, 2019

Member

Splintering the TF ecosystem with multiple repos that contain new layers/optimizers seems counter-productive. Addons is already going to manage the burden of graduating layers into core so why have 2 repos for the same thing.

cc @karmel @facaiy for visibility

This comment has been minimized.

Copy link
@AakashKumarNain

AakashKumarNain Aug 7, 2019

With this, we are going in the direction of TF1.x again.

This comment has been minimized.

Copy link
@karmel

karmel Aug 7, 2019

Member

@rachellj218 , I believe we discussed having a process to ensure any broadly useful layers/etc make it to Addons and/or tf.text. We should update the doc to reflect that, as it seems to be a common concern-- maybe a section describing what you would like the evaluation/graduation process to be?

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

Thanks for raising this! As karmel@ mentioned, we do have the plan to graduate the common layers to tensorflow/addons. I clarified in the RFC. Thanks!

I removed optimizers/ subdirectory. I agree it should be good to add to Addons directly.

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 12, 2019

Thanks for your clarification! In the latest RFC:

Temporary folder to hold common modules/layers, not built-in tensorflow layers yet during refactoring. Broadly used layers will be graduated to tensorflow/addons and/or tf.text.

It seems to me that graduation will be another SLIM. The graduation will create different versions of one network which are based on different API, say a network uses 3 layers A, B and C firstly introduced in model garden, these 3 layers move to TF Addons one by one, thus we have four versions of network code. That is pretty confusing. And, as the interfaces of these layers are probably going to be different, it won't be as easy as re-write one line. Eventually, the effort to maintain these code will be huge.

What about implement the layers directly in TF Addons, which I think is much more flexible than the TF repo?

Thanks, hoping that my concern won't be too annoying...

This comment has been minimized.

Copy link
@seanpmorgan

seanpmorgan Aug 13, 2019

Member

I agree with this concern. IMO it's less of a headache if they are moved from the temporary folder to Addons prior to a model-garden pip release. We can promise a quick review and patch release for official model additions. To ensure this we could have a model-garden team member on addons team.

This comment has been minimized.

Copy link
@saberkun

saberkun Aug 13, 2019

Member

I think we need a consolidation stage for modeling components. This is not to develop infra but write models. We will guarantee layers moved to tf-addon must be removed in model garden. It is also up to how do you define "layers" as some of them are not common components as the ones in tf-addon now.

This comment has been minimized.

Copy link
@facaiy

facaiy Aug 21, 2019

Member

To ensure this we could have a model-garden team member on addons team.

+1, good idea. We can work in close coordination.

Maybe model garden can put its private layers in each model's module. And when you find that some layers need be shared by two models or more, I believe it's a good time to move those layers to tf.addons, rather than modeling.layers. What do you think?

| modeling | | | Common modeling libraries |
| | layers | | Common modules/layers, not built-in tensorflow layers yet.|
| | networks | | Well-known networks built on top of layers, e.g. transformer |
| | optimziers | | New or customized optimizers |

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 5, 2019

Similar to layers, I think we are creating TF dialect here.

This comment has been minimized.

Copy link
@seanpmorgan

seanpmorgan Aug 7, 2019

Member

Same concern, addons has additional optimizers that can be graduated to core.

Any optimizer/layer that isn't useful as a supplementary package can just be committed to the model garden repository IMO.

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

Removed optimizers subdirectory.


These models will have continuous convergence and performance testing to
make sure no regression. In general, we won’t deprecate these models unless:
* the model isn’t compatible with the TF APIs any more and have to be replaced by a new version

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 5, 2019

Does this imply that the model will only target latest version of TF?

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

We plan to do model garden release for major TF release, e.g. TF 2.1, 2.2. Thanks!

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 12, 2019

Cool! Thanks for your reply.

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

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

@jayfurmanek
Copy link

left a comment

This is a great idea! As it is now, it's hard to know which models work against which version of TF. Getting some focus here is good. The TF-hub integration is good as well.


We are going to reorganize the official model directory to provide:

* common libraries, mainly two types:

This comment has been minimized.

Copy link
@jayfurmanek

jayfurmanek Aug 5, 2019

Moving common code into a central location is always a good design practice, but let's be careful not to create another SLIM here. There are still models (Deeplab anyone) that are chained to SLIM.

Remember, part of what would be valuable here is showing good examples on how to write a model, not necessarily good examples on how to write a model garden with intertwined and complicated dependencies.

This comment has been minimized.

Copy link
@AakashKumarNain

AakashKumarNain Aug 5, 2019

I agree on the last part. models/research is a typical example of this. The whole garden there is so complex and huge that it is nowhere near usable for a project until unless you:

  • Just want the model off the shelf without any modification
  • You know in an out of each and every part of it

This comment has been minimized.

Copy link
@jackwish

jackwish Aug 6, 2019

Same concern. Software engineering design may not be applied to deep learning model garden these days.

Personally, a model garden is simply somewhere user can obtain the whole runnable model from a very sub directory. Sharing common library, and any similar things, sounds like a SDK built on top of TF to me. Maybe we can have TF SDK, and then the garden.

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

Thank you for raising this concern! We definitely need a good balance between implementation duplication and complicated dependencies. The current design is to split the modeling process to two stages: 1)common networks (in modeling directory), such as resnet, transformer, so that these networks can be reused in specific model/task, e.g. resnet50, mask r-cnn, sequence model; 2) specific model/task along with public dataset (e.g. in NLP and vision directory), models will be defined in its own sub directory, data preprocessing and eval and other utils for the same type of tasks/datasets can be shared.

If you have better suggestions, we are definitely open to them. Thanks again!


## Motivation

The current [TF official model garden](https://github.com/tensorflow/models/tree/master/official)

This comment has been minimized.

Copy link
@jayfurmanek

jayfurmanek Aug 5, 2019

I have this same question.

hyperparameter definition in a consistent style.
* Model category related common library, e.g. primitives as basic building
block for NLP models. We will follow the fundamental design of Keras
layer/network/model to define and utilize model building blocks.

This comment has been minimized.

Copy link
@fchollet

fchollet Aug 5, 2019

Member

Note: Network is a private class, so objects will need to subclass either Layer or Model.

Also note that whenever possible, we should prefer the pattern of Functional model + custom layers, rather than subclassed models (which have less functionality).

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

Thanks for the suggestions! We do plan to follow the pattern of functional model + custom layers.

| | | EfficientNet | |
| | | MnasNet | |
| | | ... | |
| | object_detection | | |

This comment has been minimized.

Copy link
@AakashKumarNain

AakashKumarNain Aug 5, 2019

Unet is mainly used for segmentation. So instead of object_detection, rather make two sub-directories object_detection and segmentation. segmentation will then contain:

  • DeepLab v3/v3+
  • Unet
  • FastSCNN
    ...

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

Thanks! Updated to reflect the suggestion. We are still debating the best structure for vision models, this may subject to changes in near future.

We are going to provide the pretrained models for research exploration and
real-world application development. The plan is to integrate with [TensorFlow Hub](https://www.tensorflow.org/hub),
where users can access the Hub modules and SavedModel for pretrained checkpoints and links to the code in the model
garden.

This comment has been minimized.

Copy link
@AakashKumarNain

AakashKumarNain Aug 5, 2019

Please avoid use of configs as much as possible. The code should be flexible enough to change the parameters in the code itself. Changing things though a config file makes sense for some people but not for all. It is easy to maintain the changes in the code but it is hard to track the changes in the config file and at the same time, it is hard to validate the changes as well

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

Thanks for your suggestion! We plan to follow the practice in cloud tpu model repo, for example, https://github.com/tensorflow/tpu/blob/master/models/official/mnasnet/mnasnet_main.py#L674. A default parameter dictionary will be provided, but users can overwrite the default value or extend to new key/value with string, dict or yaml.

| Directory | Subdirectories | | Explainations |
:-------------- |:---------------------|:--|:------------------------------ |
| modeling | | | Common modeling libraries |
| | layers | | Common modules/layers, not built-in tensorflow layers yet.|

This comment has been minimized.

Copy link
@lc0

lc0 Aug 5, 2019

how are these layers different from tf-addons

| modeling | | | Common modeling libraries |
| | layers | | Common modules/layers, not built-in tensorflow layers yet.|
| | networks | | Well-known networks built on top of layers, e.g. transformer |
| | optimziers | | New or customized optimizers |

This comment has been minimized.

Copy link
@lc0

lc0 Aug 5, 2019

Suggested change
| | optimziers | | New or customized optimizers |
| | optimizers | | New or customized optimizers |

This comment has been minimized.

This comment has been minimized.

Copy link
@rachellj218

rachellj218 Aug 10, 2019

Author Member

Thanks! We do have the plan to graduate the common layers to tensorflow/addons. I clarified in the RFC. Thanks!

optimizers/ subdirectory was removed from RFC. I agree it should be good to add to Addons directly.

rachellj218 added some commits Aug 10, 2019

@martinwicke

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.