[Relay, Topi, TF Frontend] Isfinite operator #4981

maheshambule · 2020-03-03T17:50:27Z

Added support for isfinite operator.

@yongwww, @zhiics, @kevinthesun, @FrozenGene
Could you please review the PR?

maheshambule · 2020-03-11T09:11:55Z

Please help in reviewing this: @jwfromm, @huajsj, @yzhliu, @zxy844288792

jwfromm · 2020-03-14T01:37:46Z

tests/python/frontend/tensorflow/test_forward.py

@@ -38,6 +38,8 @@
 from tvm import te
 from tvm import relay
 import tvm.relay.testing.tf as tf_testing
+from tensorflow.python.framework import dtypes


We don't need to import this, just use tf.float32 and tf.float64 instead.

jwfromm · 2020-03-14T01:42:56Z

include/tvm/tir/op.h

@@ -58,6 +58,13 @@ TVM_DLL PrimExpr max_value(const DataType& dtype);
 */
 TVM_DLL PrimExpr min_value(const DataType& dtype);

+/*!
+ * Get the infinity.


"Get the value of infinity."

jwfromm · 2020-03-14T01:44:21Z

include/tvm/tir/expr.h

@@ -829,6 +829,7 @@ class CallNode : public PrimExprNode {
  static constexpr const char* glsl_texture_store = "glsl_texture_store";
  static constexpr const char* prefetch = "prefetch";
  static constexpr const char* isnan = "isnan";


Is it worth adding an isinf operator to this PR? That way we can check for both halves of isfinite separately if needed. I'm not sure if you'd ever need to check for just infinity rather than both nan and infinity though.

Added support for isinf as well.

maheshambule · 2020-03-17T11:45:44Z

@jwfromm Thanks for the review and comments. I have added support for isinf op as well.

jwfromm

Thanks for the changes! LGTM

maheshambule · 2020-03-18T20:12:11Z

@jwfromm could you help in merging this?

jwfromm · 2020-03-19T19:22:48Z

@masahi, @tqchen, @FrozenGene, I think this PR is ready to merge. Can one of you take a quick look and pull the trigger?

masahi · 2020-03-20T01:16:19Z

What is the use case of this op in tensorflow? Having to check finite-ness inside DL models seems very weird to me. If there is indeed a possibility of infinity to show up, something (model or data) is broken anyway, so it is not a problem for us.

Error handling should be done in earlier layers before TVM.

maheshambule · 2020-03-20T20:47:05Z

@masahi, Yes generally it does not make sense to use finiteness check inside DL model. It can be generally used in preprocessing input data etc but not in the model. We had received a model from the customer which was using this operator. We do not know the exact use case though. But there is a possibility that this operator appears in the DL models. Also, will it be useful considering training support in TVM?

masahi · 2020-03-20T21:04:20Z

If it is only TF frontend change, it is totally fine. But you also added a new relay/topi op, and even added new TIR intrinsics, just to support that customer's model. That makes me concerned.

TVM is supposed to support other frameworks not just TF, and if we start adding these framework specific corner case ops, it quickly leads to code bloat. I prefer adding new ops only if they could be useful across different frameworks.

People use TVM because they want to go fast. Having finteness check doesn't help with that. You should talk to your customer if they can remove it, or if it is fine for us to ignore Isfinite op in our frontend.

Also, will it be useful considering training support in TVM?

I don't know if it is useful to training. Maybe @MarisaKirisame can comment.

masahi · 2020-03-20T21:29:52Z

@maheshambule also check out the proposal by @soiferj below. If you need to support TF models, this should be a good solution.
https://discuss.tvm.ai/t/discuss-running-graphs-when-ops-arent-supported/4482

FrozenGene · 2020-03-21T09:15:04Z

Generally speaking, I think Isfinite / Isnan should be provided in our tir, which has been provided in Halide. @tqchen could comment it more.

masahi · 2020-03-21T10:40:37Z

I also found IsInf and IsNaN are also on ONNX https://github.com/onnx/onnx/blob/master/docs/Operators.md#IsInf

Still not sure what the use case is. We have to use VM runtime, which has some overhead, if these ops are used together with if/else.

masahi · 2020-03-21T10:52:16Z

@FrozenGene I grepped through the Halide code base, isinf/isnan don't seem to be in Halide's IR, but they use std::isnan and std::isfinite during codegen. They also appear inside generated code for C source based backend (OpenCL etc.)

FrozenGene · 2020-03-21T11:39:53Z

@FrozenGene I grepped through the Halide code base, isinf/isnan don't seem to be in Halide's IR, but they use std::isnan and std::isfinite during codegen. They also appear inside generated code for C source based backend (OpenCL etc.)

halide/Halide@697b4be this commit add nan / inf support in Halide.

masahi · 2020-03-23T01:42:48Z

@maheshambule There is a conflict, please rebase

masahi · 2020-03-23T11:17:08Z

Merging it for now, thanks @maheshambule @jwfromm @FrozenGene

maheshambule · 2020-03-23T11:35:31Z

Thanks @masahi.
I will try to check for the exact use case and will let you know. Also, the proposal to add support to fallback to frontend runtime looks interesting. I will try to look into it once I get some time.

* isfinite doc update * isfinit expr * isfinit expr * isfinite schedule reg * isfinite python binding * isfinite python binding * relay register isfinite * isfinite type relation * intrin isfinite * topi isfinite * testcase topi isfinite * tf frontend isfinite * tf frontend isfinite testcase * test case relay isfinite * small fixes * test forward tf isfinite * test cases injective for cuda * remove float16 test case * add support for isinf * remove unwanted import * fix conflict

maheshambule added 15 commits March 3, 2020 14:20

isfinite doc update

9e61334

isfinit expr

465dbe9

isfinit expr

37a79c0

isfinite schedule reg

074d733

isfinite python binding

9c2d11e

isfinite python binding

0b2cb02

relay register isfinite

29908b1

isfinite type relation

495acff

intrin isfinite

553c6b2

topi isfinite

cdf7d1a

testcase topi isfinite

ed3fde2

tf frontend isfinite

415de20

tf frontend isfinite testcase

90f4188

test case relay isfinite

99f0fa5

small fixes

a3594ec

maheshambule requested review from Huyuwei and Laurawly as code owners March 3, 2020 17:50

maheshambule and others added 4 commits March 4, 2020 14:32

test forward tf isfinite

168f274

test cases injective for cuda

5cc4cf8

remove float16 test case

22d8871

Merge branch 'master' into isfinite

6704d3d

FrozenGene added the status: need review label Mar 6, 2020

Merge branch 'master' into isfinite

0fc067a

jwfromm reviewed Mar 14, 2020

View reviewed changes

maheshambule added 3 commits March 17, 2020 15:34

resolve conflicts

ea93829

add support for isinf

46afbcb

remove unwanted import

92c2829

jwfromm approved these changes Mar 17, 2020

View reviewed changes

maheshambule and others added 2 commits March 23, 2020 10:51

Merge branch 'master' into isfinite

fcf45ba

fix conflict

d1fdda4

masahi approved these changes Mar 23, 2020

View reviewed changes

masahi merged commit 9037f4e into apache:master Mar 23, 2020

ZihengJiang mentioned this pull request Sep 25, 2020

TVM v0.7 Release Note Candidate #6486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay, Topi, TF Frontend] Isfinite operator #4981

[Relay, Topi, TF Frontend] Isfinite operator #4981

maheshambule commented Mar 3, 2020 •

edited

Loading

maheshambule commented Mar 11, 2020

jwfromm Mar 14, 2020

maheshambule Mar 17, 2020

jwfromm Mar 14, 2020

maheshambule Mar 17, 2020

jwfromm Mar 14, 2020

maheshambule Mar 17, 2020

maheshambule commented Mar 17, 2020

jwfromm left a comment

maheshambule commented Mar 18, 2020

jwfromm commented Mar 19, 2020

masahi commented Mar 20, 2020 •

edited

Loading

maheshambule commented Mar 20, 2020

masahi commented Mar 20, 2020

masahi commented Mar 20, 2020

FrozenGene commented Mar 21, 2020

masahi commented Mar 21, 2020 •

edited

Loading

masahi commented Mar 21, 2020 •

edited

Loading

FrozenGene commented Mar 21, 2020 •

edited

Loading

masahi commented Mar 23, 2020

masahi commented Mar 23, 2020

maheshambule commented Mar 23, 2020

[Relay, Topi, TF Frontend] Isfinite operator #4981

[Relay, Topi, TF Frontend] Isfinite operator #4981

Conversation

maheshambule commented Mar 3, 2020 • edited Loading

maheshambule commented Mar 11, 2020

jwfromm Mar 14, 2020

Choose a reason for hiding this comment

maheshambule Mar 17, 2020

Choose a reason for hiding this comment

jwfromm Mar 14, 2020

Choose a reason for hiding this comment

maheshambule Mar 17, 2020

Choose a reason for hiding this comment

jwfromm Mar 14, 2020

Choose a reason for hiding this comment

maheshambule Mar 17, 2020

Choose a reason for hiding this comment

maheshambule commented Mar 17, 2020

jwfromm left a comment

Choose a reason for hiding this comment

maheshambule commented Mar 18, 2020

jwfromm commented Mar 19, 2020

masahi commented Mar 20, 2020 • edited Loading

maheshambule commented Mar 20, 2020

masahi commented Mar 20, 2020

masahi commented Mar 20, 2020

FrozenGene commented Mar 21, 2020

masahi commented Mar 21, 2020 • edited Loading

masahi commented Mar 21, 2020 • edited Loading

FrozenGene commented Mar 21, 2020 • edited Loading

masahi commented Mar 23, 2020

masahi commented Mar 23, 2020

maheshambule commented Mar 23, 2020

maheshambule commented Mar 3, 2020 •

edited

Loading

masahi commented Mar 20, 2020 •

edited

Loading

masahi commented Mar 21, 2020 •

edited

Loading

masahi commented Mar 21, 2020 •

edited

Loading

FrozenGene commented Mar 21, 2020 •

edited

Loading