kevinthesun edited a comment on pull request #6449: URL: https://github.com/apache/incubator-tvm/pull/6449#issuecomment-692917238
> @zhiics @kevinthesun @masahi > Thank you, @kevinthesun for your summary and all the work in the investigation and your PR. > > I think using `if isinstance(..., _expr.Expr):` would be very much preferable to using exceptions. > > 1. I see the uses of `try: ... except: ...` as using exceptions for regular control flow (because the error case is the one where the old normal logic is applicable and so we should have a clear view when the new logic is applicable and when not) and it would then except on the old cases, > 2. Not using `try: ... except: ...` for regular control flow seems like good programming fundamentals to me. It would seem odd if TVM as a compiler stack would not strive to follow best practice here. > > Neither am I entirely sure whether 1. is contentious or not and to me it would seem that a PR is an odd place to form an opinion on 2. At the same time I see the construct as problematic enough to have a really hard time liking the current state of the PR. It would bring great joy if you could be convinced to move it to `if`. > > I should emphasize that I'm entirely for having the new functions appreciate your @kevinthesun work on this. Thank you! @t-vi Thanks for your thoughts. To handle dynamic op correctly, we have to use ```if isinstance(..., _expr.Expr)``` together with ```try ... except```. Do we have an agreement that in the dynamic ops involved in this PR, ```try ... except``` is necessary? Currently there is no other way rather than try _infer_value to get constant attribute. Unfortunately this is not about good programming fundamentals but about functionality, since regular control flow won't do the trick. You can see the similar methodology in TensorFlow frontend. We need to do these because tf/pt od models are in the most complicated models which TVM has ever tried to compiled. As @masahi mentioned, we can gradually move these complicated dynamic inference logics to backend while dyn namespace is improved. However, currently these are something necessary to support OD models. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
