allow tensor alias for Loop's carried params by fs-eire · Pull Request #497 · microsoft/onnxruntime

fs-eire · 2019-02-20T20:20:28Z

Currently the memory allocator of all graph's outputs will not reuse any previous slots, regardless whether tensor alias can be applied. This change allows graph's output to reuse the allocated buffer, if:

the 2 tensors are identified as identical (alias)
the 2 tensors are the matching carried parameters of a Loop operator
the 2 tensors are input and output of a sub-graph (parent_node is not null)

skottmckay · 2019-02-20T21:05:24Z

My understanding is that the shape of carried dependencies can change between iterations for Loop (but must remain the same for Scan). Does this change handle that?

skottmckay · 2019-02-20T21:09:41Z

+            return graph_input_index >= 2 && graph_input_index == graph_output_index + 1;
+          } else {
+            // TODO: other operators like Scan
+          }


It would be better if this information is provided in a more generic manner so that we're not hardcoding special casing for a list of operators here.

e.g. something like the kernel def could be expanded and we lookup that information based on OpType.

Otherwise changes to operator specs (e.g. say Loop in opset 10 changes the order of things) are easily missed and the code becomes fragile.

I am fully agree with the point. However I think we need a design agreement before I make a change since this change may apply to the core type KernelOp.

skottmckay · 2019-02-20T21:11:36Z

class PlannerTest : public ::testing::Test {

You need unit tests here testing the new code and validating it's doing what is expected and ensuring future changes to the allocation planner don't accidentally break existing code.

Refers to: onnxruntime/test/framework/allocation_planner_test.cc:138 in 1d60776. [](commit_id = 1d60776, deletion_comment = False)

skottmckay · 2019-02-20T21:40:25Z

+          auto graph_input_index = std::distance(graph_inputs.cbegin(), it);
+          ORT_ENFORCE(graph_input_index >= 0 && static_cast<size_t>(graph_input_index) < graph_inputs.size());
+          auto graph_output_index = std::distance(graph_outputs.cbegin(),
+                                                  std::find(graph_outputs.cbegin(), graph_outputs.cend(), node.OutputDefs()[output_arg_num]));


node.OutputDefs()[output_arg_num]) [](start = 106, length = 34)

This should probably be outside the iteration so OutputDefs() and operator[] aren't called every time. Will probably get optimized out, but no need to rely on that.

skottmckay · 2019-02-20T22:04:41Z

+    // skip if this is main graph
+    if (parent_node_) {
+      // only check a single layer. ie. an Identity/Dropout node connecting graph's input and output
+      auto& graph_inputs = graph_viewer_.GetInputs();


Can you clarify this comment? Not quite sure what you mean by 'only check a single layer'. FindReusableInput looks at the alias and 'may inplace' maps and I'm not quite translating that into limiting the check to a 'single layer'.

This logic only checks one node instead of triversal the whole graph.

Triversal the whole graph will make this logic very complicated; for most of the cases, it's one Identity node connected the graph's input and output

A would expect the comment to be it just checks this graph rather than 'layer'. A graph could contain multiple layers of say RNNs, which I thought was a completely different concept.

Also I don't quite buy that 'most of the cases' will be one Identity node. If there's some state being carried between iterations, I would expect it is changed across each iteration and therefore there would be other nodes involved. I can understand unit test or very simple example models using an Identity node, but I wouldn't expect that in a real model.

In reply to: 258747489 [](ancestors = 258747489)

skottmckay

🕐

fs-eire · 2019-02-21T01:19:05Z

My understanding is that the shape of carried dependencies can change between iterations for Loop (but must remain the same for Scan). Does this change handle that?

If there is a change between those tensors, they will not labeled as alias.

gramalingam · 2019-02-21T01:23:00Z

        MLValueIndex reused;
-        if (std::find(graph_outputs.begin(), graph_outputs.end(), node_output) != graph_outputs.end()) {
+        if (std::find(graph_outputs.begin(), graph_outputs.end(), node_output) != graph_outputs.end() &&
+            !FindReusableGraphInput(*pnode, output_arg_num, &reused)) {


I don't understand this fallthrough logic, because the lines below will invoke "FindReusableInput" again. (In this case, correctness depends on FindReusableGraph & FindReusableInput "finding" the same "reused".) I think it will be better to use a nested "if FindReusableGraphInput(…)" and do the right thing inside this if-statement than cascading to subsequent else-branches.

Hi, How about this comment? Can we change this to: if (this is a graph output) { if (FindReusableGraphInput(…)) Reuse(reused, current) else ...kAllocateOutput" ?

gramalingam · 2019-02-21T19:26:32Z

The high-level goal of optimizing the static allocation planning for loops/scan is a good one. There is definitely scope for improving reuse across loop iterations. But it is also somewhat complex. For example: the assumption that it is safe to reuse the memory of an (loop carried) input-state for the corresponding output-state may not always hold. Consider a Loop statement with an initial-value X for some loop-carried dependency P. The tensor X is "owned" by the caller of the Loop (eg., the main graph executor). If this tensor X is used by some node after the Loop, then it would be a problem if the Loop's internal execution overwrites X (by reusing it). We need to make sure that the "ownership protocol" is generalized correctly to enable this kind of optimization, keeping in mind here that we have multiple parties involved: the "main graph executor", which calls the "Loop implementation", which in turn calls the "subgraph executor". We have to be clear "who owns which tensor".

gramalingam · 2019-02-22T02:12:32Z

Please ignore my comment above. I spoke with Yulong. As I understand it, the goal of this PR is to target a specific scenario where a loop copies some state-in parameter to state-out, and goal is to avoid the copy (and not just reuse of memory). Another solution for this scenario would be to have a graph-rewrite-rule optimization that can transform such a loop to eliminate such a parameter (which can be directly referenced as an outer-scope variable inside the loop body). But writing this optimization is also a non-trivial amount of work.

gramalingam · 2019-02-22T22:49:20Z

+      auto& graph_inputs = graph_viewer_.GetInputs();
+      auto& graph_outputs = graph_viewer_.GetOutputs();
+
+      if (FindReusableInput(node, output_arg_num, reusable_input)) {


Okay, after looking through this again, there are still some edge cases where this can be a problem. First, FindReusableInput looks for opportunities for both Alias (identity copy operations) and MayInplace (where we use destructive in-place updates). So, for example, if we have X_out = ADD (X_in, Y), and "ADD" has MayInplace, this optimization will kick in. Do we want that? On one hand, it seems useful when it is valid. On the other hand, there are some cases where it is invalid. E.g., if the initial-value of "X" is some constant tensor C defined in an initializer, this will end up overwriting that constant tensor C. So, at the least, it looks like some extra checks will be needed.

…sor-alias

## Describe your changes Pin azureml-fsspec to unblock our ci pipeline. ![image](https://github.com/microsoft/Olive/assets/13343117/3a611194-a40b-4380-963e-32bc3f63cc1a) ## Checklist before requesting a review - [ ] Add unit tests for this change. - [ ] Make sure all tests can pass. - [ ] Update documents if necessary. - [ ] Format your code by running `pre-commit run --all-files` - [ ] Is this a user-facing change? If yes, give a description of this change to be included in the release notes. ## (Optional) Issue link

## Describe your changes 1. This reverts commit 9fd6df3. As the bug is fix in azureml-dataprep 4.12.1 https://pypi.org/project/azureml-dataprep/4.12.1/ 2. Remove azureml-defaults to avoid the hanging on aml env building. ## Checklist before requesting a review - [ ] Add unit tests for this change. - [ ] Make sure all tests can pass. - [ ] Update documents if necessary. - [ ] Format your code by running `pre-commit run --all-files` - [ ] Is this a user-facing change? If yes, give a description of this change to be included in the release notes. ## (Optional) Issue link

allow tensor alias for Loop's carried params

1d60776

fs-eire requested a review from a team as a code owner February 20, 2019 20:20

skottmckay requested a review from gramalingam February 20, 2019 20:54

skottmckay reviewed Feb 20, 2019

View reviewed changes

skottmckay requested changes Feb 20, 2019

View reviewed changes

gramalingam reviewed Feb 21, 2019

View reviewed changes

gramalingam reviewed Feb 22, 2019

View reviewed changes

fs-eire added 2 commits March 6, 2019 11:53

Merge remote-tracking branch 'ms/master' into allow-loop-subgraph-ten…

2360bae

…sor-alias

generalize the allocation conditions

0914ff4

skottmckay mentioned this pull request Mar 15, 2019

Avoid copy of pre-existing value to subgraph output #637

Merged

skottmckay closed this Apr 4, 2019

raymondxyang deleted the allow-loop-subgraph-tensor-alias branch April 26, 2019 07:43

Uh oh!

Conversation

fs-eire commented Feb 20, 2019

Uh oh!

skottmckay commented Feb 20, 2019

Uh oh!

skottmckay Feb 20, 2019 • 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!

fs-eire Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

skottmckay commented Feb 20, 2019

Uh oh!

skottmckay Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

skottmckay Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

fs-eire Feb 21, 2019 • 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!

skottmckay Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

skottmckay left a comment

Choose a reason for hiding this comment

Uh oh!

fs-eire commented Feb 21, 2019

Uh oh!

gramalingam Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

gramalingam Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

gramalingam commented Feb 21, 2019

Uh oh!

gramalingam commented Feb 22, 2019

Uh oh!

gramalingam Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

skottmckay Feb 20, 2019 •

edited

Loading

fs-eire Feb 21, 2019 •

edited

Loading