Skip to content

Change specialization rules in GraphExecutors#10977

Closed
apaszke wants to merge 3 commits intopytorch:masterfrom
apaszke:jit_graph_executor
Closed

Change specialization rules in GraphExecutors#10977
apaszke wants to merge 3 commits intopytorch:masterfrom
apaszke:jit_graph_executor

Conversation

@apaszke
Copy link
Contributor

@apaszke apaszke commented Aug 28, 2018

Review last commit only. Stacked on top of #10949.

This commit fixes a number of issues connected to caching
differentiability status of graphs inside graph executors,
and changes the rules for optimization of differentiable subgraphs.
Previously every one of those was instantiated as a separate graph
executor, but now they are simply heavier-optimized graph regions,
and graph executors are only instantiated for their backward.

@zdevito

zdevito
zdevito approved these changes Aug 29, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I have minor comments below.

torch/csrc/jit/ir.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/interpreter.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/graph_executor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/graph_executor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/graph_executor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/graph_executor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@apaszke apaszke force-pushed the jit_graph_executor branch from 4054bd0 to 0a3ea2e Compare August 30, 2018 18:11
@apaszke apaszke force-pushed the jit_graph_executor branch from 090b773 to 6fc95a3 Compare August 30, 2018 19:35
@apaszke apaszke added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 30, 2018
@apaszke apaszke force-pushed the jit_graph_executor branch from 6fc95a3 to a3fcea0 Compare August 30, 2018 21:18
apaszke added 3 commits August 30, 2018 19:56
@apaszke
Change specialization rules in GraphExecutors
a69ad94 
This commit fixes a number of issues connected to caching
differentiability status of graphs inside graph executors,
and changes the rules for optimization of differentiable subgraphs.
Previously every one of those was instantiated as a separate graph
executor, but now they are simply heavier-optimized graph regions,
and graph executors are only instantiated for their backward.
@apaszke
Fix for AD issues
3f83300
@apaszke
Hopefully fix Windows build
21532b0
@apaszke apaszke force-pushed the jit_graph_executor branch from a3fcea0 to 21532b0 Compare August 31, 2018 03:03
facebook-github-bot
facebook-github-bot reviewed Aug 31, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apaszke has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@apaszke apaszke deleted the jit_graph_executor branch September 1, 2018 01:16
PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
@apaszke @PenghuiCheng
Change specialization rules in GraphExecutors (pytorch#10977)
f599697 
Summary:
**Review last commit only.** Stacked on top of pytorch#10949.

This commit fixes a number of issues connected to caching
differentiability status of graphs inside graph executors,
and changes the rules for optimization of differentiable subgraphs.
Previously every one of those was instantiated as a separate graph
executor, but now they are simply heavier-optimized graph regions,
and graph executors are only instantiated for their backward.

zdevito
Pull Request resolved: pytorch#10977

Differential Revision: D9600626

Pulled By: apaszke

fbshipit-source-id: dad09a0f586e396afbd5406319c1cd54fbb8a3d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@zdevito zdevito zdevito approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@anderspapitto anderspapitto Awaiting requested review from anderspapitto

@bddppq bddppq Awaiting requested review from bddppq

@dzhulgakov dzhulgakov Awaiting requested review from dzhulgakov

@houseroad houseroad Awaiting requested review from houseroad

@jamesr66a jamesr66a Awaiting requested review from jamesr66a

@smessmer smessmer Awaiting requested review from smessmer

@Yangqing Yangqing Awaiting requested review from Yangqing

Assignees

No one assigned

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@apaszke @zdevito @facebook-github-bot @ezyang