Skip to content

Add support for setting shape inference function on fused nodes - #7007

Merged
Scott McKay (skottmckay) merged 4 commits into
microsoft:masterfrom
pranav-prakash:shape_inf_fuse
May 5, 2021
Merged

Scott McKay (skottmckay) merged 4 commits into
microsoft:masterfrom
pranav-prakash:shape_inf_fuse

Conversation

@pranav-prakash

@pranav-prakash Pranav Prakash (pranav-prakash) commented Mar 14, 2021

Copy link
Copy Markdown
Contributor

Description: See #7006.

The ability to manually set a shape inference function for function-based nodes is useful for execution providers who perform node-fusion during graph partitioning. While normally shape info is computed before partitioning and hence is preserved by node fusion, in the case of graph transforms that operate after partitioning (level 3 and above, e.g. NHWC graph transformations) shape info can be reset or new nodeArgs created. In such cases, rather than manually setting shape info in every graph transform that deals with fused nodes, it can be cleaner to just define the shape inference function upfront.

@snnn

Copy link
Copy Markdown
Contributor

Hi Ashwini Khade (@askhade), could you please help?

Comment thread onnxruntime/test/framework/inference_session_test.cc Outdated
Comment thread include/onnxruntime/core/graph/indexed_sub_graph.h Outdated
return *model_;
}
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use InferenceSessionWrapper

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for the pointer (didn't realize this existed). We need access to the mutable graph though (in order to call .resolve() which clears out the inferred shapes) so I added an additional GetMutableGraph method to InferenceSessionWrapper.

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux CPU x64 NoContribops CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux Nuphar CI Pipeline,Linux OpenVINO CI Pipeline

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run MacOS CI Pipeline,MacOS NoContribops CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule,orttraining-ortmodule-distributed

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 7 pipeline(s).

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 5 pipeline(s).

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run Windows WebAssembly CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pranav-prakash

Copy link
Copy Markdown
Contributor Author

Scott McKay (@skottmckay) The CI for minimal ORT seems to fail (despite the regular CPU pipeline succeeding).

/onnxruntime_src/include/onnxruntime/core/graph/indexed_sub_graph.h:41:41: error: ‘InferenceContext’ is not a member of ‘onnx’
   41 |     std::function<void (ONNX_NAMESPACE::InferenceContext&)> type_and_shape_inference_function;

Should I need to surround this by an #ifdef (I'm not too familiar with the minimal ORT builds).

@skottmckay

Copy link
Copy Markdown
Contributor

Scott McKay (@skottmckay) The CI for minimal ORT seems to fail (despite the regular CPU pipeline succeeding).

/onnxruntime_src/include/onnxruntime/core/graph/indexed_sub_graph.h:41:41: error: ‘InferenceContext’ is not a member of ‘onnx’
   41 |     std::function<void (ONNX_NAMESPACE::InferenceContext&)> type_and_shape_inference_function;

Should I need to surround this by an #ifdef (I'm not too familiar with the minimal ORT builds).

Yes please. Put it inside #if !defined(ORT_MINIMAL_BUILD)

We have an extremely limited dependency on ONNX in the minimal build in order to minimize the binary size. The type/shape info is saved in the ORT format model used by the minimal build and not recalculated at runtime.

@pranav-prakash

Copy link
Copy Markdown
Contributor Author

Fixed, thanks for the info! Could you please re-run CI?

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run MacOS CI Pipeline,MacOS NoContribops CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,Windows WebAssembly CI Pipeline

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux CPU x64 NoContribops CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux Nuphar CI Pipeline,Linux OpenVINO CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule,orttraining-ortmodule-distributed

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 7 pipeline(s).

op_schema->SetDoc(meta_def->doc_string);
op_schema->SinceVersion(meta_def->since_version);

if (meta_def->type_and_shape_inference_function) {

@skottmckay Scott McKay (skottmckay) May 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably need an ifdef around all usages (both here and in unit tests).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems to pass the minimal CI without it; I guess function.cc and the unit test are never included in the minimal build?

Comment thread onnxruntime/test/util/include/inference_session_wrapper.h
@skottmckay

Copy link
Copy Markdown
Contributor

/azp run orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule,orttraining-ortmodule-distributed

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run MacOS CI Pipeline,MacOS NoContribops CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,Windows WebAssembly CI Pipeline

@skottmckay

Copy link
Copy Markdown
Contributor

/azp run Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux CPU x64 NoContribops CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline,Linux Nuphar CI Pipeline,Linux OpenVINO CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 6 pipeline(s).

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 7 pipeline(s).

@skottmckay
Scott McKay (skottmckay) merged commit 053bada into microsoft:master May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core runtime issues related to core runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants