Skip to content

[jit] Resolve with closed over variables instead of stack frame #22270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
driazati wants to merge 2 commits into from
Closed

[jit] Resolve with closed over variables instead of stack frame #22270

driazati wants to merge 2 commits into from

Conversation

@driazati
Copy link
Contributor

@driazati driazati commented Jun 26, 2019 •
edited
Loading

Previously we looked at the stack frame of the function that called
script to resolve variables. This doesn't work if someone calls script
with a function defined somewhere else that references captured
variables. We already have a mechanism to look at the closed over
variables for a function, so this changes the rcb to use that.

Differential Revision: D16391346

@pytorchbot pytorchbot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jun 26, 2019
@driazati driazati changed the title [jit] Resolve closed over variables instead of stack [jit] Resolve with closed over variables instead of stack Jun 26, 2019
@driazati driazati changed the title [jit] Resolve with closed over variables instead of stack [jit] Resolve with closed over variables instead of stack frame Jun 26, 2019
@driazati driazati requested review from suo and wanchaol June 26, 2019 18:32
wanchaol
wanchaol approved these changes Jun 26, 2019
View reviewed changes
Copy link
Collaborator

@wanchaol wanchaol left a comment

Choose a reason for hiding this comment

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

Nice!

wanchaol
wanchaol reviewed Jun 26, 2019
View reviewed changes
@wanchaol
Copy link
Collaborator

wanchaol commented Jul 10, 2019

any updates on this?

@driazati
Copy link
Contributor Author

driazati commented Jul 10, 2019

It breaks resolution of type comment variables since they're not actually captured by the function since they're just strings. We could fix it by combining both methods of rcbs, but that seems kind of hacky. I'm not sure if there's a better workaround or not

@driazati driazati changed the base branch from master to driazati/rec/class July 17, 2019 17:34
@driazati driazati force-pushed the driazati/fixscope branch from b49decb to 759157b Compare July 17, 2019 17:35
@driazati driazati requested a review from wanchaol July 17, 2019 17:36
wanchaol
wanchaol approved these changes Jul 17, 2019
View reviewed changes
Copy link
Collaborator

@wanchaol wanchaol left a comment

Choose a reason for hiding this comment

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

looks good.

@driazati driazati changed the base branch from driazati/rec/class to master July 18, 2019 23:17
@driazati driazati force-pushed the driazati/fixscope branch from 24e147b to 2436514 Compare July 18, 2019 23:17
@pytorchbot pytorchbot added module: autograd Related to torch.autograd, and the autograd engine in general module: cuda Related to torch.cuda, and CUDA support in general oncall: distributed Add this issue/PR to distributed oncall triage queue module: docs Related to our documentation, both in docs/ and docblocks module: nccl Problems related to nccl support module: nn Related to torch.nn module: operators module: pybind Related to our Python bindings / interactions with other Python libraries module: typing Related to mypy type annotations labels Jul 18, 2019
@driazati driazati removed module: autograd Related to torch.autograd, and the autograd engine in general module: cuda Related to torch.cuda, and CUDA support in general oncall: distributed Add this issue/PR to distributed oncall triage queue module: docs Related to our documentation, both in docs/ and docblocks module: nccl Problems related to nccl support module: nn Related to torch.nn module: operators module: pybind Related to our Python bindings / interactions with other Python libraries module: typing Related to mypy type annotations labels Jul 18, 2019
@driazati driazati force-pushed the driazati/fixscope branch from 2436514 to cc5be3e Compare July 18, 2019 23:46
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 22, 2019

@driazati merged this pull request in 2891784.

@facebook-github-bot facebook-github-bot deleted the driazati/fixscope branch July 13, 2020 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wanchaol wanchaol wanchaol approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@driazati @wanchaol @facebook-github-bot @pytorchbot @mruberry