TypeTransformer for Keras#1242
Closed
ryankarlos wants to merge 6 commits into
Closed
Conversation
9bb6a88 to
1a99a2f
Compare
8 tasks
Signed-off-by: Ryan Nazareth <ryankarlos@gmail.com>
Signed-off-by: Ryan Nazareth <ryankarlos@gmail.com>
Signed-off-by: Ryan Nazareth <ryankarlos@gmail.com>
1a99a2f to
3216e8b
Compare
ryankarlos
commented
Oct 20, 2022
| google-cloud-bigquery-storage | ||
| IPython | ||
| torch | ||
| tensorflow<=2.8.1 |
Contributor
Author
There was a problem hiding this comment.
see my comment in #1240 (comment) for reasons for pinning tf versions.
Contributor
Author
There was a problem hiding this comment.
Ignore this - ive pinned grpcio-status<1.49.0 instead based on suggestion from @pingsutw in another PR, which fixed it !
Signed-off-by: Ryan Nazareth <ryankarlos@gmail.com>
pingsutw
reviewed
Oct 20, 2022
pingsutw
left a comment
Member
There was a problem hiding this comment.
There is an error in test_to_python_value_and_literal
def test_to_python_value_and_literal(transformer, python_type, format, python_val):
ctx = context_manager.FlyteContext.current_context()
tf = transformer
lt = tf.get_literal_type(python_type)
lv = tf.to_literal(ctx, python_val, type(python_val), lt) # type: ignore
assert lv.scalar.blob.metadata == BlobMetadata(
type=BlobType(
format=format,
dimensionality=BlobType.BlobDimensionality.SINGLE,
)
)
assert lv.scalar.blob.uri is not None
output = tf.to_python_value(ctx, lv, python_type)
if isinstance(python_val, keras.Sequential):
for p1, p2 in zip(output.weights, python_val.weights):
np.testing.assert_array_equal(p1.numpy(), p2.numpy())
assert True
else:
> assert isinstance(output, dict)
E assert False
E + where False = isinstance(<keras.engine.functional.Functional object at 0x000001AE37320108>, dict)
Contributor
Author
Ahh yes, forgot to account for keras.Model in the check, after the refactor ....... just pushed fix now |
Codecov Report
@@ Coverage Diff @@
## master #1242 +/- ##
==========================================
+ Coverage 68.68% 68.71% +0.02%
==========================================
Files 288 292 +4
Lines 26333 26482 +149
Branches 2486 2494 +8
==========================================
+ Hits 18087 18196 +109
- Misses 7768 7805 +37
- Partials 478 481 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Ryan Nazareth ryankarlos@gmail.com
TL;DR
Adds a typetransformer to support
keras.Modelandkeras.Sequentialas native typesType
Are all requirements met?
Complete description
__init__.pyfile handles case where the user doesn't have keras installed.Tracking Issue
flyteorg/flyte#2759
Follow-up issue
N/A