[Relay] Conv2d grad

[vinx13]

No description provided.

[MarisaKirisame]

How is the numerical gradient going?

[vinx13]

@MarisaKirisame I disabled numerical tests for now

[MarisaKirisame]

dont stress about it. It might be an error with numerical test.

[SWu]

I tried playing around with this PR but I'm getting:

  File "/usr/tvm/python/tvm/relay/op/_tensor_grad.py", line 257, in conv2d_grad
    data_shape = get_const_tuple(data.checked_type.shape)
  File "/usr/tvm/python/tvm/relay/expr.py", line 47, in checked_type
    raise ValueError("The type checker has not populated"
ValueError: The type checker has not populated the checked_type for this node

Is there something I need to call to populate checked_type?

[vinx13]

@SWu You need to call InferType before calling gradient pass

[vinx13]

@MarisaKirisame please review this change

[SWu]

@vinx13 I am calling InferType pass on the function before gradient pass, but it doesn't seem to make a difference (for what it's worth, I'm using mode="first_order" for gradient pass since it seems there are still missing pieces in the relay VM executor for running functions generated by higher order gradient pass). If there's something non-obvious happening here we can discuss elsewhere; I was just curious since it doesn't seem like any other gradients so far require statically knowing the data shapes before calling gradient pass.

[vinx13]

@SWu I have updated the gradient pass here, that will copy the checked type before calling the registered gradient function. Have you tried the test case here? If your code is different from the test case, can you share your script and I can take a look?

[SWu]

@vinx13 I think I narrowed it down mode="first_order" when calling relay.transform.gradient(). It seems to work otherwise (using debug executor). I'm not sure if that's a bug in the first_order (as opposed to higher order) gradient transform or somewhere else though.

[vinx13]

@MarisaKirisame what do you think is the best way to set the type of orig in https://github.com/dmlc/tvm/blob/aee16d87152b4737445e0c8784e9eb7cbc0134c8/src/relay/pass/gradient.cc#L149

[MarisaKirisame]

@vinx13 It is indeed the best way. I am pretty ok with it. @jroesch is this ok with you?

[vinx13]

@MarisaKirisame The problem is we don't have access to the original call in this function (this is VisitExpr_(const OpNode*)

[MarisaKirisame]

@vinx13 it is easily solved by requiring the lambda return to take an extra argument. type.
Another thing is the first_order mode shouldnt be used - it support less feature then the higher order mode (even in first order case!) For example, it dont has tuple support as operator call/return/whole function return. I am thinking of deprecating it.

[junrushao]

If the supporting infra for non first order is not quite ready yet, let's keep first order for now.

[junrushao]

Also CC @zhiics if you have time

[zhiics]

@junrushao1994 Sure. Will do it today.

[zhiics]

I am not sure if assigning the type directly is the best way because my impression is that the type should be retained somehow.

Otherwise, LGTM.

[junrushao]

I don't object to assigning checked_type directly if there is no other way to do it.

[MarisaKirisame]

@jroesch I want your input with changing the type directly.

[MarisaKirisame]

@junrushao1994 @vinx13 can you guys wait a bit before getting this merged? I am a bit hesitant about writing the type directly and want to think about it a bit.

[junrushao]

@MarisaKirisame not a hurry, we are hesitant as well

[junrushao]

Is there any update?

[MarisaKirisame]

@junrushao1994 I will talk with jared today.

[MarisaKirisame]

@junrushao1994 it is ok.

[zhiics]

Okay, I think we can now merge. @vinx13 Should we rebase to retrigger the CI since this PR has been here for a while?