Changes to allow the export of functions with no user input. #8031

cptspacemanspiff · 2025-01-29T15:33:56Z

Summary

This is a set of changes that I have been using to get executorch working with Multiple Entrypoints with shared state.
This is related to: #8030

Issue & Changes

Currently trying to export a function:

def init():
  self.shared_state.fill_(0)

Will fail, because in to_edge in exir in the program.py the lifted constants are assumed to be placed before the first user input. This causes weird behavior when there are no user inputs. I changed it to place the lifted constant after the last buffer, if there are no user inputs to place before. However, this relies on my understanding of the implicit layout of the graph inputs.

At the same time later in the after the memory planning phase of to_executorch, it validates that memory planning was correct based on whether the graph_input_allocated flag is set, this only applies to user inputs, of which we have none, so it errors out. I added a check to bypass this error if there are no user inputs, but I honestly do not understand enough of the validation check to know if that is appropriate.

Comments

In the current executorch with no support for shared state, this case does not make sense, but #8030 is my attempt at adding that capability. and having initialization methods that init the buffers from constants is useful, especially since their initail state is undefined.

Currently this is not ready, I have no tests/ and have my random notes in the commit as comments, and other than validating that it worked as I was working on the shared state export have not done anything in depth....

But I kind-of want feedback if my solution seems correct, or if I am missing something. Particularly regarding my understanding of placeholder ordering and signature logic, and whether bypassing the graph_input_allocated validation is appropriate.

Out of all the changes I had to make for shared state, this is the one I am least sure about.

cc @JacobSzwejbka @angelayi

pytorch-bot · 2025-01-29T15:34:01Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

❌ 5 New Failures

As of commit d7284e9 with merge base 19a3002 ():

NEW FAILURES - The following jobs have failed:

Build documentation / build (buck2) / Build doc (gh)
RuntimeError: Command docker exec -t 55035cbf6d6e9b9d4205a2ce7bc235163b4c573ec0e73f0c78d4288c9cb0e4e5 /exec failed with exit code 2
pull / unittest / linux / linux-job (gh)
exir/backend/test/test_delegate_map_builder.py::TestDelegateMapBuilder::test_reinsert_delegate_debug_identifier
pull / unittest / macos / macos-job (gh)
exir/backend/test/test_delegate_map_builder.py::TestDelegateMapBuilder::test_reinsert_delegate_debug_identifier
pull / unittest-editable / linux / linux-job (gh)
exir/backend/test/test_delegate_map_builder.py::TestDelegateMapBuilder::test_reinsert_delegate_debug_identifier
pull / unittest-editable / macos / macos-job (gh)
exir/backend/test/test_delegate_map_builder.py::TestDelegateMapBuilder::test_reinsert_delegate_debug_identifier

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

JacobSzwejbka · 2025-01-29T20:48:03Z

@ydwu4 @angelayi can you guys take a look at this one

JacobSzwejbka · 2025-01-29T20:49:12Z

Hmm I thought we already supported functions with no inputs, but I guess before we only tested with constant returns. Now theres no user return either.

cc @tarun292 who I think worked on the no inputs thing before

JacobSzwejbka · 2025-01-29T20:54:12Z

exir/memory_planning.py

+        # only check if inputs are allocated if there are user inputs:
+        user_inputs_exist = len(list(filter(lambda input: input.kind == InputKind.USER_INPUT, self.graph_signature.input_specs))) > 0
+
+        if "placeholder" in check_list and user_inputs_exist:


this change looks fine. On our side we should probably take a pass over all the memory planning infra. A lot of it was written before export graphs were functionalized so the concept of non user inputs didnt make exist.

Like realistically we should /only/ be verifying the memory planning of user inputs here

angelayi · 2025-01-29T23:24:00Z

exir/program/_program.py

+        # this is a bit hacky, but I am not certain of what the contract is for
+        # node ordering. is the first non-placeholder node guranteed to be the
+        # first node after input paramters? what if there is no op, and it is
+        # just placeholders? Easier to just find the last buffer, and insert after.


I think it would be fine to leave it as None if there are no user inputs -- then it will insert it as the first placeholder. The official order of inputs is [parameters, buffers, constants, user inputs], but the order of the first 3 doesn't matter as much.

angelayi · 2025-01-30T00:50:49Z

exir/program/_program.py

@@ -306,6 +335,9 @@ def lift_constant_tensor_pass(ep):
            new_input_specs.extend(lifted_constants)


And if we just insert all the constants at the beginning, we can just directly prepend to input_specs.

done, and removed all the logic I had previously, now:

No user inputs -> inserting before None -> inserts at the beginning.

angelayi · 2025-01-30T00:51:29Z

exir/program/_program.py

@@ -306,6 +335,9 @@ def lift_constant_tensor_pass(ep):
            new_input_specs.extend(lifted_constants)
            lifted_constants.clear()
        new_input_specs.append(s)


Can you add a test case?

I added a test case, shifted to the simpler implementation of just prepending the signature since the order of the non-user inputs does not matter.

I have also validated that it works with my export process as well.

Simplified code, In the case of no user input lifted constants are placed before 'none' node, aka the begining. We update the graph node to place these at the beginning as well. fixed formatting.

angelayi · 2025-02-11T19:12:35Z

exir/tests/test_passes.py

+            "b_a"  # followed by the buffer input.
+        ).run(ep.graph_module.code)
+        # the graph signature should also be the same:
+        assert ep.graph_signature.input_specs[0].arg.name == "_lifted_tensor_constant1"


Suggested change

assert ep.graph_signature.input_specs[0].arg.name == "_lifted_tensor_constant1"

self.assertEquals(ep.graph_signature.input_specs[0].arg.name, "_lifted_tensor_constant1")

angelayi · 2025-02-11T19:12:52Z

exir/tests/test_passes.py

+        # executorch_program.graph_signature.input_specs[0].arg.name == "_lifted_tensor_constant1"
+        # executorch_program.graph_signature.input_specs[1].arg.name == "b_a"


why are these commented out?

I accidentally left those in, when I was initially writing the test/debugging and validating changes I just copy/pasted those lines so I had something to drop a debugger breakpoint on.

I removed those, and left in the to_executorch call, this is there to ensure that the validation steps that were previously failing are correctly bypassed. (Does not throw an exception.)

cptspacemanspiff · 2025-02-28T13:35:55Z

@angelayi

angelayi

thanks!!

cptspacemanspiff · 2025-03-07T12:55:59Z

@angelayi So I am not sure what else needs to happen to merge this in (I think I might have messed up the workflow by merging in main to fix some of the CI tests because my branch was out of date)

JacobSzwejbka · 2025-03-10T17:54:30Z

Im rerunning CI and then assuming it passes Ill merge it

shoumikhin · 2025-03-17T03:28:27Z

@cptspacemanspiff any updates? Do we still want this change?
cc @JacobSzwejbka

swolchok · 2025-03-17T17:12:44Z

looks like this broke unittest-release, build documentation, and perhaps other tests?

@JacobSzwejbka

### Summary CI is broken due to #8031. `graph_signature` is optional, and is None. ### Test plan CI cc @JacobSzwejbka @angelayi

@JacobSzwejbka

…#8031) ### Summary This is a set of changes that I have been using to get executorch working with Multiple Entrypoints with shared state. This is related to: pytorch#8030 ### Issue & Changes Currently trying to export a function: ```python def init(): self.shared_state.fill_(0) ``` Will fail, because in to_edge in exir in the program.py the lifted constants are assumed to be placed before the first user input. This causes weird behavior when there are no user inputs. I changed it to place the lifted constant after the last buffer, if there are no user inputs to place before. However, this relies on my understanding of the implicit layout of the graph inputs. At the same time later in the after the memory planning phase of to_executorch, it validates that memory planning was correct based on whether the `graph_input_allocated` flag is set, this only applies to user inputs, of which we have none, so it errors out. I added a check to bypass this error if there are no user inputs, but I honestly do not understand enough of the validation check to know if that is appropriate. ### Comments In the current executorch with no support for shared state, this case does not make sense, but pytorch#8030 is my attempt at adding that capability. and having initialization methods that init the buffers from constants is useful, especially since their initail state is undefined. Currently this is not ready, I have no tests/ and have my random notes in the commit as comments, and other than validating that it worked as I was working on the shared state export have not done anything in depth.... But I kind-of want feedback if my solution seems correct, or if I am missing something. Particularly regarding my understanding of placeholder ordering and signature logic, and whether bypassing the graph_input_allocated validation is appropriate. Out of all the changes I had to make for shared state, this is the one I am least sure about. cc @JacobSzwejbka @angelayi

@JacobSzwejbka

### Summary CI is broken due to pytorch#8031. `graph_signature` is optional, and is None. ### Test plan CI cc @JacobSzwejbka @angelayi

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 29, 2025

cptspacemanspiff mentioned this pull request Jan 29, 2025

Multiple Entrypoints and Shared State implementation; Request for comments/feedback. #8030

Open

JacobSzwejbka added the topic: not user facing label Jan 29, 2025

JacobSzwejbka reviewed Jan 29, 2025

View reviewed changes

manuelcandales added module: exir Issues related to Export IR and the code under exir/ triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Jan 29, 2025

manuelcandales assigned angelayi Jan 29, 2025

manuelcandales requested a review from angelayi January 29, 2025 21:29

angelayi reviewed Jan 30, 2025

View reviewed changes

WIP need to clean up functions.

a563d8a

cptspacemanspiff force-pushed the fix-lifted-constants-with-no-user-input branch from bb07bb0 to a563d8a Compare February 6, 2025 20:45

Added unit test.

0f1bc67

Simplified code, In the case of no user input lifted constants are placed before 'none' node, aka the begining. We update the graph node to place these at the beginning as well. fixed formatting.

angelayi reviewed Feb 11, 2025

View reviewed changes

Cleaned up test.

36f9903

angelayi approved these changes Feb 28, 2025

View reviewed changes

Merge branch 'main' into fix-lifted-constants-with-no-user-input

d7284e9

cptspacemanspiff requested review from tarun292 and larryliu0820 as code owners March 1, 2025 12:26

byjlw merged commit 8d4ba1d into pytorch:main Mar 17, 2025
45 of 50 checks passed

jathu mentioned this pull request Mar 17, 2025

Fix optional graph_signature access in memory planning #9325

Merged

jathu added a commit that referenced this pull request Mar 17, 2025

Fix optional graph_signature access in memory planning (#9325)

77752a4

### Summary CI is broken due to #8031. `graph_signature` is optional, and is None. ### Test plan CI cc @JacobSzwejbka @angelayi

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to allow the export of functions with no user input. #8031

Changes to allow the export of functions with no user input. #8031

cptspacemanspiff commented Jan 29, 2025 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Jan 29, 2025 •

edited

Loading

JacobSzwejbka commented Jan 29, 2025

JacobSzwejbka commented Jan 29, 2025

JacobSzwejbka Jan 29, 2025 •

edited

Loading

angelayi Jan 29, 2025

angelayi Jan 30, 2025

cptspacemanspiff Feb 8, 2025

angelayi Jan 30, 2025

cptspacemanspiff Feb 8, 2025

cptspacemanspiff Feb 8, 2025

angelayi Feb 11, 2025

angelayi Feb 11, 2025

cptspacemanspiff Feb 11, 2025

cptspacemanspiff commented Feb 28, 2025

angelayi left a comment

cptspacemanspiff commented Mar 7, 2025

JacobSzwejbka commented Mar 10, 2025

shoumikhin commented Mar 17, 2025

swolchok commented Mar 17, 2025

		@@ -306,6 +335,9 @@ def lift_constant_tensor_pass(ep):
		new_input_specs.extend(lifted_constants)

	assert ep.graph_signature.input_specs[0].arg.name == "_lifted_tensor_constant1"
	self.assertEquals(ep.graph_signature.input_specs[0].arg.name, "_lifted_tensor_constant1")

		# executorch_program.graph_signature.input_specs[0].arg.name == "_lifted_tensor_constant1"
		# executorch_program.graph_signature.input_specs[1].arg.name == "b_a"

Changes to allow the export of functions with no user input. #8031

Changes to allow the export of functions with no user input. #8031

Conversation

cptspacemanspiff commented Jan 29, 2025 • edited by pytorch-bot bot Loading

Summary

Issue & Changes

Comments

pytorch-bot bot commented Jan 29, 2025 • edited Loading

🔗 Helpful Links

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

❌ 5 New Failures

JacobSzwejbka commented Jan 29, 2025

JacobSzwejbka commented Jan 29, 2025

JacobSzwejbka Jan 29, 2025 • edited Loading

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

cptspacemanspiff commented Feb 28, 2025

angelayi left a comment

Choose a reason for hiding this comment

cptspacemanspiff commented Mar 7, 2025

JacobSzwejbka commented Mar 10, 2025

shoumikhin commented Mar 17, 2025

swolchok commented Mar 17, 2025

cptspacemanspiff commented Jan 29, 2025 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented Jan 29, 2025 •

edited

Loading

JacobSzwejbka Jan 29, 2025 •

edited

Loading