Minor cleanup in executor.cpp #2750

csarofeen · 2024-08-02T18:37:20Z

Add NVF_THROW
Add more information in bindInputs error
Add IValue debug string function
Cleanup some stale code

Was just going through executor a bit to get oriented and tried to do a bit of cleanup.

Improves error handling when invalid static sizes are provided to nvFuser. Specifically, when finding incompatible sizes within void ExpressionEvaluator::propagateBoundValuesThroughExactMaps(Fusion* fusion) {.

Before this code would throw an error like:

C++ exception with description "known_size == this_size INTERNAL ASSERT FAILED at "/opt/pytorch/Fuser/csrc/expr_evaluator.cpp":362, please report a bug with repro script to NVFuser at https://github.com/NVIDIA/Fuser/issues. Conflicting sizes: 654, 456

Now it looks like:

When trying to propagate constant tensor sizes through the graph a conflict was found with 2 different sizes across dimensions that are expected to match.
For Producer TV: T1_g[ iS2{( (nvfuser_index_t)(i0) )}, iS3{( (nvfuser_index_t)(i1) )} ] id: iS3{( (nvfuser_index_t)(i1) )} found size: 654
For Consumer TV: T2_g[ iS4{( (nvfuser_index_t)(i0) )}, iS5{( (nvfuser_index_t)(i1) )} ] id: iS5{( (nvfuser_index_t)(i1) )} found size: 456
With producer-consumer relationship through the expression: T2_g[ iS4{( (nvfuser_index_t)(i0) )}, iS5{( (nvfuser_index_t)(i1) )} ]
= T0_g[ iS0{( (nvfuser_index_t)(i0) )}, iS1{( (nvfuser_index_t)(i1) )} ]
+ T1_g[ iS2{( (nvfuser_index_t)(i0) )}, iS3{( (nvfuser_index_t)(i1) )} ];

- Add NVF_THROW - Add more information in bindInputs error - Add IValue debug string function - Cleanup some stale code

csarofeen · 2024-08-02T18:37:44Z

csrc/exceptions.h

-#define NVF_ERROR(cond, ...)                                  \
-  if ((!(cond))) {                                            \
-    nvfuser::nvfErrorFail(                                  \
+#define NVF_THROW(...) \


Wanted a throw to be able to get some better error messages in bindInputs.

csrc/utils.cpp

wujingyue · 2024-08-02T18:46:20Z

csrc/executor_utils.cpp

+      ss << "When trying to run the provided host program,"
+         << " there was an error with the provided input " << i
+         << ". Provided input was:\n  ";
+      ss << PolymorphicValue_functions::toString(*args[i]);
+      ss << "\n  which does not match the expected input:\n  ";
+      ss << inputs[i]->toString() << "\n";


Is e.what() worth keeping in the new exception message? It has the original source code location at least, which I think helps debugging.

In this instance I didn't think it was particularly helpful, as it's the caller that we would typically want to point at, not the inside of expression evaluator which is the first to find it. I think we'd actually want the error to be thrown higher where bindInputs is called as I expect most instances that would fail are because of providing bad inputs.

WDYT?

I trust your judgement -- I haven't ran into enough errors to have a strong opinion.

AFAICT, ExpressionEvaluator::bind is a deep function that can fail at

Fuser/csrc/evaluator_common.cpp

Line 369 in 346e51c

"Could not evaluate metadata expression for ",

. While I agree with you that "most instances that would fail are because of providing bad inputs", why an input is bad might not be immediately clear to the user.

I agree, that's why I lifted the error in the common place we bind a bunch of inputs to expression evaluator provided from someplace not generated by nvFuser (developers in tests and Thunder in integration).

csrc/utils.cpp

csrc/executor.cpp

jjsjann123

Thanks for removing the dead code!

csrc/executor.cpp

csarofeen · 2024-08-03T22:00:01Z

csrc/executor.h

      std::vector<at::Tensor> outputs,
      ExpressionEvaluator& expr_eval);

+  // TODO: args shouldn't come in a reference here because we will append the


@jjsjann123 see the todo here. We don't ever use args as they're updated with the outputs. We always pass it back as an array of tensors. So it is different behavior with evaluateFusionOutputs, but that seems to be the behavior we should want.

Cleanup printing. Co-authored-by: Jingyue Wu <[email protected]>

…cleanup

…to executor_cleanup

wujingyue · 2024-08-05T05:03:01Z

csrc/executor_utils.cpp

+      ss << "When trying to run the provided host program,"
+         << " there was an error with the provided input " << i
+         << ". Provided input was:\n  ";
+      ss << PolymorphicValue_functions::toString(*args[i]);
+      ss << "\n  which does not match the expected input:\n  ";
+      ss << inputs[i]->toString() << "\n";


I trust your judgement -- I haven't ran into enough errors to have a strong opinion.

AFAICT, ExpressionEvaluator::bind is a deep function that can fail at

Fuser/csrc/evaluator_common.cpp

Line 369 in 346e51c

"Could not evaluate metadata expression for ",

. While I agree with you that "most instances that would fail are because of providing bad inputs", why an input is bad might not be immediately clear to the user.

csarofeen · 2024-08-05T16:27:04Z

!build

…cleanup

…nt sizes through the exact graph.

csarofeen · 2024-08-28T21:18:45Z

!build

csarofeen · 2024-08-28T21:21:46Z

I went back over the error handling, I could use another review please, @jjsjann123 @wujingyue. Focused on changes in expr_evaluator.cpp @wujingyue and @jjsjann123 because I touched the ops testing in python.

wujingyue · 2024-08-28T21:27:52Z

csrc/expr_evaluator.cpp

+void handlePropagateError(
+    Fusion* fusion,
+    ExpressionEvaluator* expr_eval,
+    std::shared_ptr<VectorOfUniqueEntries<const IterDomain*>> id_set) {


Suggested change

std::shared_ptr<VectorOfUniqueEntries<const IterDomain*>> id_set) {

const VectorOfUniqueEntries<const IterDomain*>& id_set) {

Passing shared_ptr is slightly more expensive than passing in the underlying reference. The latter seems to suffice because id_set outlives this function.

csrc/expr_evaluator.cpp

jjsjann123 · 2024-08-30T21:20:28Z

csrc/executor.h

+  // outputs to be able to send it to the kernel. For now none of the users are
+  // reconsuming the args, so it is okay. It isn't done now because changing it
+  // from a reference makes a call as runFusion({}) ambiguous, and that is used
+  // in some places in the codebase.


Would be really interesting to see how we can resolve this cleanly, but I don't know any way to do that without changing the call site to not use braced-init-list. This gives me headache: https://en.cppreference.com/w/cpp/language/overload_resolution#Implicit_conversion_sequence_in_list-initialization

I don't know anything that can help overload resolution? wondering if @zasdfgbnm knows any dark magic?

Typos Co-authored-by: Jingyue Wu <[email protected]>

…cleanup

…to executor_cleanup

csarofeen · 2024-09-02T21:21:08Z

!build

tests/cpp/test_alias.cpp

…cleanup

csarofeen · 2024-09-06T20:25:10Z

!build

Minor cleanup in executor.cpp

d74a8bf

- Add NVF_THROW - Add more information in bindInputs error - Add IValue debug string function - Cleanup some stale code

csarofeen requested review from wujingyue and jjsjann123 August 2, 2024 18:37

csarofeen commented Aug 2, 2024

View reviewed changes

wujingyue reviewed Aug 2, 2024

View reviewed changes

jjsjann123 reviewed Aug 2, 2024

View reviewed changes

csrc/executor.cpp Show resolved Hide resolved

csarofeen commented Aug 3, 2024

View reviewed changes

Apply suggestions from code review

11df23a

Cleanup printing. Co-authored-by: Jingyue Wu <[email protected]>

csarofeen mentioned this pull request Aug 4, 2024

Move fusion executor files to their own directory #2756

Merged

csarofeen added 2 commits August 3, 2024 17:23

Merge branch 'main' of https://github.com/NVIDIA/Fuser into executor_…

78ca610

…cleanup

Merge branch 'executor_cleanup' of https://github.com/NVIDIA/Fuser in…

75543ce

…to executor_cleanup

wujingyue approved these changes Aug 5, 2024

View reviewed changes

csarofeen added 5 commits August 5, 2024 23:57

Merge branch 'main' of https://github.com/NVIDIA/Fuser into executor_…

549d150

…cleanup

Merge branch 'main' of https://github.com/NVIDIA/Fuser into executor_…

3f1036b

…cleanup

Merge branch 'main' of https://github.com/NVIDIA/Fuser into executor_…

f23b40b

…cleanup

Merge branch 'main' of https://github.com/NVIDIA/Fuser into executor_…

453e44f

…cleanup

Improve error message in expression evaluator when propagating consta…

9943152

…nt sizes through the exact graph.

csarofeen requested a review from wujingyue August 28, 2024 21:21

wujingyue reviewed Aug 28, 2024

View reviewed changes

jjsjann123 approved these changes Aug 30, 2024

View reviewed changes

csarofeen and others added 4 commits August 31, 2024 16:49

Apply suggestions from code review

97ef121

Typos Co-authored-by: Jingyue Wu <[email protected]>

Merge branch 'main' of https://github.com/NVIDIA/Fuser into executor_…

5826645

…cleanup

PR Feedback.

9bc2af8

Merge branch 'executor_cleanup' of https://github.com/NVIDIA/Fuser in…

8029b1b

…to executor_cleanup

wujingyue approved these changes Sep 2, 2024

View reviewed changes

tests/cpp/test_alias.cpp Outdated Show resolved Hide resolved

Merge branch 'main' of https://github.com/NVIDIA/Fuser into executor_…

8ecc5e7

…cleanup

csarofeen added 2 commits September 6, 2024 13:12

resolve bad conflict resolution.

1db8e06

Minor fix.

8f3ff98

csarofeen merged commit e9d27a2 into main Sep 8, 2024
35 of 36 checks passed

csarofeen deleted the executor_cleanup branch September 8, 2024 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor cleanup in executor.cpp #2750

Minor cleanup in executor.cpp #2750

csarofeen commented Aug 2, 2024 •

edited

Loading

csarofeen Aug 2, 2024

wujingyue Aug 2, 2024

csarofeen Aug 3, 2024

wujingyue Aug 5, 2024

csarofeen Aug 5, 2024

jjsjann123 left a comment

csarofeen Aug 3, 2024

wujingyue Aug 5, 2024

csarofeen commented Aug 5, 2024

csarofeen commented Aug 28, 2024

csarofeen commented Aug 28, 2024

wujingyue Aug 28, 2024

jjsjann123 Aug 30, 2024

csarofeen commented Sep 2, 2024

csarofeen commented Sep 6, 2024

	std::shared_ptr<VectorOfUniqueEntries<const IterDomain*>> id_set) {
	const VectorOfUniqueEntries<const IterDomain*>& id_set) {

Minor cleanup in executor.cpp #2750

Minor cleanup in executor.cpp #2750

Conversation

csarofeen commented Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsjann123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csarofeen commented Aug 5, 2024

csarofeen commented Aug 28, 2024

csarofeen commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csarofeen commented Sep 2, 2024

csarofeen commented Sep 6, 2024

csarofeen commented Aug 2, 2024 •

edited

Loading