Restructure function inliner by gramalingam · Pull Request #11731 · microsoft/onnxruntime

gramalingam · 2022-06-03T21:20:00Z

Description:

Reimplement the function inliner.

Motivation and Context
The current function inliner has several limitations and drawbacks.
(i) It doesn't handle sub-graphs (graph-valued attributes contained in control-flow nodes) in several contexts.
(ii) There is a duplication of renaming-logic, with renaming of tensors happening in two places.
(iii) The creation of a FunctionImpl as a temporary (from a FunctionProto), which is then copied over by duplicating all its nodes in the main graph.

The new implementation fixes these limitations.

lgtm-com · 2022-06-04T01:12:59Z

This pull request introduces 1 alert when merging 5679db2 into 95a16c1 - view on LGTM.com

new alerts:

1 for Commented-out code

onnxruntime/core/graph/function_utils.cc

xadupre · 2022-06-07T09:45:51Z

onnxruntime/test/framework/function_test.cc

+// Check variable renaming in subgraphs.
+// Scenario: input lx is used within subgraphs, but not in main graph.
+// Both must be renamed to match the actual parameter name.
+TEST(FunctionTest, InputInSubgraph) {


Maybe add a test checking a function calling another function can be inlined as well and another one checking that a function called from a subgraph can be inlined as well.

Good point. Added these test-cases.

onnxruntime/core/graph/function_utils.cc

souptc · 2022-06-13T15:30:23Z

onnxruntime/core/graph/graph.cc

-    // This node has a schema defined function proto. If it is a context dependent function
-    // then build it otherwise fetch the FunctionProto from schema.
-    ONNX_NAMESPACE::FunctionProto onnx_function_proto;
+    onnx_function_proto = *func_template_->onnx_func_proto_;


copy probably is fine as there is no initializers in function proto. but do we really need a copy?

Good question. I thought about it earlier. But decided this was fine for current use, because we end up changing the names/attributes when inlining. So, we end up with a copy anyway. (The current implementation reduces the number of intermediate-representations used during inlining from two to one. Reducing the one to zero is possible, but would complicate the code, so settled on this.)

souptc · 2022-06-13T15:32:06Z

onnxruntime/core/graph/graph.cc

+    ORT_ENFORCE(callnode.TryGetFunctionProto(inlined_fp), "Node has no function body and cannot be inlined.");
+    ONNX_NAMESPACE::NodeProto callnode_proto;
+    callnode.ToProto(callnode_proto);
+    function_utils::Specialize(inlined_fp, callnode_proto, callnode.GetAttributes(), uniq_identifier);


any reason why we prefer the NodeProto in the signature, instead of use Graph::Node? or, could we provide an overload with Graph::Node so we don't need convert to nodeproto here?

I think we could promote/move the underlying code into ONNX eventually (purely proto-based code). Yes, I can add an overload with Graph::Node.

Added the overload.

onnxruntime/core/graph/function_utils.cc

lgtm-com · 2022-06-13T20:11:22Z

This pull request introduces 1 alert when merging 4f08762 into b0e027c - view on LGTM.com

new alerts:

1 for Commented-out code

liqunfu · 2022-06-14T03:34:23Z

onnxruntime/core/graph/function_utils.cc

        ONNX_NAMESPACE::shape_inference::InferShapeForFunctionNode(*onnx_func_proto, func_domain_to_version,
-                                                                   schema_registry, ctx, options, map_copy);
+                                                                   schema_registry, ctx, options, map_copy,
+                                                                   nullptr, &empty_map);


When debugging with onnx-script optional input, I am hitting symbol_table being nullptr exception. Not sure the root cause is here and if this PR solves the issue. I am urging test this code with onnx-script because now, I just merged function-experiment branch with master branch and I see too many breaks :(

@liqunfu : this PR requires the update to ONNX version. I think that might be the problem for the issues?

Fixes #11640

garymm · 2022-06-15T23:44:26Z

This is needed to support ONNX 1.12, in particular for the new SequenceMap op, which is a function that uses subgraphs.

lgtm-com · 2022-06-16T21:59:50Z

This pull request introduces 1 alert when merging 9e5af64 into 3d99f16 - view on LGTM.com

new alerts:

1 for Commented-out code

lgtm-com · 2022-06-16T23:40:00Z

This pull request introduces 1 alert when merging f279243 into 3d99f16 - view on LGTM.com

new alerts:

1 for Commented-out code

csharp/test/Microsoft.ML.OnnxRuntime.Tests.Common/OnnxMl.cs

Follow-ups that need to happen after this and before the next ORT release: * Support SequenceMap with #11731 * Support signal ops with #11778 Follow-ups that need to happen after this but don't necessarily need to happen before the release: * Implement LayerNormalization kernel for opset version 17: #11916 Fixes #11640

garymm · 2022-06-22T00:21:08Z

ONNX 1.12 has been merged. @gramalingam can you please rebase, test and merge?

lgtm-com · 2022-06-23T02:38:51Z

This pull request introduces 1 alert when merging 11e1d12 into e24349b - view on LGTM.com

new alerts:

1 for Commented-out code

garymm · 2022-06-23T15:22:04Z

The only failing check seems unrelated to this change. It seems numpy.asscalar has been removed but there's still some code calling it:
https://numpy.org/doc/1.21/reference/generated/numpy.asscalar.html

https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=675901&view=logs&j=7536d2cd-87d4-54fe-4891-bfbbf2741d83&t=305229be-e8ba-5189-ca61-fcb77d866478

garymm · 2022-06-23T15:25:09Z

Should be fixed by #11965. Pipeline running again now.

garymm

https://github.com/microsoft/onnxruntime/blob/master/onnxruntime/test/testdata/onnx_backend_test_series_filters.jsonc#L8

lgtm-com · 2022-06-23T18:23:25Z

This pull request introduces 1 alert when merging 3d666d9 into c398ad5 - view on LGTM.com

new alerts:

1 for Commented-out code

gramalingam · 2022-06-24T00:07:15Z

@garymm : can you approve again? The last update dismissed your previous approval, thanks!

Follow-ups that need to happen after this and before the next ORT release: * Support SequenceMap with #11731 * Support signal ops with #11778 Follow-ups that need to happen after this but don't necessarily need to happen before the release: * Implement LayerNormalization kernel for opset version 17: #11916 Fixes #11640

* Add nested function call tests * Add overload for Specialize * Pass symboltable to onnx shape inference * Avoid renaming empty names * Enable sequence_map tests which failed before this change

Follow-ups that need to happen after this and before the next ORT release: * Support SequenceMap with #11731 * Support signal ops with #11778 Follow-ups that need to happen after this but don't necessarily need to happen before the release: * Implement LayerNormalization kernel for opset version 17: #11916 Fixes #11640

* Add nested function call tests * Add overload for Specialize * Pass symboltable to onnx shape inference * Avoid renaming empty names * Enable sequence_map tests which failed before this change

Follow-ups that need to happen after this and before the next ORT release: * Support SequenceMap with #11731 * Support signal ops with #11778 Follow-ups that need to happen after this but don't necessarily need to happen before the release: * Implement LayerNormalization kernel for opset version 17: #11916 Fixes #11640

* Add nested function call tests * Add overload for Specialize * Pass symboltable to onnx shape inference * Avoid renaming empty names * Enable sequence_map tests which failed before this change

* Update ONNX to 1.12 (#11924) Follow-ups that need to happen after this and before the next ORT release: * Support SequenceMap with #11731 * Support signal ops with #11778 Follow-ups that need to happen after this but don't necessarily need to happen before the release: * Implement LayerNormalization kernel for opset version 17: #11916 Fixes #11640 * Dll version fix ovep4.1 (#11953) * Setting default version values for ovep dlls as well * Update backend_manager.cc Co-authored-by: mayavijx <[email protected]> Co-authored-by: mohsin <[email protected]> * Optimize t5 encoder in beam search (#11926) * ooptimize t5 encoder * update * update * update * refactor expand impl * cuda tests passed * update * alignment * more alignments * review comments * Allow saving on CPU usage for infrequent inference requests by reducing thread spinning (#11841) Introduce Start/Stop threadpool spinning switch Add a session config option to force spinning stop at the end of the Run() * Restructure function inliner (#11731) * Add nested function call tests * Add overload for Specialize * Pass symboltable to onnx shape inference * Avoid renaming empty names * Enable sequence_map tests which failed before this change * Deprecate APIs returning raw ptrs and provide replacements (#11922) Provider better documentation * register signal ops for opset 17 (#11778) * Register signal ops for op set 17 Note code is mostly being moved, not added. These ops were previously only registered as Microsoft contrib ops and only built if `BUILD_MS_EXPERIMENTAL_OPS=1`. They've been added to the ai.onnx standard op set in version 17. Main components of this change: * Move the kernels from the conrib_ops directory to the core directory. * Add function bodies for ms experimental ops. This will allow old models that use the contrib ops to continue to function. All the function bodies consist of a single op (the new standard op), so performance overhead should be minimal. Minor clean-up also in this change: * De-duplicate get_scalar_value_from_tensor: put it in a new utils.h. * Fix some bugs that caused compilation errors with the experimental ops. Tested with `build.sh --ms_experimental` * Fix some spelling errors and lint violations. * Replace a couple of switch statements with `MLTypeCallDispatcher`. * Use `InlineVector` instead of `std::vector`. Unblocks #11640 * Include opset 15 in Conv+BatchNormalization fusion (#11960) * Fix WinML Tests are still targetting deprecated (deleted) experimental signal op definitions (#12006) * fix winml tests * remove legacy test * switch idft -> dft+inverse attr * upgrade opset 13->17 for signal ops tests * [C# Tests] Add support for double tensor output in TestPreTrainedModels. (#12008) Add support for double tensor output in TestPreTrainedModels. * DML EP ResNet50 opset 15 fails in ONNX checker for FusedBatchNormalization lacking training_mode attribute (#12010) FusedBatchNormalization include training_mode attribute * Generalize native op creation (#11539) * create op from ep * read input count from context * create holder to host nodes * fix typo * cast type before comparison * throw error on API fail * silence warning from minimal build * switch to unique_ptr with deleter to host nodes * fix typo * fix build err for minimal * fix build err for minimal * add UT for conv * enable test on CUDA * add comment * fix typo * use gsl::span and string view for Node constructor * Added two APIs - CopyKernelInfo and ReleaseKernelInfo * pass gsl::span by value * switch to span<NodeArg* const> to allow for reference to const containers * fix typo * fix reduced build err * fix reduced build err * refactoring node construction logic * rename exceptions * add input and output count as arguments for op creation * refactor static member * use ORT_CATCH instead of catch * cancel try catch * add static value name map * format input definition and set err code * fix comments * fix typo * [DML EP] Pad operator: Handle negative pad counts (#11974) * Pad fallback to CPU * Added queryPad in operatorRegistration.cpp * Acknowledged PR comments * Used any_of * used none_of instead of any_of Co-authored-by: Sumit Agarwal <[email protected]> * Add warning about future computation change for ConvTranspose with auto_pad (#11984) * Add warning about future computation change for Convtranspose with auto_pad * improve msg * update TODO to make lint happy * update more contents for warning and add if * valid was not infected * move it into kernel registration * parse auto_pad myself * try to use conv_transpose_attrs_.auto_pad directly * update roialign cuda impl to onnx opset16 (#12036) * roialign opset16 * fix * fix * Fix windows eager build break by pinning to torch version 1.11.0 (#12033) Fix windows and linux eager build to torch 1.11.0. * Skip Constant Folding for ops producing an optional type output (#11839) * Disable sequence-type tests since C# infra doesn't support well (#12037) * Extend lifetime of KernelDef when creating a standalone op (#12057) place tmp kernel def as local variable to cover the lifetime of kernel creation * Add targets files for new .net6 frameworks (#12016) * Add net6 targets. Remove maccatalyst as we don't have a native build targetting that. * Set platform in macos targets * Add targetFramework entries * Move NativeLib.DllName definition and set using preprocessor values for simplicity. Couldn't get it to build with the preprocessor based setup when it was in a separate file. Update the nuspec generation to set platform version for .net6 targets. TODO: Validate versions. I copied them from the managed nuget package the packaging pipeline generated prior to adding targets. Possibly w could/should lower some of the versions. Hopefully the need to specify a version goes away when the release version of VS2022 supports .net6. * Try android 31.1 as https://github.com/actions/virtual-environments/blob/main/images/win/Windows2022-Readme.md suggests that should be available on the CI machines * Fix patch version mismatch Add some extra debug info in case it helps * Debug nuget location in CI * Add workspace entry back in * Add steps * One more attempt with hardcoded nuget.exe path and original android31.0 version * Better fix - found explicit nuget download and updated version there. * flake8 fixes * Fix black complaints. * Exit Microsoft_ML_OnnxRuntime_CheckPrerequisites for net6 iOS. * Removed outdated comment * Fix DML custom operators which set descriptor heap to command list (#12059) * Make C# runtest.sh automatically set latest opset (#12039) * Update C# runtest.sh for opset 17 Should have been part of #11924 * get appropriate opset version from onnx doc * use absolute rather than relative path * fix typo in var name * Disable DML command list reuse for Xbox (#12063) disable cl reuse for xbox * Add data type check in ConvAddRelu fusion (#12058) * Add undocumented attribute to disable generation of Java bindings from the Android AAR. (#12075) The generated bindings causes C# build errors that require workaround code. Disabling generation should avoid the need for any workarounds. As the user has the C# ORT package with the C# to C bindings there's no need for binding generation that calls the ORT Java API (which is C# -> Java ->C). * enable the extensions custom build for java and android (#11823) * generate quantization parameter for outputs (#12089) * DML EP Update to DML 1.9 (#12090) * Update to DML 1.9 * Appease obnoxious Python formatting tool * Fix orttraining-linux-ci-pipeline - Symbolic shape infer (#11965) fix symbolic shape error due to upgraded numpy + legacy sympy * check consumers of dq node before swap dq and transpose (#12099) * check consumers of dq node before swap dq and transpose * add unit test Co-authored-by: Gary Miguel <[email protected]> Co-authored-by: Preetha Veeramalai <[email protected]> Co-authored-by: mayavijx <[email protected]> Co-authored-by: mohsin <[email protected]> Co-authored-by: Ye Wang <[email protected]> Co-authored-by: Dmitri Smirnov <[email protected]> Co-authored-by: G. Ramalingam <[email protected]> Co-authored-by: Dwayne Robinson <[email protected]> Co-authored-by: Sheil Kumar <[email protected]> Co-authored-by: Edward Chen <[email protected]> Co-authored-by: sumitsays <[email protected]> Co-authored-by: Sumit Agarwal <[email protected]> Co-authored-by: Chun-Wei Chen <[email protected]> Co-authored-by: George Wu <[email protected]> Co-authored-by: Wil Brady <[email protected]> Co-authored-by: Hariharan Seshadri <[email protected]> Co-authored-by: Wei-Sheng Chin <[email protected]> Co-authored-by: Scott McKay <[email protected]> Co-authored-by: Jeff Bloomfield <[email protected]> Co-authored-by: Justin Stoecker <[email protected]> Co-authored-by: Wenbing Li <[email protected]> Co-authored-by: Yufeng Li <[email protected]> Co-authored-by: pengwa <[email protected]>

gramalingam added 3 commits June 3, 2022 13:35

Rewrite function inliner

6aa4119

Fix merge conflict

a0ea33d

Fix overlooked merge conflict

5679db2

xadupre reviewed Jun 7, 2022

View reviewed changes

onnxruntime/core/graph/function_utils.cc Show resolved Hide resolved

xadupre reviewed Jun 7, 2022

View reviewed changes

onnxruntime/core/graph/function_utils.cc Show resolved Hide resolved

xadupre reviewed Jun 7, 2022

View reviewed changes

This was referenced Jun 8, 2022

Local function with If: GraphProto attribute inferencing is not enabled in this InferenceContextImpl instance. #10698

Closed

FunctionImpl should consider nested subgraphs when updating graph inputs/outputs #10876

Closed

souptc reviewed Jun 13, 2022

View reviewed changes

xadupre reviewed Jun 13, 2022

View reviewed changes

onnxruntime/core/graph/function_utils.cc Outdated Show resolved Hide resolved

gramalingam added 2 commits June 13, 2022 11:05

Add nested function call tests

be704d1

Add overload for Specialize

4f08762

liqunfu reviewed Jun 14, 2022

View reviewed changes

souptc previously approved these changes Jun 14, 2022

View reviewed changes

garymm added 9 commits June 15, 2022 20:04

Update C# generated protocol buffer code

6fab021

Update ONNX to 1.12

369bc17

Fixes #11640

dont change ONNX version in CI pipelines

273aabb

update onnx commit

53b84af

install latest onnx package

5cc9ca5

latest onnx 1.12 commit

3d496ea

DO NOT SUBMIT temporarily disabling opset 17 tests

fead0e0

onnx 1.12.0 rc4

7c97cf3

Merge branch 'onnx-1.12' into rama/fun-inliner

32d1e80

garymm added the release:1.12 label Jun 15, 2022

garymm dismissed souptc’s stale review via 32d1e80 June 16, 2022 19:00

Pass symboltable to onnx shape inference

9e5af64

Avoid renaming empty names

f279243

xadupre reviewed Jun 17, 2022

View reviewed changes

csharp/test/Microsoft.ML.OnnxRuntime.Tests.Common/OnnxMl.cs Show resolved Hide resolved

This was referenced Jun 21, 2022

Update ONNX to 1.12 #11642

Closed

Update ONNX to 1.12 #11924

Merged

Merge with master

11e1d12

garymm previously approved these changes Jun 23, 2022

View reviewed changes

garymm reviewed Jun 23, 2022

View reviewed changes

Enable sequence_map tests

3d666d9

gramalingam dismissed garymm’s stale review via 3d666d9 June 23, 2022 16:48

garymm approved these changes Jun 24, 2022

View reviewed changes

garymm merged commit b1411c8 into master Jun 24, 2022

garymm deleted the rama/fun-inliner branch June 24, 2022 16:21

RandySheriffH added the triage:approved label Jul 6, 2022

Conversation

gramalingam commented Jun 3, 2022

Uh oh!

lgtm-com bot commented Jun 4, 2022

Uh oh!

Uh oh!

Uh oh!

xadupre Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgtm-com bot commented Jun 13, 2022

Uh oh!

liqunfu Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garymm commented Jun 15, 2022

Uh oh!

lgtm-com bot commented Jun 16, 2022

Uh oh!

lgtm-com bot commented Jun 16, 2022

Uh oh!

Uh oh!

garymm commented Jun 22, 2022

Uh oh!

lgtm-com bot commented Jun 23, 2022

Uh oh!

garymm commented Jun 23, 2022

Uh oh!

garymm commented Jun 23, 2022

Uh oh!

garymm left a comment

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jun 23, 2022

Uh oh!

gramalingam commented Jun 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xadupre Jun 7, 2022 •

edited

Loading

liqunfu Jun 14, 2022 •

edited

Loading