[Relay] Port LSTM to Relay for testing

[slyubomirsky]

We would like to be able to evaluate Relay's performance on an LSTM, particularly since Relay can directly incorporate control flow. However, I could not find a concise example of an LSTM in NNVM to port over. @merrymercy pointed me to his own prior implementation of an LSTM cell, which is what I ported over.

However, in order to get this to match up with the other examples, I would also need to set up the rest of the network and load in a benchmark; I would appreciate any pointers as to how I could proceed, since I am not familiar with LSTMs. Namely, a pointer to NNVM implementations I could point over would be most helpful for being able to set up comparisons. (@tqchen, @merrymercy)

I would also appreciate any advice on how to best present this example (e.g., references to include).

Edit: Full disclosure, this variant of an LSTM is still an unrolled loop because we haven't merged in planned changes for abstract data types in Relay. Relay can currently handle a loop via recursion but without ADTs, it can't take in an arbitrary-length input list

[slyubomirsky]

I noticed in the example that the line read simply, i2h + h2h, which was the only place where a + operator was used. There were several elemwise_add calls otherwise. What was the reason for the distinction? Did the + mean to concatenate, rather than add?

[joshpoll]

I believe it is addition, not concatenation. My guess is the plus operator is used because there is no need to assign a name to gates, but there is for next_c.

[slyubomirsky]

I see, thanks

[joshpoll]

See #2010 and #2009. Weights and biases should not be exposed as arguments to this function. Instead, you should end the Python function with

args = relay.ir_pass.free_vars(foo)
return relay.Function(args, foo)

where foo is the final node in the network (i.e. what you are currently returning).

[slyubomirsky]

Duly noted, will change

[joshpoll]

Weight and bias should be left out. They will be created automatically by dense_add_bias

[slyubomirsky]

Interesting

[slyubomirsky]

Are there any other explicit type annotations I could have in this function? It would be nice to annotate the return type on the function too, but I am not sure how to state it (do you have a suggestion, @jroesch?)

[slyubomirsky]

Something gives one way or another: Type unification is inadequate (I had to annotate the types because it would not be able to conclude that you can unify tuple(tensor, tensor) and tuple(unknown, unknown) -- there should really be a visitor for unification!) and the error reporting is worse (I know there's a shape mismatch somewhere but not where)

[slyubomirsky]

I will do a substantial refactor using the ScopeBuilder to see if that might help anything

[slyubomirsky]

Annotated every type and am no longer getting type errors in Relay (this points to some serious shortcomings in type unification and inference), but now it hangs on create_workload, possibly looping infinitely. Would appreciate pointers as to what could be done differently.

[slyubomirsky]

@tqchen The default value for axis in split is zero, but the relation for split rejects an axis of 0. That doesn't seem right -- should the relation be corrected, or the default argument?

[tqchen]

Split should be able to support axis=0

[slyubomirsky]

Will change it then (very easy)

[tqchen]

@slyubomirsky can you update this PR? it is ready for another look?

[slyubomirsky]

What updates, exactly, are needed? I addressed previous feedback. I suppose I can potentially reduce the number of explicit type annotations, though, so I will investigate whether that's possible.

[tqchen]

No, I just want to make sure if you need further updates, we can merge this in as it is

[slyubomirsky]

Ah, in that case I would say it's not waiting on further changes. We can perhaps simplify the Relay implementation in a follow-up PR