flytekit icon indicating copy to clipboard operation
flytekit copied to clipboard

feat: add TypeTransformer for Tensorflow Model

Open techytushar opened this issue 2 years ago • 1 comments

TL;DR

This PR adds support for:

  1. TypeTransformer for Tensorflow Model type

Note: I was not able to run the pip-compile commands to generate the new requirements files, it failed on protobuf version error

Type

  • [ ] Bug Fix
  • [x] Feature
  • [ ] Plugin

Are all requirements met?

  • [x] Code completed
  • [x] Smoke tested
  • [x] Unit tests added
  • [x] Code documentation added
  • [ ] Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/flyteorg/flyte/issues/2570

Follow-up issue

NA

techytushar avatar Oct 18 '22 18:10 techytushar

I was not able to run the pip-compile commands to generate the new requirements files, it failed on protobuf version error

Adding grpcio-status<1.49.0 to the requirement.in can probably fix it.

pingsutw avatar Oct 18 '22 22:10 pingsutw

@pingsutw, what do you think about having a model.py rather than a model folder, or better module.py since the type transformer will be for tf.Module? It seems to make more sense to me. Since we also have a PR for tensor, we can have a tensor.py file.

samhita-alla avatar Oct 27 '22 09:10 samhita-alla

do you think about having a model.py rather than a model folder

yes. make sense. I prefer module.py

pingsutw avatar Oct 27 '22 10:10 pingsutw

Codecov Report

Merging #1241 (dadd0fb) into master (b787849) will decrease coverage by 0.09%. The diff coverage is 72.17%.

:exclamation: Current head dadd0fb differs from pull request most recent head 4a48c58. Consider uploading reports for the commit 4a48c58 to get more accurate results

@@            Coverage Diff             @@
##           master    #1241      +/-   ##
==========================================
- Coverage   68.80%   68.70%   -0.10%     
==========================================
  Files         286      292       +6     
  Lines       26363    26460      +97     
  Branches     2492     2124     -368     
==========================================
+ Hits        18138    18180      +42     
- Misses       7740     7801      +61     
+ Partials      485      479       -6     
Impacted Files Coverage Δ
flytekit/extras/tensorflow/__init__.py 0.00% <0.00%> (ø)
flytekit/extras/tensorflow/model.py 42.85% <42.85%> (ø)
...kit/unit/extras/tensorflow/test_transformations.py 93.18% <93.18%> (ø)
...ests/flytekit/unit/extras/tensorflow/test_model.py 100.00% <100.00%> (ø)
flytekit/clients/raw.py 22.29% <0.00%> (-4.21%) :arrow_down:
tests/flytekit/unit/core/test_dynamic.py 93.06% <0.00%> (-0.95%) :arrow_down:
flytekit/core/tracker.py 48.67% <0.00%> (-0.90%) :arrow_down:
flytekit/models/literals.py 40.28% <0.00%> (-0.58%) :arrow_down:
flytekit/core/promise.py 51.39% <0.00%> (-0.56%) :arrow_down:
flytekit/clis/flyte_cli/main.py 44.34% <0.00%> (ø)
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Oct 28 '22 06:10 codecov[bot]

@techytushar, let me know once the changes are in.

samhita-alla avatar Oct 28 '22 10:10 samhita-alla

@samhita-alla I have pushed the changes.

Note: I could not use the generic Struct type for storing the layer config because in protobuf format the int values gets converted to float due to which when we convert it back to the tf layer it was not able to parse it. So I have stored them as simple json string

More info here: https://stackoverflow.com/questions/51818125/how-to-use-ints-in-a-protobuf-struct

techytushar avatar Oct 28 '22 11:10 techytushar

@samhita-alla since its the last day of Hacktoberfest and all comments are resolved, please can you merge this PR or add label so that it can count towards my contribution

techytushar avatar Oct 31 '22 07:10 techytushar

@techytushar, I'll merge the PR later as I need to push some changes.

samhita-alla avatar Oct 31 '22 07:10 samhita-alla

fixes https://github.com/flyteorg/flyte/issues/2570.

We're going to need to move these changes into the tensorflow plugin @eapolinario

cosmicBboy avatar Feb 14 '23 20:02 cosmicBboy

fixes flyteorg/flyte#2570.

We're going to need to move these changes into the tensorflow plugin @eapolinario

Great, I'll make sure to incorporate those there.

eapolinario avatar Feb 14 '23 23:02 eapolinario