Skip to content
This repository has been archived by the owner on Aug 9, 2024. It is now read-only.

keras-cv Scoping RFC. #23

Merged
merged 4 commits into from
Sep 15, 2020
Merged

keras-cv Scoping RFC. #23

merged 4 commits into from
Sep 15, 2020

Conversation

tanzhenyu
Copy link
Contributor

No description provided.

Copy link

@seanpmorgan seanpmorgan left a comment

Choose a reason for hiding this comment

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

Similar questions and thoughts as in #22

## Boundaries between keras-cv and Tensorflow Addons

- Highly experimental modeling, layers, losses, etc, live in addons.
- Components from addons will graduate to Model Garden, given it incurs more usage,

Choose a reason for hiding this comment

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

Similar question regarding Addons graduation as in the NLP RFC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same response as NLP scoping doc.


## Dependencies

- Tensorflow version >= 2.4

Choose a reason for hiding this comment

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

Could we elaborate on why 2.4 is the minimum version to utilize this library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same response as NLP scoping doc.

Specifically, for Object Detection tasks, `keras-cv` will include most anchor-based modules:

- Common objects such as anchor generator, box matcher.
- Keras layer components such as ROI generator, NMS postprocessor.
Copy link

@seanpmorgan seanpmorgan Sep 1, 2020

Choose a reason for hiding this comment

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

Similar question about custom op kernels as in the NLP RFC. NMS postprocessor has a few custom op implementations in TF core I believe.. but there may be new custom-ops needed in a CV repo like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NMS is mostly supported (through 5 different versions). I agree there maybe new custom-ops needed, I believe one thing we haven't mentioned that @bhack asked is what things should go to keras-cv and what goes to tf core. In this specific case, graduating tfa custom ops to tf core, not keras, seems a better option IMO.

Copy link
Contributor

@bhack bhack Sep 1, 2020

Choose a reason for hiding this comment

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

Main question probably alteady done in another ticket: are these repositories going to host c++ code?
Cause "Keras world" was historically python only.
More in genetsl see also our old thread about custom ops at tensorflow/addons#1752 (comment)

Copy link
Contributor

@bhack bhack Sep 1, 2020

Choose a reason for hiding this comment

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

I am asking this also by an inference point of view. As we could see from the Google mediapipe experience about putting end2end TF model "in production" on different platforms (e.g. also on devices where the python interpreter is not available) it still require many c++ calculators with impl sometime depending on external c++ library like OpenCV etc...
Just to make an example see c++ calculators code in the image folder: https://github.com/google/mediapipe/tree/master/mediapipe/calculators/image

Copy link
Contributor Author

@tanzhenyu tanzhenyu Sep 1, 2020

Choose a reason for hiding this comment

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

I think the philosophy here is, we cannot guarantee everything has a tf op. This is not scalable, and not aligned with our future endeavor such as MLIR as well. Instead we should allow compiler to interpret them and break down into simpler ops.
But this is a good point, specifically regarding OpenCV, at least they can be executed wrapped in tf.numpy_function. For training this is good enough (and part of data preprocessing, so if you don't have accelerate support, it's fine). For serving, I can imagine people have their own solutions for optimization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Meanwhile will this discussion alternate the proposal?

Copy link
Contributor

Choose a reason for hiding this comment

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

It Is hard to say cause I am not in the position to allocate TF teams resources in a specific direction. 😉 But It would be nice if you will find, as a team, an internal consesus and resources to bootstrap an MVP on this (if It really makes any sense to create a CV dialect)

Copy link
Contributor

@bhack bhack Sep 2, 2020

Choose a reason for hiding this comment

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

Also as you know by some concrete example we had on different tickets coordination It Is hard in the Wild with a classical approach.
E.g. Just to mention something old and fresh at the same time tensorflow/addons#914 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically related to this package being pure python? (keras_cv is on top of Keras IMO)

Copy link
Contributor

@bhack bhack Sep 2, 2020

Choose a reason for hiding this comment

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

Related to your mentioned NMS postprocessor and iter future candidate operators like this one. Being python only doesn't solve the runtime topic about trasformation and compilers.
Where keras-cv could run? Can run only on targets with python interpreter.
IMHO It Is a Little bit like when we used PIL preprocessing in Keras instead of rely on TF ops like in the new preprocessing. It Is not really the case of NMS cause we are now at v3 impl in Tensorflow. I picked this just cause you mentioned as an example.

EDIT:
In the TF (MLIR) dialect NMS is at v5 https://www.tensorflow.org/mlir/tf_ops?hl=en#tfnonmaxsuppressionv5_tfnonmaxsuppressionv5op

Comment on lines 78 to 86
- Components from addons will graduate to Model Garden, given it incurs more usage,
and it works in CPU/GPU/TPU. The API interface will remain experimental after graduation.

## Boundaries between keras-cv and Model Garden

- End to end modeling workflow and model specific details live in Model Garden
- Model garden will re-use most of the building blocks from keras-cv
- Components from Model Garden can graduate to keras-cv, given it is widely accepted,
it works performant in CPU/GPU/TPU. The API interface should remain stable after graduation.

Choose a reason for hiding this comment

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

@tanzhenyu per discussion at the SIG Addons meeting could we update this to say modular components should not be housed in model garden, and that with sufficient usage/reliability Addons components will be graduated to keras/tf-core as needed/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@tanzhenyu tanzhenyu merged commit 827f176 into keras-team:master Sep 15, 2020
@bhack
Copy link
Contributor

bhack commented Sep 17, 2020

@tanzhenyu Can you explain why we have keras-cv code in its own repository and in model garden:
https://github.com/tensorflow/models/tree/master/official/vision/keras_cv
https://github.com/keras-team/keras-cv/tree/master/keras_cv

@tanzhenyu
Copy link
Contributor Author

@bhack
Copy link
Contributor

bhack commented Sep 17, 2020

So if I translate correctly It Is duplicated untill this repository will have Google CI infrastrutture. Right?

@tanzhenyu
Copy link
Contributor Author

So if I translate correctly It Is duplicated untill this repository will have Google CI infrastrutture. Right?

Yes + when we finalize the API-level RFC

@bhack
Copy link
Contributor

bhack commented Sep 17, 2020

Is that why, in the original version of the RFC, you proposed to upstream Addons in Model Garden?
I suppose that in readonly mode it is impossible to upstream on this repository directly.
So is this commit still means that we upstream to model garden?
Cause if we have to wait also for API-level RFC stabilization other then CI infrastructure I suppose that it will be a not so much a short term temporary phase.

@tanzhenyu
Copy link
Contributor Author

Is that why, in the original version of the RFC, you proposed to upstream Addons in Model Garden?
I suppose that in readonly mode it is impossible to upstream on this repository directly.
So is this commit still means that we upstream to model garden?
Cause if we have to wait also for API-level RFC stabilization other then CI infrastructure I suppose that it will be a not so much a short term temporary phase.

We need both: 1) API-level RFC, 2) CI infrastructure. The readonly repo is simply for those that want to read the code but don't want the trouble to finding that through Model Garden, and a general impression that this will soon be the only repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants