Making keras-contrib compatible with tf.keras #387
Conversation
|
It seems travis is not notified anymore. |
gabrieldemarmiesse
force-pushed
the
tf_keras_again
branch
4 times, most recently
from
b301941 to
1edb648
Compare
gabrieldemarmiesse
force-pushed
the
tf_keras_again
branch
from
1edb648 to
97219b5
Compare
|
@farizrahman4u could you review this PR when/if you have the time? Thanks in advance. |
CONTRIBUTING.md
Outdated
| python convert_to_tf_keras.py --revert | ||
| ``` | ||
|
|
||
| Not that you are strongly encouraged to commit your code before in case the parsing would go wrong. To discard all the changes you made since the previous commit: |
There was a problem hiding this comment.
Are we sure we want to encourage users to use the git reset --hard ? Something like git stash would be less definitive.
There was a problem hiding this comment.
Sure, the method I describe here is very brittle. This is because my knowledge of GIT is quite limited. Do you have a workflow in mind which would be cleaner and more robust?
There was a problem hiding this comment.
I would encourage to do a soft reset with git reset HEAD^ and then stash all the current untracked modifications with git stash. In this case, you can retrieve those untracked modifications with git stash pop
There was a problem hiding this comment.
Thanks for the tip!
convert_to_tf_keras.py
Outdated
| ('from keras ', 'from tensorflow.keras ')] | ||
|
|
||
|
|
||
| def replace_imports(file_path, revert): |
There was a problem hiding this comment.
Would be more prudent to add a test, to make sure the created file is as expected (like we do with generated docs)
There was a problem hiding this comment.
We already run all the tests with the generated files. Do you have a specific test in mind?
There was a problem hiding this comment.
Ho, I see what you mean. I'm not in favor of including tests at running time (except for catching user errors) because this will complexify the codebase. Separating the tests and the implementation allow for a better readability. And when tests fail, we know exactly which use case caused it. It's more extensible.
There was a problem hiding this comment.
@RaphaelMeudec do you have any simple tests in mind?
There was a problem hiding this comment.
@gabrieldemarmiesse What I had in mind is :
- Create an asset file in the test directory, and apply
replace_importsto it. Make sure that there is an output file created, and that the return value of the function is approriate - Apply in reverse the same function to make sure generated file matches the original file
There was a problem hiding this comment.
I added a test in the same file (we don't have a choice, we can't import the function directly). And added the command to run it in travis.
gabrieldemarmiesse
dismissed
farizrahman4u’s stale review
farizaman4u agreed with the current plan.
|
I seems that tensorflow.keras.backend doesn't have local_conv_1d and local_conv2d. |
gabrieldemarmiesse
changed the title
CONTRIBUTING.md
Outdated
| @@ -95,7 +95,10 @@ python convert_to_tf_keras.py --revert | |||
|
|
|||
| Note that you are strongly encouraged to commit your code before in case the parsing would go wrong. To discard all the changes you made since the previous commit: | |||
| ``` | |||
| git reset --hard | |||
| # saves a copy of your current codebase in the git stash and comes back to the previous commit | |||
| git reset && git stash | |||
There was a problem hiding this comment.
Using git stash is enough, you don't need the git reset doc
|
@gabrieldemarmiesse Final note on your PR, also I explained how I imagined the tests for the transformation. |
|
Any more comments about this? |
|
I'll merge now. I'll open an issue in the tensorflow repo about the input of the build function. |
|
Opened tensorflow/tensorflow#25790 to track the weird incompatibility. I hope it'll go away at some point. |
Making keras-contrib compatible with tf.keras (keras-team#387)
closes #376