[FX] Make Graphs immutable and make GraphModule recompile after assigning graph #44830

jamesr66a · 2020-09-16T21:44:34Z

Stack from ghstack:

[FX] Make Graphs immutable and make GraphModule recompile after assigning graph #44830 [FX] Make Graphs immutable and make GraphModule recompile after assigning graph

Differential Revision: D23743850

[ghstack-poisoned]

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

zdevito

I don't think we should expose generate forward. It makes more sense that assignment to the .graph property automatically regenerates forward. graph itself should be immutable. This patch should also enforce the immutability of graph after it has had an output declared.

jamesr66a · 2020-09-16T22:04:57Z

@zdevito how do we enforce immutability of Graph? Subclass list and disable all methods except append and getitem or something?

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

ghstack-source-id: 21f609a Pull Request resolved: #44830

jamesr66a · 2020-09-16T23:03:18Z

@zdevito updated. I don't particularly like how this breaks editing after copy/deepcopy

dzhulgakov · 2020-09-16T23:28:57Z

torch/fx/graph.py

+            self.nodes : FinalizableList = FinalizableList()
+            val_map = {}
+            for node in from_graph.nodes:
+                val_map[node] = self.node_copy(node, lambda n: val_map[n])


btw, should node_copy be delegate-customizable too? I'm thinking of cases when extra metadata is hanging off the node and we want to copy it

Delegate is now Tracer and only affects how tracing occurs

Then what is the recommended mechanism fo attaching more stuff to nodes in the graph and preserving them? Should we copy entries in the __dict__ (though it's a bit hacky)

Maybe we could add extra_info : Dict[str, Any] to Node that gets copied over?

dzhulgakov · 2020-09-16T23:29:27Z

torch/fx/graph.py

    def output(self, result: Argument):
        self.result = result
        self._mark_uses(result)
+        self.nodes.finalize()


btw, have you considered just doing self.nodes = tuple(self.nodes)? :) What is the benefit of FinalizableList over Tuple?

Every time you append to the tuple you'd have to create a new data structure? Also that doesn't match with the specification above, of immutable after an output has been specified

I mean to change type from list to tuple only when you want to finalize. Afaiu, list and tuple are mostly indistinguishable from the consumption standpoint.

dr-ci · 2020-09-17T00:22:14Z

💊 CI failures summary and remediations

As of commit e38e222 (more details on the Dr. CI page):

✅ None of the CI failures appear to be your fault 💚

1/1 broken upstream at merge base 92f8f75 on Sep 21 from 11:10am to 2:09pm PDT (6 commits; 92f8f75 - c941dd3)

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

pytorch_xla_linux_bionic_py3_6_clang9_build on Sep 21 from 11:10am to 2:09pm PDT (6 commits; 92f8f75 - c941dd3)
- 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 29 times.

zdevito · 2020-09-17T20:03:20Z

torch/fx/graph.py

    else:
        return a

+class FinalizableList(list):


Not sure all this infrastructure is worth it. We can just change nodes to be a property and then editing nodes doesn't change the graph.

zdevito · 2020-09-17T20:06:19Z

torch/fx/graph.py

 class Graph:
-    def __init__(self):
-        self.nodes : List[Node] = []
+    def __init__(self, from_graph : Optional['Graph'] = None):


It's a big odd to put this in the constructor. A graph could have a function copy_graph similar to copy node that would copy/append the entire graph. Then someone wanting to append to a graph can:

b = Graph() a.copy_graph(b, input_fn) # same structure as copy node but copies an entire graph

Appending to a graph only is a specific use case, the general one is editing a graph (i.e. doing node-by-node transforms) and then splices graphs into eachother (graph_copy).

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

ghstack-source-id: bc1a53b Pull Request resolved: #44830

…after assigning graph" Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

ghstack-source-id: 2c2d7dd Pull Request resolved: #44830

…after assigning graph" Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

ghstack-source-id: a0d4a01 Pull Request resolved: #44830

…after assigning graph" Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

ghstack-source-id: 05a0c84 Pull Request resolved: #44830

facebook-github-bot · 2020-09-22T22:15:22Z

@jamesr66a merged this pull request in 79fe794.

[FX] Make forward regen public and add docstring

80895f0

[ghstack-poisoned]

jamesr66a mentioned this pull request Sep 16, 2020

[FX] Fix GraphModule copy methods not regenerating forward #44806

Closed

facebook-github-bot added the fx label Sep 16, 2020

Update on "[FX] Make forward regen public and add docstring"

37474e8

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

zdevito reviewed Sep 16, 2020

View reviewed changes

Update on "[FX] Make forward regen public and add docstring"

533ac2f

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

jamesr66a changed the title ~~[FX] Make forward regen public and add docstring~~ [FX] Make Graphs immutable after output is set Sep 16, 2020

Update on "[FX] Make Graphs immutable after output is set"

a3b6843

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

Update on "[FX] Make Graphs immutable after output is set"

2b4f00b

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

jamesr66a pushed a commit that referenced this pull request Sep 16, 2020

[FX] Make Graphs immutable after output is set

b0185ad

ghstack-source-id: 21f609a Pull Request resolved: #44830

dzhulgakov reviewed Sep 16, 2020

View reviewed changes

zdevito reviewed Sep 17, 2020

View reviewed changes

Update on "[FX] Make Graphs immutable after output is set"

c7c9b66

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

Update on "[FX] Make Graphs immutable after output is set"

3b39a97

Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

jamesr66a pushed a commit that referenced this pull request Sep 17, 2020

[FX] Make Graphs immutable after output is set

c40d246

ghstack-source-id: bc1a53b Pull Request resolved: #44830

jamesr66a changed the title ~~[FX] Make Graphs immutable after output is set~~ [FX] Make Graphs immutable and make GraphModule recompile after assigning graph Sep 17, 2020

Update on "[FX] Make Graphs immutable and make GraphModule recompile …

1358781

…after assigning graph" Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

Update on "[FX] Make Graphs immutable and make GraphModule recompile …

9da5cf9

…after assigning graph" Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

jamesr66a pushed a commit that referenced this pull request Sep 18, 2020

[FX] Make Graphs immutable after output is set

f7199bd

ghstack-source-id: 2c2d7dd Pull Request resolved: #44830

jamesr66a requested a review from zdevito September 18, 2020 20:45

zdevito approved these changes Sep 19, 2020

View reviewed changes

Update on "[FX] Make Graphs immutable and make GraphModule recompile …

b39c158

…after assigning graph" Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

jamesr66a pushed a commit that referenced this pull request Sep 19, 2020

[FX] Make Graphs immutable after output is set

f83b66c

ghstack-source-id: a0d4a01 Pull Request resolved: #44830

Update on "[FX] Make Graphs immutable and make GraphModule recompile …

e38e222

…after assigning graph" Differential Revision: [D23743850](https://our.internmc.facebook.com/intern/diff/D23743850) [ghstack-poisoned]

jamesr66a pushed a commit that referenced this pull request Sep 21, 2020

[FX] Make Graphs immutable after output is set

177deb3

ghstack-source-id: 05a0c84 Pull Request resolved: #44830

facebook-github-bot closed this in 79fe794 Sep 22, 2020

facebook-github-bot added the merged label Sep 22, 2020

facebook-github-bot deleted the gh/jamesr66a/285/head branch September 26, 2020 14:14

mruberry added the Merged label Oct 28, 2020

[FX] Make Graphs immutable and make GraphModule recompile after assigning graph #44830

[FX] Make Graphs immutable and make GraphModule recompile after assigning graph #44830

Uh oh!

Conversation

jamesr66a commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

jamesr66a commented Sep 16, 2020

Uh oh!

jamesr66a commented Sep 16, 2020

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!

Choose a reason for hiding this comment

Uh oh!

dr-ci bot commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🚧 1 fixed upstream failure:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jamesr66a commented Sep 16, 2020 •

edited

Loading

dr-ci bot commented Sep 17, 2020 •

edited

Loading