[onert] Add check for topological order validity #12602

Aeren1564 · 2024-02-05T08:00:53Z

This commit introduces the following functions

assertValidTopologicalOrder
assertValidBackwardTopologicalOrder

which will catch any potential bugs on forward or backward topological ordering used in onert.

ONE-DCO-1.0-Signed-off-by: YongHyun An [email protected]

From draft #12562

Aeren1564 · 2024-02-05T08:07:57Z

The model on #12325 (comment) is now correctly rejected as follows.

Error during nnfw_session::train_prepare : Invalid backward topological order: inversion between @45 and @46

Aeren1564 · 2024-02-05T08:11:42Z

runtime/onert/core/src/ir/train/TrainableGraph.cc

+        if (p > q)
+          throw std::runtime_error{
+            "Invalid " + order_type + " topological order: inversion between @" +
+            std::to_string(index.value()) + " and @" + std::to_string(use.value())};


There is an inversion between node u and v in an ordering if there is an edge from u to v and v preceeds u.

A valid topological order should not contain any inversion.

Aeren1564 · 2024-02-05T08:13:38Z

runtime/onert/core/src/ir/train/TrainableGraph.cc

+void TrainableGraph::assertValidBackwardTopologicalOrder(
+  std::vector<ir::OperationIndex> order) const
+{
+  std::reverse(order.begin(), order.end());
+  assertValidTopologicalOrder(order, "backward");
+}


An order is a backward topological order if and only if its reverse is a forward topological order.

This has been moved to

ONE/runtime/onert/core/src/ir/train/TrainableGraph.cc

Lines 132 to 133 in 6f0884b

if (!is_forward)

std::reverse(order.begin(), order.end());

runtime/onert/core/src/ir/train/TrainableGraph.cc

glistening · 2024-02-06T02:30:45Z

runtime/onert/core/include/ir/train/TrainableGraph.h

+  void assertValidTopologicalOrder(const std::vector<ir::OperationIndex> &order,
+                                   const std::string &order_type) const;
+  void assertValidBackwardTopologicalOrder(std::vector<ir::OperationIndex> order) const;


From the name of assert..., are these for test only? If so, what about moving this into test code?

I intended ONERT to throw error upon finding an invalid topological order.

Adding them as unittest instead would only catch some specific pre-determined cases and ONERT could still end up using some invalid topological order.

IMHO,

I believe backwardTopologicalOrder should always return correct answer. It is well-known problem. We already have several ones from compiler frontend in this repo, and internal repo.

If there was a bug, I would add regression testcase for backwardTopologicalOrder with your validate function.

zetwhite

I read the code. The logic looks good to me. 👍

But I agree with this opinion - https://github.com/Samsung/ONE/pull/12602/files#r1479151257

If this code is only for ensuring that topological sorting is done right,
I'd prefer to make it run only in debug mode_{(sorry but i either don't know how to make specific code run only on debug mode)} or move it into the test.

Aeren1564 · 2024-02-06T06:16:30Z

If this code is only for ensuring that topological sorting is done right,
I'd prefer to make it run only in debug mode

May I ask why? I think I expect ONERT to reject such incorrect result regardless of I'm in debug mode or not, and I believe this check is not a performance bottleneck either.

runtime/onert/core/src/ir/train/TrainableGraph.cc

jyoungyun

LGTM

jyoungyun · 2024-02-06T07:05:38Z

runtime/onert/core/src/ir/train/TrainableGraph.cc

 std::vector<ir::OperationIndex> TrainableGraph::topolSortOperations() const
 {
-  return _graph.topolSortOperations();
+  auto ret = _graph.topolSortOperations();
+  assertValidForwardTopologicalOrder(ret);


I prefer to use verify instead of assert for the function name, but this depends on the developer's taste. So your function naming is okay. In addition, the assertValidTopologicalOrder function can be called directly, but the current code is also good. :)

zetwhite · 2024-02-06T07:45:20Z

May I ask why? I think I expect ONERT to reject such incorrect result regardless of I'm in debug mode or not, and I believe this check is not a performance bottleneck either.

In my understanding overall view is that :

topolSortOperations and btopolSortOperations do the sorting and its' sorting algorithm should guarantee that order, somehow.
This code is kind of an additional validation check to verify sorting algorithm is correct.

In the point of 2, this validation looks like a test code to verify sorting algorithm. And it is meaningful for onert developers not user. If user figured out that btopol is wrong, there is no option except waiting the onert core is fixed.

So I thought the direction that this logic runs for only debug mode for developer, to get help to update sorting algorithm.

I don't know the detailed thing about this issue. Is there a near-plan to fix btopolSortOperations to resolve this - #12325 (comment) ?

I don't have a strong opinion about this.
So, I could approve this draft as-is!

runtime/onert/core/src/ir/train/TrainableGraph.cc

glistening · 2024-02-06T08:09:20Z

runtime/onert/core/include/ir/train/TrainableGraph.h

+  void assertValidTopologicalOrder(const std::vector<ir::OperationIndex> &order,
+                                   const std::string &order_type) const;
+  void assertValidBackwardTopologicalOrder(std::vector<ir::OperationIndex> order) const;


IMHO,

I believe backwardTopologicalOrder should always return correct answer. It is well-known problem. We already have several ones from compiler frontend in this repo, and internal repo.

If there was a bug, I would add regression testcase for backwardTopologicalOrder with your validate function.

runtime/onert/core/src/ir/train/TrainableGraph.cc

This commit introduces the following functions - assertValidTopologicalOrder - assertValidBackwardTopologicalOrder which will catch any potential bugs on forward or backward topological ordering used in onert. ONE-DCO-1.0-Signed-off-by: YongHyun An <[email protected]>

jyoungyun

LGTM

ragmani

LGTM

hseok-oh

LGTM

glistening

LGTM

Aeren1564 mentioned this pull request Feb 5, 2024

[onert] Fix btopolSortOperations #12601

Closed

2 tasks

Aeren1564 commented Feb 5, 2024

View reviewed changes

Aeren1564 marked this pull request as ready for review February 5, 2024 08:16

Aeren1564 added approval: 2 Require at least 2 approvals PR/ready for review It is ready to review. Please review it. labels Feb 5, 2024

Aeren1564 requested review from jyoungyun and a team February 5, 2024 08:20

zetwhite reviewed Feb 6, 2024

View reviewed changes

runtime/onert/core/src/ir/train/TrainableGraph.cc Show resolved Hide resolved

glistening reviewed Feb 6, 2024

View reviewed changes

zetwhite reviewed Feb 6, 2024

View reviewed changes

hseok-oh reviewed Feb 6, 2024

View reviewed changes

runtime/onert/core/src/ir/train/TrainableGraph.cc Outdated Show resolved Hide resolved

jyoungyun previously approved these changes Feb 6, 2024

View reviewed changes

glistening reviewed Feb 6, 2024

View reviewed changes

ragmani reviewed Feb 6, 2024

View reviewed changes

runtime/onert/core/src/ir/train/TrainableGraph.cc Outdated Show resolved Hide resolved

Aeren1564 dismissed jyoungyun’s stale review via c01c27e February 7, 2024 04:51

Aeren1564 requested a review from jyoungyun February 7, 2024 04:53

jyoungyun approved these changes Feb 7, 2024

View reviewed changes

zetwhite approved these changes Feb 7, 2024

View reviewed changes

ragmani approved these changes Feb 7, 2024

View reviewed changes

hseok-oh approved these changes Feb 7, 2024

View reviewed changes

glistening approved these changes Feb 7, 2024

View reviewed changes

glistening merged commit 1ef4072 into Samsung:master Feb 7, 2024
9 checks passed

Aeren1564 deleted the add_toposort_check branch February 7, 2024 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[onert] Add check for topological order validity #12602

[onert] Add check for topological order validity #12602

Aeren1564 commented Feb 5, 2024

Aeren1564 commented Feb 5, 2024 •

edited

Loading

Aeren1564 Feb 5, 2024 •

edited

Loading

Aeren1564 Feb 5, 2024

Aeren1564 Feb 6, 2024

glistening Feb 6, 2024

Aeren1564 Feb 6, 2024 •

edited

Loading

glistening Feb 6, 2024 •

edited

Loading

zetwhite left a comment

Aeren1564 commented Feb 6, 2024 •

edited

Loading

jyoungyun left a comment

jyoungyun Feb 6, 2024

zetwhite commented Feb 6, 2024 •

edited

Loading

glistening Feb 6, 2024 •

edited

Loading

jyoungyun left a comment

ragmani left a comment

hseok-oh left a comment

glistening left a comment

[onert] Add check for topological order validity #12602

[onert] Add check for topological order validity #12602

Conversation

Aeren1564 commented Feb 5, 2024

Aeren1564 commented Feb 5, 2024 • edited Loading

Aeren1564 Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Aeren1564 Feb 5, 2024

Choose a reason for hiding this comment

Aeren1564 Feb 6, 2024

Choose a reason for hiding this comment

glistening Feb 6, 2024

Choose a reason for hiding this comment

Aeren1564 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

glistening Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

zetwhite left a comment

Choose a reason for hiding this comment

Aeren1564 commented Feb 6, 2024 • edited Loading

jyoungyun left a comment

Choose a reason for hiding this comment

jyoungyun Feb 6, 2024

Choose a reason for hiding this comment

zetwhite commented Feb 6, 2024 • edited Loading

glistening Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

jyoungyun left a comment

Choose a reason for hiding this comment

ragmani left a comment

Choose a reason for hiding this comment

hseok-oh left a comment

Choose a reason for hiding this comment

glistening left a comment

Choose a reason for hiding this comment

Aeren1564 commented Feb 5, 2024 •

edited

Loading

Aeren1564 Feb 5, 2024 •

edited

Loading

Aeren1564 Feb 6, 2024 •

edited

Loading

glistening Feb 6, 2024 •

edited

Loading

Aeren1564 commented Feb 6, 2024 •

edited

Loading

zetwhite commented Feb 6, 2024 •

edited

Loading

glistening Feb 6, 2024 •

edited

Loading