Extend context and name propagation in errors #5396

klecki · 2024-03-26T18:03:08Z

Category: Refactoring

Description:

Add error context related to particular operator during graph construction and pipeline build:

error propagation in Pipeline::Build() is adjusted
Python calls to backend: Pipeline::AddOperator now augment errors with context.
OpSpec building errors propagate the proper operator name.

Replace most of the SchemaName() occurrences that were used to indicate the name operator
with the fully formatted operator name in the correct API.

Make sure that the naming information is added as soon as possible, so it can be accessed in partially
constructed schema with the correct values present.

Note: This PR mostly adds the context like:

Error in <device> operator `nvidia.dali.fn.operator_name`,
which was used in the pipeline definition with the following traceback:
<traceback>
encountered:
<Original error message>

to places where it was not previously used, but we are processing a single operator.

Error messages are adjusted to show the user-facing input/output/argument name (in uniform way) rather than the internal one.
Otherwise the checks and messages are preserved.
The types of error messages are not adjusted.

Additional information:

Affected modules and functionalities:

Pipeline and spec building

Key points relevant for the review:

Typos, broken formatting or conditions.

Tests:

Existing tests apply
Some of those error conditions are behind double or triple layer of error checks and are not observable by the Python user.
Will use CI to determine how much adjusting the wording affects the tests.
New tests added
The context added in AddOperator is verified to work, not all conditions are easily verified from Python tests. I won't build loads of broken GTest pipelines just to match the exact error messages.
- Python tests
- GTests
- Benchmark
- Other
N/A

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

klecki · 2024-03-26T18:35:00Z

!build

dali-automaton · 2024-03-26T18:40:58Z

CI MESSAGE: [13808683]: BUILD STARTED

dali-automaton · 2024-03-26T20:54:46Z

CI MESSAGE: [13808683]: BUILD FAILED

klecki · 2024-03-26T21:45:40Z

!build

dali-automaton · 2024-03-26T21:50:06Z

CI MESSAGE: [13812812]: BUILD STARTED

dali-automaton · 2024-03-26T23:30:42Z

CI MESSAGE: [13812812]: BUILD PASSED

mzient · 2024-03-27T10:35:45Z

dali/pipeline/graph/graph_descr.cc

+      "Operator `" + GetOpDisplayName(spec, true) + "` does not support in-place execution.");
  DALI_ENFORCE(spec.NumRegularInput() <= schema.MaxNumInput(),
-      "Operator '" + spec.SchemaName() +
-      "' supports a maximum of " + std::to_string(schema.MaxNumInput()) + " inputs, "
+      "Operator `" + GetOpDisplayName(spec, true) +
+      "` supports a maximum of " + std::to_string(schema.MaxNumInput()) + " inputs, "
      "but was passed " + std::to_string(spec.NumRegularInput()) + ".");
  DALI_ENFORCE(spec.NumRegularInput() >= schema.MinNumInput(),
-      "Operator '" + spec.SchemaName() +
-      "' supports a minimum of " + std::to_string(schema.MinNumInput()) + " inputs, "
+      "Operator `" + GetOpDisplayName(spec, true) +
+      "` supports a minimum of " + std::to_string(schema.MinNumInput()) + " inputs, "
      "but was passed " + std::to_string(spec.NumRegularInput()) + ".");
  DALI_ENFORCE(spec.NumOutput() == schema.CalculateOutputs(spec) + additional_outputs,
-      "Operator '" + spec.SchemaName() + "' supports "
+      "Operator `" + GetOpDisplayName(spec, true) + "` supports "


Upgrade to make_string? You'll be able to drop all those std::to_string...

mzient · 2024-03-27T10:37:13Z

dali/pipeline/operator/error_reporting.cc

@@ -96,7 +96,7 @@ void PropagateError(ErrorInfo error) {
  }
 }

-std::string GetErrorContextMessage(const OpSpec &spec) {
+std::string GetErrorContextMessage(const OpSpec &spec, const std::string &message_name) {


How about using a string_view in such functions?

Suggested change

std::string GetErrorContextMessage(const OpSpec &spec, const std::string &message_name) {

std::string GetErrorContextMessage(const OpSpec &spec, std::string_view message_name) {

mzient · 2024-03-27T10:53:41Z

dali/pipeline/pipeline.cc

+      DALI_FAIL(make_string("Error for ", FormatArgument(spec, arg_name),
+                            ". Named arguments inputs to operators must be CPU data nodes. "
+                            "However, a GPU data node was provided."));


I don't like this message. It says that the input is GPU, but we're not really sure, are we? We know that it's not CPU. Perhaps there's some internal error and it's just empty? I think we should stick to the known facts and report those.

Suggested change

DALI_FAIL(make_string("Error for ", FormatArgument(spec, arg_name),

". Named arguments inputs to operators must be CPU data nodes. "

"However, a GPU data node was provided."));

DALI_FAIL(make_string("Error for ", FormatArgument(spec, arg_name),

". Named arguments inputs to operators must be CPU data nodes. ""));

or otherwise make sure that we're saying the truth

Suggested change

DALI_FAIL(make_string("Error for ", FormatArgument(spec, arg_name),

". Named arguments inputs to operators must be CPU data nodes. "

"However, a GPU data node was provided."));

assert(it->second.has_gpu);

DALI_FAIL(make_string("Error for ", FormatArgument(spec, arg_name),

". Named arguments inputs to operators must be CPU data nodes. "

"However, a GPU data node was provided."));

mzient · 2024-03-27T12:50:27Z

dali/pipeline/pipeline.cc

+                 make_string("Error for ", FormatOutput(spec, i), ". Output name \"", output_name,
+                             "\" conflicts with existing intermediate result name."));


Suggested change

make_string("Error for ", FormatOutput(spec, i), ". Output name \"", output_name,

"\" conflicts with existing intermediate result name."));

make_string("Error while specifying ", FormatOutput(spec, i), ". Output name \"", output_name,

"\" conflicts with an existing intermediate result name."));

mzient · 2024-03-27T12:59:28Z

dali/pipeline/pipeline.cc

+      eptr = std::current_exception();
+    }
+    if (eptr) {
+      PropagateError({eptr,
+                      "Critical error when building pipeline:\n" + GetErrorContextMessage(op_spec),
+                      "\nCurrent pipeline object is no longer valid."});


What's wrong with the following?

Suggested change

eptr = std::current_exception();

}

if (eptr) {

PropagateError({eptr,

"Critical error when building pipeline:\n" + GetErrorContextMessage(op_spec),

"\nCurrent pipeline object is no longer valid."});

PropagateError({std::current_exception(),

"Critical error when building pipeline:\n" + GetErrorContextMessage(op_spec),

"\nCurrent pipeline object is no longer valid."});

}

Right, much simpler, adjusted the other occurrences as well.

dali/pipeline/pipeline.cc

klecki · 2024-03-27T18:46:02Z

!build

dali-automaton · 2024-03-27T18:50:46Z

CI MESSAGE: [13836164]: BUILD STARTED

dali-automaton · 2024-03-27T21:58:49Z

CI MESSAGE: [13836164]: BUILD FAILED

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki · 2024-04-10T12:27:58Z

!build

dali-automaton · 2024-04-10T12:30:38Z

CI MESSAGE: [14134128]: BUILD STARTED

dali-automaton · 2024-04-10T22:53:06Z

CI MESSAGE: [14134128]: BUILD PASSED

klecki force-pushed the less-schema-name-more-display-name branch 2 times, most recently from 786f7e4 to 2584a08 Compare March 26, 2024 18:18

mzient reviewed Mar 27, 2024

View reviewed changes

dali-automaton assigned awolant and szalpal Mar 27, 2024

mzient reviewed Mar 27, 2024

View reviewed changes

dali/pipeline/pipeline.cc Show resolved Hide resolved

mzient assigned mzient and unassigned szalpal Mar 27, 2024

mzient approved these changes Apr 2, 2024

View reviewed changes

awolant approved these changes Apr 2, 2024

View reviewed changes

klecki added 5 commits April 10, 2024 12:24

Extend context and name propagation in errors

68fe4b4

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Improve the naming

bbf8746

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

change std::exception_ptr usage

711c111

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Review adjustments

46c6935

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Reorder includes

e8d2079

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki force-pushed the less-schema-name-more-display-name branch from 71096a0 to e8d2079 Compare April 10, 2024 12:17

Remove newline

b5bf9db

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

Rewording

53148b2

Signed-off-by: Krzysztof Lecki <klecki@nvidia.com>

klecki merged commit 3b1b5f8 into NVIDIA:main Apr 11, 2024
7 checks passed

klecki deleted the less-schema-name-more-display-name branch April 11, 2024 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend context and name propagation in errors #5396

Extend context and name propagation in errors #5396

klecki commented Mar 26, 2024 •

edited

klecki commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

klecki commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

mzient Mar 27, 2024

klecki Mar 27, 2024

mzient Mar 27, 2024

klecki Mar 27, 2024

mzient Mar 27, 2024 •

edited

klecki Mar 27, 2024

mzient Mar 27, 2024 •

edited

klecki Mar 27, 2024

mzient Mar 27, 2024

klecki Mar 27, 2024

klecki commented Mar 27, 2024

dali-automaton commented Mar 27, 2024

dali-automaton commented Mar 27, 2024

klecki commented Apr 10, 2024

dali-automaton commented Apr 10, 2024

dali-automaton commented Apr 10, 2024

	std::string GetErrorContextMessage(const OpSpec &spec, const std::string &message_name) {
	std::string GetErrorContextMessage(const OpSpec &spec, std::string_view message_name) {

		make_string("Error for ", FormatOutput(spec, i), ". Output name \"", output_name,
		"\" conflicts with existing intermediate result name."));

Extend context and name propagation in errors #5396

Extend context and name propagation in errors #5396

Conversation

klecki commented Mar 26, 2024 • edited

Category: Refactoring

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

klecki commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

klecki commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

dali-automaton commented Mar 26, 2024

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

mzient Mar 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Mar 27, 2024 • edited

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

klecki commented Mar 27, 2024

dali-automaton commented Mar 27, 2024

dali-automaton commented Mar 27, 2024

klecki commented Apr 10, 2024

dali-automaton commented Apr 10, 2024

dali-automaton commented Apr 10, 2024

klecki commented Mar 26, 2024 •

edited

mzient Mar 27, 2024 •

edited

mzient Mar 27, 2024 •

edited