Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error reporting on partially unused output of external source #3975

Merged
merged 6 commits into from
Jun 20, 2022

Conversation

stiepan
Copy link
Member

@stiepan stiepan commented Jun 8, 2022

Improve error reporting on partially unused output of external source

Signed-off-by: Kamil Tokarski ktokarski@nvidia.com

Category:

Description:

If one creates external source (and provides the source to the operator) with multiple outputs, but does not use some of the outputs, it results in vague error on feed_input that tries to feed input to non-existing operator. Catch that early (in pipeline.build()) and provide clearer error.

Find instances of partially unused external source groups during the Python pipeline build and explicitly disable the unused instances in the relevant ExternalSourceGroup, so that the group skips feeding the pruned external source nodes during the run. Emit relevant warning.

Additional information:

Affected modules and functionalities:

Python pipeline wrapper.

Nose utility module: added assert_warns mirroring asser_raises, extracted common code.

Key points relevant for the review:

Given the number of ways to define functioning pipeline, please think if the check does not interrupt some valid use case of external source that would work otherwise (I don't think it can, but it would be good to double check it with someone else).

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: DALI-2802

if op.id not in graph_op_ids:
num_outputs = len(group.instances)
source_str = truncate_str(group.source_desc.source)
raise RuntimeError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there is more than one ES it will be hard to guess which one causes this but I guess we cannot distinguish ES instances anyhow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope for group.source_desc.source to give some clue (depending how top-level object it is it may be of some value).

@klecki
Copy link
Contributor

klecki commented Jun 13, 2022

Just for the sake of documenting the discussion, the alternative to erroring out on this would be to automatically mark all the outputs of the externals source as sinks. Or at least those that are not returned, potentially issuing a warning?

Edit:
As discussed, we can extract the graph backtracking, run it for outputs and _sinks, report the warnings for the missing external sources as we do the error now, and resume the backtracking for unused external sources.

Any use that would previously error out in this case would be also missing the node in C++ backend, so I don't think there would be any problematic use cases.

num_outputs = len(group.instances)
source_str = truncate_str(group.source_desc.source)
raise RuntimeError(
f"The external source '{source_str}' produces {num_outputs} outputs, but "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
f"The external source '{source_str}' produces {num_outputs} outputs, but "
f"The external source node '{source_str}' produces {num_outputs} outputs, but "

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

…ternal source

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
@stiepan stiepan force-pushed the partially_utilized_ext_source_error branch from 1140713 to b69dacc Compare June 20, 2022 12:48
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
@stiepan
Copy link
Member Author

stiepan commented Jun 20, 2022

Okay, so to sum up the discussion: we do not want to prevent the pipeline from removing unused external sources from graph, but rather want to prevent python code from feeding them,

@stiepan
Copy link
Member Author

stiepan commented Jun 20, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5130776]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5130776]: BUILD FAILED

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
@stiepan
Copy link
Member Author

stiepan commented Jun 20, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5131731]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [5131731]: BUILD PASSED

@stiepan stiepan merged commit 0b49400 into NVIDIA:main Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants