flytekit
flytekit copied to clipboard
feat: add TypeTransformer for Tensorflow Model
TL;DR
This PR adds support for:
- 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
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, 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.
do you think about having a model.py rather than a model folder
yes. make sense. I prefer module.py
Codecov Report
Merging #1241 (dadd0fb) into master (b787849) will decrease coverage by
0.09%
. The diff coverage is72.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.
@techytushar, let me know once the changes are in.
@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
@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, I'll merge the PR later as I need to push some changes.
fixes https://github.com/flyteorg/flyte/issues/2570.
We're going to need to move these changes into the tensorflow plugin @eapolinario
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.