[frontend][keras] Add support for TimeDistributed#7006
Conversation
|
Thanks, Jared! |
| ) | ||
|
|
||
| conversion_func = lambda expr: _convert_map[inner_layer_op_name]( | ||
| expr, inner_layer, etab, input_shape=inner_input_shape, data_layout=inner_data_layout |
There was a problem hiding this comment.
it seems like you could have just make a copy of etab but set a new 'input_shape' and 'data_layout' here. This would remove the need to change every other convert function. Am I missing a reason that wouldnt work? It's unclear to me what other assumptions are being made about etab.
There was a problem hiding this comment.
That would work for data_layout. input_shape is a field in the Keras layer and is not mutable, unfortunately
There was a problem hiding this comment.
Another problem is that the etab is used in the end to produce the new Relay module. Copying the etab would be okay if I move all the definitions over to the original one and make sure there's no chance of a name conflict (that is probably doable).
There was a problem hiding this comment.
https://www.tensorflow.org/api_docs/python/tf/keras/models/clone_model I might be able to copy the inner layer to TimeDistributed and set its input_shape but I'm not sure whether it's possible. I will check
There was a problem hiding this comment.
After reading over the ExprTable implementation, I do not think it was a good idea to attach the data layout (a property that has nothing to do with the ExprTable specifically) to the etab in the first place -- it seems like unnecessary coupling. Maybe there should be a different way of handling the shared state across these conversions.
Fwiw, I could just overwrite the data_layout field in the ExprTable when handling the TimeDistributed case and avoid having to modify the other conversion functions. I am not sure whether that would be a good idea from the standpoint of maintainability, though.
There was a problem hiding this comment.
Ok that seems reasonable. I think its fine to remove data_layout from etab and use explicit inputs.
|
The test failure seems spurious (in an operator test file I didn't modify). |
|
This is a flakey test we've been seeing recently. Push a new commit to force the ci to rerun. |
slyubomirsky
force-pushed
the
keras-time-distributed
branch
from
674c241 to
80c4dbb
Compare
|
Should be up to date, thanks for the advice |
|
@jroesch @slyubomirsky @jwfromm please followup on this PR |
|
@slyubomirsky can you resolve the conflicts this PR has with the current main branch? Once you do I think we can merge this as it LGTM. |
|
Oh I'm terribly sorry, I got side tracked and haven't seen these updates. I will make the requested changes |
slyubomirsky
force-pushed
the
keras-time-distributed
branch
from
f68e57d to
958288e
Compare
|
@jwfromm Rebased, terribly sorry about the long delay |
|
@jwfromm @mbrookhart can you guys try to land this? |
jroesch
added
the
needs-triage
slyubomirsky
force-pushed
the
keras-time-distributed
branch
from
958288e to
a50848f
Compare
slyubomirsky
requested review from
Huyuwei,
areusch,
comaniac,
icemelon,
jroesch,
junrushao,
kazum,
mbrookhart,
merrymercy,
siju-samuel,
srkreddy1238,
tqchen and
yzhliu
as code owners
* First pass on modifying Keras importer to handle TimeDistributed * Use squeeze inside TimeDistributed, add tests * linter fixes * More linting * Even more linting * Fix unused argument annotations * Forgot one pylint annotation * Forgot to set up data layout in _convert_activation * Decouple data_layout from etab * Linting fix * Forgot to set data_layout argument * Missed an etab.data_format, also test_conv1d was not in the test file's main * Rebase fixes * Linting fix * _convert_lambda needs a data layout argument too * linting fix too * Lint the test file too * Redundant variables * Simplify further * Another simplification Co-authored-by: Steven Lyubomirsky <slyubomirsky@octoml.ai>
* First pass on modifying Keras importer to handle TimeDistributed * Use squeeze inside TimeDistributed, add tests * linter fixes * More linting * Even more linting * Fix unused argument annotations * Forgot one pylint annotation * Forgot to set up data layout in _convert_activation * Decouple data_layout from etab * Linting fix * Forgot to set data_layout argument * Missed an etab.data_format, also test_conv1d was not in the test file's main * Rebase fixes * Linting fix * _convert_lambda needs a data layout argument too * linting fix too * Lint the test file too * Redundant variables * Simplify further * Another simplification Co-authored-by: Steven Lyubomirsky <slyubomirsky@octoml.ai>
6 participants
This PR is an attempt to support the
TimeDistributedlayer in the Keras frontend. BecauseTimeDistributedtakes a layer as an argument, I had to modify the existing conversion functions to take parameters instead of making assumptions about the Keras model oretabdata structure. Perhaps it might be better not to attempt to reuse the existing conversion functions and special-case the different possible arguments toTimeDistributed-- I would be glad to take any suggestions on this.I would also be interested to know what you might advise as to further test cases for
TimeDistributed; I went by the example from the documentation and another use-case I found that usedTimeDistributedwithDense.@jroesch I am not entirely sure who would be the best person to review a change to the Keras frontend and would appreciate any advice