Skip to content

feat: add TypeTransformer for Tensorflow Model#1241

Closed
techytushar wants to merge 12 commits into
flyteorg:masterfrom
techytushar:tf-type-transformer
Closed

feat: add TypeTransformer for Tensorflow Model#1241
techytushar wants to merge 12 commits into
flyteorg:masterfrom
techytushar:tf-type-transformer

Conversation

@techytushar

Copy link
Copy Markdown
Contributor

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
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • 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

flyteorg/flyte#2570

Follow-up issue

NA

@techytushar techytushar force-pushed the tf-type-transformer branch 2 times, most recently from 4ec3f78 to 42d3362 Compare October 18, 2022 18:29
Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
@pingsutw

Copy link
Copy Markdown
Member

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.

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
…-type-transformer

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>

@pingsutw pingsutw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we move these files to flytekit/extras/tensorflow/model?

Comment thread flytekit/extras/tensorflow/__init__.py
…-type-transformer

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>

@pingsutw pingsutw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two tests are failing. ModuleNotFoundError: No module named 'tensorflow'
You have to run make dev-requirements.txt to update the requirements in dev-requirements.txt, and then GA will be able to install TensorFlow.

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
raise ValueError(f"Transformer {self} cannot reverse {literal_type}")


class TensorflowModelTransformer(TensorflowTypeTransformer[tf.keras.Model]):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@techytushar, tf.Module is the parent class for tf.keras.Model and tf.keras.layers.Layer. Can you use that here? Also, can you merge this class into the parent class cause I don't think we need to use Generic in the first place if there's only one override?

@samhita-alla

Copy link
Copy Markdown
Contributor

@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.

Comment on lines +16 to +18
@task
def get_model_layers(model: tf.keras.Model) -> List[tf.keras.layers.Layer]:
return model.layers

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test shouldn't use your type transformer; it should just pickle. So it doesn't make sense to have unless you add a type transformer for tf.Module.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, I'm unsure if List[...] will work. tf.keras.layers.Layer should, however, work after you add tf.Module.

Comment thread tests/flytekit/unit/extras/tensorflow/test_native.py Outdated
@pingsutw

pingsutw commented Oct 27, 2022

Copy link
Copy Markdown
Member

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

yes. make sense. I prefer module.py

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
Comment thread flytekit/extras/tensorflow/__init__.py Outdated
Comment thread flytekit/extras/tensorflow/__init__.py Outdated
Comment thread flytekit/extras/tensorflow/__init__.py Outdated
Comment thread flytekit/extras/tensorflow/model.py Outdated
@codecov

codecov Bot commented Oct 28, 2022

Copy link
Copy Markdown

Codecov Report

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

❗ 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%) ⬇️
tests/flytekit/unit/core/test_dynamic.py 93.06% <0.00%> (-0.95%) ⬇️
flytekit/core/tracker.py 48.67% <0.00%> (-0.90%) ⬇️
flytekit/models/literals.py 40.28% <0.00%> (-0.58%) ⬇️
flytekit/core/promise.py 51.39% <0.00%> (-0.56%) ⬇️
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.

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
@samhita-alla

Copy link
Copy Markdown
Contributor

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

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
@techytushar

Copy link
Copy Markdown
Contributor Author

@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

Comment thread flytekit/extras/tensorflow/__init__.py Outdated
Comment thread flytekit/extras/tensorflow/__init__.py Outdated
Comment thread flytekit/extras/tensorflow/__init__.py Outdated
Comment thread flytekit/extras/tensorflow/layer.py Outdated
Comment thread flytekit/extras/tensorflow/layer.py Outdated
Comment thread flytekit/extras/tensorflow/layer.py Outdated
Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
Comment thread flytekit/extras/tensorflow/model.py Outdated
Comment thread flytekit/extras/tensorflow/model.py Outdated
Comment thread tests/flytekit/unit/extras/tensorflow/test_layer.py Outdated
Comment thread tests/flytekit/unit/extras/tensorflow/test_layer.py
Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
…-type-transformer

Signed-off-by: Tushar Mittal <chiragmittal.mittal@gmail.com>
@techytushar

techytushar commented Oct 31, 2022

Copy link
Copy Markdown
Contributor Author

@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

@samhita-alla

Copy link
Copy Markdown
Contributor

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

@cosmicBboy

Copy link
Copy Markdown
Contributor

fixes flyteorg/flyte#2570.

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

@eapolinario

Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants