Closed Minnozz closed 1 year ago
With my very limited understanding of TensorFlow, I suspect that the old call to TF_ExtendGraph
merges the supplied GraphDef
with the current Session, while the new call to TF_GraphImportGraphDef
does not consider the existing graph and therefore cannot resolve references to existing nodes.
This is probably where the errors are coming from: https://github.com/tensorflow/tensorflow/blob/fdfc646704c37bdf450525f6ced9d80df86e4993/tensorflow/core/common_runtime/graph_constructor.cc#L726-L736
The "old" ExtendGraph call ended up here: https://github.com/tensorflow/tensorflow/blob/fdfc646704c37bdf450525f6ced9d80df86e4993/tensorflow/core/common_runtime/graph_execution_state.cc#L181-L286
I will try using TF_NewOperation calls instead of using a GraphDef as intermediate.
That might be a lot of work. If I remember correctly, a lot of the haskell code was designed exploiting the fact that we could build up a graph of protos (without having to interact with the C API or do anything "impure") and then just chuck it all into tensorflow at once via TF_ExtendGraph
, so a lot of refactoring might be necessary (I'd be happy to be wrong though).
Not sure if it is feasible, but maybe at the point where TF_ExtendGraph
is currently called, instead we could traverse the GraphDef
proto in haskell, translating its contents into calls to TF_NewOperation
and friends. Of course it seems like a waste to build up the GraphDef
and then only use it for that, but it might be the shortest path to a solution.
Thanks for the feedback; I'll give it a try to see if it's feasible.
I'm currently using the NodeDef
protos in the Builder
before they are put into the GraphDef
proto, but I think that is equivalent to your suggestion.
It is indeed a lot of work. I will try using the TF_GraphImportGraphDef approach again, but this time with an input mapping for existing nodes in the graph.
Passing an input map seems to work.
@fkm3 Thanks for looking into this PR; can you tell something about the CI failure?
It is just some warnings-as-errors, you should be able to reproduce using the --pedantic
flag of stack or ghc (or use the CI docker script).
tensorflow > [ 1 of 12] Compiling TensorFlow.Internal.Raw
tensorflow >
tensorflow > /[tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs:86](https://cs.corp.google.com/piper///depot/google3/tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs?l=86):29: error: [-Wname-shadowing, -Werror=name-shadowing]
tensorflow > This binding for ‘ptr’ shadows the existing binding
tensorflow > bound at [src/TensorFlow/Internal/Raw.chs:86](https://cs.corp.google.com/piper///depot/google3/src/TensorFlow/Internal/Raw.chs?l=86):10
tensorflow > |
tensorflow > 86 | peek ptr = Output <$> {# get TF_Output->oper #} ptr
tensorflow > | ^^^
tensorflow >
tensorflow > /[tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs:87](https://cs.corp.google.com/piper///depot/google3/tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs?l=87):47: error: [-Wname-shadowing, -Werror=name-shadowing]
tensorflow > This binding for ‘ptr’ shadows the existing binding
tensorflow > bound at [src/TensorFlow/Internal/Raw.chs:86](https://cs.corp.google.com/piper///depot/google3/src/TensorFlow/Internal/Raw.chs?l=86):10
tensorflow > |
tensorflow > 87 | <*> (fromIntegral <$> {# get TF_Output->index #} ptr)
tensorflow > | ^^^
tensorflow >
tensorflow > /[tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs:89](https://cs.corp.google.com/piper///depot/google3/tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs?l=89):11: error: [-Wname-shadowing, -Werror=name-shadowing]
tensorflow > This binding for ‘ptr’ shadows the existing binding
tensorflow > bound at [src/TensorFlow/Internal/Raw.chs:88](https://cs.corp.google.com/piper///depot/google3/src/TensorFlow/Internal/Raw.chs?l=88):10
tensorflow > |
tensorflow > 89 | {# set TF_Output->oper #} ptr oper
tensorflow > | ^^^
tensorflow >
tensorflow > /[tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs:90](https://cs.corp.google.com/piper///depot/google3/tfhs/tensorflow/src/TensorFlow/Internal/Raw.chs?l=90):11: error: [-Wname-shadowing, -Werror=name-shadowing]
tensorflow > This binding for ‘ptr’ shadows the existing binding
tensorflow > bound at [src/TensorFlow/Internal/Raw.chs:88](https://cs.corp.google.com/piper///depot/google3/src/TensorFlow/Internal/Raw.chs?l=88):10
tensorflow > |
tensorflow > 90 | {# set TF_Output->index #} ptr $ fromIntegral index
tensorflow > | ^^^
Oops, thought I checked that. Fixed now.
This is a prerequisite for supporting SavedModel.