-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Python to parse tensorflow v2 keras schemas (Closes #149) #180
Conversation
…nts using inspect
The tests for the schemas should actually still work as long as the schema format doesn't change. (It shouldn't need to.) |
@brollb I am requesting your review in this PR. I will commit the JSON files once other issues are resolved in this PR. |
1. Remove keras from requirements (use tensorflow.keras) 2. Fix code generation logic and test cases 3. Minor changes in layer names in test fixtures
With every test passing with increased timeouts, I think this PR is ready to be reviewed/merged. Would any changes to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a glance, it looks pretty good! I didn't quite have time for a full review but wanted to let you know my thoughts after skimming the code.
Do you know why the test timeout needed to be increased?
I will play with it tomorrow and see if I have any more comments!
environment.worker.yml
Outdated
@@ -2,7 +2,6 @@ name: deepforge-keras | |||
dependencies: | |||
- python=3.7 | |||
- pip: | |||
- tensorflow==1.14 | |||
- keras==2.2.5 | |||
- tensorflow==2.3.0 | |||
- pillow | |||
- dill |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is dill still needed? This might be able to be split out into its own PR but it is hard to say for sure since we don't want it in a broken state :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this file is being used:
https://github.com/deepforge-dev/deepforge-keras/blob/master/init_code.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -263,7 +262,7 @@ define([ | |||
return undefined; | |||
} | |||
type = arg.type; | |||
layer = this.getLayerSchema(layer.base); | |||
layer = layer.name !== layer.base ? this.getLayerSchema(layer.base) : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary? This was required before because we were parsing the source and had to reconstruct the inheritance chain to determine the type info for a given argument. Since the current version parses the argument type from the runtime, we shouldn't need to reconstruct this (and I don't know that it actually helps us at all).
After looking at it a bit more, it looks like this could be an issue with the JSONImporter as an attribute was updated to no longer be an enum. However, I am not sure if this attribute should be changed. Is |
|
I almost forgot that we will need to update initCode.py: https://www.tensorflow.org/guide/keras/save_and_serialize |
Do we want to save the model in |
Blocked by deepforge-dev/deepforge#1894 |
# Make a tmp dir | ||
import os | ||
from os import path | ||
import time | ||
tmp_dir = infile.name + '-tmp-' + str(time.time()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why hasn't this been removed, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make sure there are no conflicts in the SavedModel
directory. i.e. deserializing two models in the same directory can cause problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple minor comments about the serialization but other than that, it looks good! Nice work.
While converting the test project seed to use this branch for Sequence 2 Sequence Model (specifically when combining 2 states to one between 2 LSTMS as shown in the figure below: I got the following error:
|
Can you share the JSON representation of dimensionality/error info? |
Checklist: