Skip to content

Conversation

@chilo-ms
Copy link
Contributor

@chilo-ms chilo-ms commented Dec 3, 2025

Description

In some cases, e.g. when model has a node that produces output consumed by multiple nodes, calling the current implementation of Graph_GetGraphView() to get a subgraph returns incorrect OrtGraph.

This PR fixes the issue.

  • Original graph:

    image
  • Incorrect graph after calling Graph_GetGraphView() to get the subgraph:

    It includes three of the nodes from the original graph.
    The topk_indices is the output of the TopK and it shouldn't be added as a graph input shown in the graph below.
    The API implementation has issue handling this case.
    If we feed this subgraph into TRT parser, it would fail to parse the graph.

    image
  • Correct graph after calling Graph_GetGraphView() to get the subgraph:

    It includes three of the nodes from the original graph.
    The topk_indices now is not added as a graph input. Instead, the topk_indices is added as a graph output which is expected as Mod is in another subgraph that consumes it, so this subgraph has to make topk_indices a graph output.

    image

Motivation and Context

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

Copy link
Contributor

@adrianlizarraga adrianlizarraga left a comment

Choose a reason for hiding this comment

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

Hi Chi, I had some initial comments.

Also, I think there is existing code that seems to do something very similar:

std::unique_ptr<ComputeCapability> MakeComputeCapability(const GraphViewer& graph_viewer,
const std::vector<const Node*>& group,
const GenerateMetadefNameFn& generate_metadef_name,
const std::string& execution_provider_name,
bool drop_constant_initializers) {
std::unordered_set<const Node*> node_set;
node_set.reserve(group.size());
node_set.insert(group.cbegin(), group.cend());
std::unique_ptr<IndexedSubGraph> sub_graph = std::make_unique<IndexedSubGraph>();
std::unordered_set<const NodeArg*> node_outputs;
std::unordered_set<const NodeArg*> subgraph_inputs;
std::unordered_set<const NodeArg*> subgraph_outputs;
std::vector<const NodeArg*> ordered_subgraph_inputs;
std::vector<const NodeArg*> ordered_subgraph_outputs;
const auto& graph_output_list = graph_viewer.GetOutputs();
std::unordered_set<const NodeArg*> graph_outputs(graph_output_list.cbegin(), graph_output_list.cend());
for (const Node* node : group) {
sub_graph->nodes.push_back(node->Index());
for (const auto* input : node->InputDefs()) {
if (!input->Exists()) {
// skip the placeholder inputs
continue;
}
// if the node input was not produced by this subgraph, add it to the subgraph inputs.
if (!Contains(node_outputs, input)) {
if (!Contains(subgraph_inputs, input)) {
subgraph_inputs.insert(input);
ordered_subgraph_inputs.push_back(input);
}
}
}
const auto& output_defs = node->OutputDefs();
for (const auto* output_def : output_defs) {
node_outputs.insert(output_def);
// if output is overall graph output we need to produce it.
if (Contains(graph_outputs, output_def)) {
ordered_subgraph_outputs.push_back(output_def);
}
}
// if output connects to a node not in this subgraph we need to add it
// unless it was already added as an overall graph output,
for (auto it = node->OutputEdgesBegin(), end = node->OutputEdgesEnd(); it != end; ++it) {
if (!Contains(node_set, &it->GetNode())) {
const auto* output_def = output_defs[it->GetSrcArgIndex()];
if (!Contains(subgraph_outputs, output_def) && !Contains(graph_outputs, output_def)) {
subgraph_outputs.insert(output_def);
ordered_subgraph_outputs.push_back(output_def);
}
}
}
}
// Assign inputs and outputs to subgraph's meta_def
auto meta_def = std::make_unique<::onnxruntime::IndexedSubGraph::MetaDef>();
meta_def->name = generate_metadef_name();
meta_def->domain = execution_provider_name;
meta_def->since_version = 1;
meta_def->status = ONNX_NAMESPACE::EXPERIMENTAL;
for (const auto& input : ordered_subgraph_inputs) {
if (drop_constant_initializers && graph_viewer.IsConstantInitializer(input->Name(), true)) {
continue;
}
meta_def->inputs.push_back(input->Name());
}
for (const auto& output : ordered_subgraph_outputs) {
meta_def->outputs.push_back(output->Name());
}
sub_graph->SetMetaDef(std::move(meta_def));
return std::make_unique<ComputeCapability>(std::move(sub_graph));
}

Since that code has been tested, perhaps there's some way to reuse some of the implementation in some way? Just fyi.

int input_order = 0;
int output_order = 0;

std::vector<std::string> initializers;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could these be std::string_view to avoid copies? Would probably need to convert to std::string only when pushing into meta_def->constant_initializers. (or keep it as string and do a move).

indexed_sub_graph->nodes.push_back(node.Index());

for (const auto& input : node.InputDefs()) {
if (graph_viewer.IsConstantInitializer(input->Name(), true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to skip this input if !input->Exists() (i.e., for missing optional inputs).

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.

3 participants