[SR] cover static runtime for ops that uses getitem instead of listunpack #124148

lakshmananrm1993 · 2024-04-16T05:44:08Z

Summary: cover static runtime for ops that uses getitem instead of listunpack

Differential Revision: D55924256

pytorch-bot · 2024-04-16T05:44:11Z

This appears to be a diff that was exported from phabricator, but the PR author does not have sufficient permissions to run CI. @lakshmananrm1993, please do step 2 of internal wiki to get write access so you do not need to get CI approvals in the future. If you think this is a mistake, please contact the Pytorch Dev Infra team.

linux-foundation-easycla · 2024-04-16T05:44:12Z

The committers listed above are authorized under a signed CLA.

✅ login: lakshmananrm1993 (e7bbf7d)

pytorch-bot · 2024-04-16T05:44:14Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/124148

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (2 Unrelated Failures)

As of commit e7bbf7d with merge base 402b289 ():

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

pull / linux-focal-py3_8-clang9-xla / test (xla, 1, 1, linux.12xlarge) (gh) (similar failure)
test_all_reduce_no_op_with_one_replica
pull / linux-jammy-py3.8-gcc11 / test (distributed, 2, 2, linux.2xlarge) (gh) (similar failure)
Process completed with exit code 1.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

facebook-github-bot · 2024-04-16T05:44:32Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-04-16T05:45:07Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-04-16T20:12:32Z

This pull request was exported from Phabricator. Differential Revision: D55924256

swolchok

overall concerned about it being easier to create cases it looks like this code didn't consider using __getitem__

swolchok · 2024-04-16T20:42:58Z

torch/csrc/jit/runtime/static/passes.cpp

-    Node* list_unpack_node = value_out->uses()[0].user;
-    if (list_unpack_node->kind() != prim::ListUnpack) {
+    Node* user_node = value_out->uses()[0].user;
+    if (user_node->kind() != prim::ListUnpack && user_node->kind() != aten::__getitem__) {
      continue;
    }


this worries me. 1) what if there is more than one __getitem__? as written it looks like we will just leave the other ones around?
2) what if the number of __getitem__ calls in the input program doesn't match the size of the output list?

i changed it to work only fb::equally_split. This has an argument num_splits and I am using that to do all the following checks

same as number of uses of this module

all the uses are get_item

get_item uses is not duplicated

only after this I do the fuse

torch/csrc/jit/runtime/static/passes.cpp

swolchok · 2024-04-16T20:45:08Z

torch/csrc/jit/runtime/static/passes.cpp

+
+    std::vector<std::tuple<Node*, ArrayRef<Value*>>> user_to_remove;
+
+    for(auto index = 0; index < value_out->uses().size(); index++) {


use c10::irange

torch/csrc/jit/runtime/static/passes.cpp

facebook-github-bot · 2024-04-20T00:18:55Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-04-20T17:56:02Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-04-20T18:21:18Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-04-20T18:27:03Z

This pull request was exported from Phabricator. Differential Revision: D55924256

lakshmananrm1993 · 2024-04-22T03:14:34Z

overall concerned about it being easier to create cases it looks like this code didn't consider using __getitem__

I have changed it to cover the scenarios. I do the fuse only if the following validations succeed and I changed it to work only for equally_split

same as number of uses of this module
all the uses are get_item
get_item uses is not duplicated

swolchok · 2024-04-22T18:05:18Z

torch/csrc/jit/runtime/static/passes.cpp

+
+    const auto num_splits_ivalue = toIValue(value_num_splits);
+
+    // check if equally_split num of splits is 1.


this comment repeats the code; delete

swolchok · 2024-04-22T18:05:51Z

torch/csrc/jit/runtime/static/passes.cpp

+// Its only for fb::equally_split operator
+// the operators results are being accessed by get_item rather than ListUnpack
+void FuseEquallySplitGetItemUnpack(std::shared_ptr<torch::jit::Graph>& graph) {
+  // replacement contains (old_node, new_node, list_unpack_node)


stale comment, delete

swolchok · 2024-04-22T18:07:07Z

torch/csrc/jit/runtime/static/passes.cpp

+    const Value* value_num_splits = node->inputs()[1];
+
+    // check if equally_split num of splits is equal to the number of output uses
+    if(!toIValue(value_num_splits).has_value() || !toIValue(value_num_splits).value().isInt() || toIValue(value_num_splits).value().toInt() != value_out->uses().size()) {


don't call toIValue repeatedly; we should not be requiring LTO for good performance

replaced it with constant_as()

swolchok · 2024-04-22T18:07:17Z

torch/csrc/jit/runtime/static/passes.cpp

+
+    const Value* value_num_splits = node->inputs()[1];
+
+    // check if equally_split num of splits is equal to the number of output uses


comment repeats the code; delete

swolchok · 2024-04-22T18:07:48Z

torch/csrc/jit/runtime/static/passes.cpp

+        break;
+      }
+
+      auto getItem_indice_out = toIValue(user->inputs()[1]);


this should be getItem_index_out

use constant_as<int>() as recommended above

swolchok · 2024-04-22T19:22:45Z

torch/csrc/jit/runtime/static/passes.cpp

+    bool should_fuse = true;
+
+    std::vector<UserToRemove> user_to_remove_list;
+
+    std::unordered_set<std::int64_t> getItem_indices;
+
+    for(auto index = 0; index < value_out->uses().size(); index++) {


nit: please do not put so many blank lines; keep style consistent with the surrounding code

removed the not needed blank lines

swolchok · 2024-04-22T19:25:38Z

torch/csrc/jit/runtime/static/passes.cpp

+
+    std::vector<UserToRemove> user_to_remove_list;
+
+    std::unordered_set<std::int64_t> getItem_indices;


use c10::FastSet

swolchok · 2024-04-22T19:26:31Z

torch/csrc/jit/runtime/static/passes.cpp

+
+      LOG(INFO) << "Found fb::equally_split node2: " << user->kind().toQualString();
+
+      getItem_indice_out.emplace(getItem_indice_out);


this does nothing

it should have been getItem_Indices. Updated it now

swolchok · 2024-04-22T19:26:40Z

torch/csrc/jit/runtime/static/passes.cpp

+        break;
+      }
+
+      LOG(INFO) << "Found fb::equally_split node2: " << user->kind().toQualString();


why node2 and not just node?

this was testing comment. removed it

swolchok · 2024-04-22T19:27:35Z

torch/csrc/jit/runtime/static/passes.cpp

+  for(const auto& replacement : replacement_list) {
+    auto* new_node = graph->create(replacement.new_sym, 0);
+    for (Value* in : replacement.node_to_be_fused->inputs()) {
+        new_node->addInput(in);
+      }
+    for(const auto& user_to_remove_node : replacement.user_to_remove) {
+      for (Value* out : user_to_remove_node.user_outputs) {
+        Value* new_out = new_node->addOutput();
+        new_out->copyMetadata(out);
+        out->replaceAllUsesWith(new_out);
+      }
+      user_to_remove_node.user->destroy();
+    }
+    new_node->insertAfter(replacement.node_to_be_fused);
+    replacement.node_to_be_fused->destroy();
+  }


it would be easier for me to review this for correctness if it ended up looking the same as the FuseListUnpack path. If you need to send a separate stacked PR to get rid of the std::tuple in FuseListUnpack, go for it

facebook-github-bot · 2024-05-03T23:18:32Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-05-23T02:44:24Z

This pull request was exported from Phabricator. Differential Revision: D55924256

…pack (pytorch#124148) Summary: cover static runtime for ops that uses getitem instead of listunpack. Reviewed By: snabelkabiya, swolchok Differential Revision: D55924256

facebook-github-bot · 2024-05-23T04:34:01Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-06-03T22:23:39Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-06-03T22:27:00Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-06-04T01:22:25Z

This pull request was exported from Phabricator. Differential Revision: D55924256

…pack (pytorch#124148) Summary: cover static runtime for ops that uses getitem instead of listunpack. Reviewed By: snabelkabiya, swolchok Differential Revision: D55924256

facebook-github-bot · 2024-06-04T01:51:52Z

This pull request was exported from Phabricator. Differential Revision: D55924256

…pack (pytorch#124148) Summary: cover static runtime for ops that uses getitem instead of listunpack. Reviewed By: snabelkabiya, swolchok Differential Revision: D55924256

facebook-github-bot · 2024-06-04T05:20:50Z

This pull request was exported from Phabricator. Differential Revision: D55924256

facebook-github-bot · 2024-06-10T23:58:36Z

This pull request was exported from Phabricator. Differential Revision: D55924256

…pack (pytorch#124148) Summary: Pull Request resolved: pytorch#124148 cover static runtime for ops that uses getitem instead of listunpack. Reviewed By: snabelkabiya, swolchok Differential Revision: D55924256

facebook-github-bot · 2024-06-11T06:50:45Z

This pull request was exported from Phabricator. Differential Revision: D55924256

github-actions · 2024-08-10T09:33:34Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

pytorch-bot bot added the release notes: jit release notes category label Apr 16, 2024

facebook-github-bot added the fb-exported label Apr 16, 2024

lakshmananrm1993 force-pushed the export-D55924256 branch from 737e3a7 to e195b9c Compare April 16, 2024 05:45

lakshmananrm1993 changed the title ~~[SR] fix SR for op gb::equally_split for jit scripted model~~ [SR] cover static runtime for ops that uses getitem instead of listtupleunpack Apr 16, 2024

lakshmananrm1993 changed the title ~~[SR] cover static runtime for ops that uses getitem instead of listtupleunpack~~ [SR] cover static runtime for ops that uses getitem instead of listunpack Apr 16, 2024

lakshmananrm1993 force-pushed the export-D55924256 branch from e195b9c to 12bbced Compare April 16, 2024 20:12

swolchok requested changes Apr 16, 2024

View reviewed changes

lakshmananrm1993 force-pushed the export-D55924256 branch from 12bbced to 5d382e8 Compare April 20, 2024 00:18

lakshmananrm1993 force-pushed the export-D55924256 branch from 5d382e8 to b5de8c0 Compare April 20, 2024 17:55

lakshmananrm1993 force-pushed the export-D55924256 branch from b5de8c0 to 46d83ce Compare April 20, 2024 18:21

lakshmananrm1993 force-pushed the export-D55924256 branch from 46d83ce to a2632cc Compare April 20, 2024 18:26

swolchok requested changes Apr 22, 2024

View reviewed changes

lakshmananrm1993 force-pushed the export-D55924256 branch from a2632cc to b186caa Compare May 3, 2024 23:18

lakshmananrm1993 force-pushed the export-D55924256 branch from 0afc7ba to a98d4f2 Compare May 23, 2024 02:44

lakshmananrm1993 force-pushed the export-D55924256 branch from a98d4f2 to 6939277 Compare May 23, 2024 04:33

lakshmananrm1993 force-pushed the export-D55924256 branch from 6939277 to 670ee82 Compare June 3, 2024 22:23

lakshmananrm1993 force-pushed the export-D55924256 branch from 670ee82 to dd9ac1c Compare June 3, 2024 22:26

lakshmananrm1993 force-pushed the export-D55924256 branch from dd9ac1c to 57694c3 Compare June 4, 2024 01:22

lakshmananrm1993 force-pushed the export-D55924256 branch from 57694c3 to 6a2d81c Compare June 4, 2024 01:51

lakshmananrm1993 force-pushed the export-D55924256 branch from 6a2d81c to 9be6709 Compare June 4, 2024 05:20

lakshmananrm1993 force-pushed the export-D55924256 branch from 9be6709 to 8721217 Compare June 10, 2024 23:58

[SR] cover static runtime for ops that uses getitem instead of listun…

e7bbf7d

…pack (pytorch#124148) Summary: Pull Request resolved: pytorch#124148 cover static runtime for ops that uses getitem instead of listunpack. Reviewed By: snabelkabiya, swolchok Differential Revision: D55924256

lakshmananrm1993 force-pushed the export-D55924256 branch from 8721217 to e7bbf7d Compare June 11, 2024 06:50

github-actions bot added the Stale label Aug 10, 2024

github-actions bot closed this Sep 9, 2024


		std::vector<std::tuple<Node, ArrayRef<Value>>> user_to_remove;

		for(auto index = 0; index < value_out->uses().size(); index++) {


		const auto num_splits_ivalue = toIValue(value_num_splits);

		// check if equally_split num of splits is 1.


		const Value* value_num_splits = node->inputs()[1];

		// check if equally_split num of splits is equal to the number of output uses


		std::vector<UserToRemove> user_to_remove_list;

		std::unordered_set<std::int64_t> getItem_indices;


		LOG(INFO) << "Found fb::equally_split node2: " << user->kind().toQualString();

		getItem_indice_out.emplace(getItem_indice_out);

[SR] cover static runtime for ops that uses getitem instead of listunpack #124148

[SR] cover static runtime for ops that uses getitem instead of listunpack #124148

Conversation

lakshmananrm1993 commented Apr 16, 2024 • edited Loading

pytorch-bot bot commented Apr 16, 2024

linux-foundation-easycla bot commented Apr 16, 2024 • edited Loading

pytorch-bot bot commented Apr 16, 2024 • edited Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/124148

✅ You can merge normally! (2 Unrelated Failures)

facebook-github-bot commented Apr 16, 2024

facebook-github-bot commented Apr 16, 2024

facebook-github-bot commented Apr 16, 2024

swolchok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 20, 2024

facebook-github-bot commented Apr 20, 2024

facebook-github-bot commented Apr 20, 2024

facebook-github-bot commented Apr 20, 2024

lakshmananrm1993 commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented May 3, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented May 23, 2024

facebook-github-bot commented Jun 3, 2024

facebook-github-bot commented Jun 3, 2024

facebook-github-bot commented Jun 4, 2024

facebook-github-bot commented Jun 4, 2024

facebook-github-bot commented Jun 4, 2024

facebook-github-bot commented Jun 10, 2024

facebook-github-bot commented Jun 11, 2024

github-actions bot commented Aug 10, 2024

lakshmananrm1993 commented Apr 16, 2024 •

edited

Loading

linux-foundation-easycla bot commented Apr 16, 2024 •

edited

Loading

pytorch-bot bot commented Apr 16, 2024 •

edited

Loading