Skip to content

Conversation

@jamesr66a
Copy link
Collaborator

@jamesr66a jamesr66a commented Nov 7, 2019

Stack from ghstack:

Fixes #29367

Differential Revision: D18380559

@jamesr66a jamesr66a requested a review from apaszke as a code owner November 7, 2019 21:57
jamesr66a pushed a commit that referenced this pull request Nov 7, 2019
ghstack-source-id: b56388f
Pull Request resolved: #29411
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Nov 7, 2019
@jamesr66a jamesr66a requested review from mruberry, suo and zdevito November 7, 2019 21:58
check_tolerance, _force_outplace, True, _module_class)

torch.jit._trace_module_map = None
old_module_map = torch.jit._trace_module_map
Copy link
Member

Choose a reason for hiding this comment

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

can we add a comment here? So next time we don't have to "wtf" our way through the code again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean it's pretty self explanatory now. The issue was discovering that this was needed in the first place :p

Copy link
Collaborator

Choose a reason for hiding this comment

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

the usual pattern to do such things is to only replace the attribute when you finish computing it, i.e., start with a local _trace_module_map and only do torch.jit._trace_module_map = _trace_module_map at the end.

Copy link
Member

@suo suo left a comment

Choose a reason for hiding this comment

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

ok boomer

@facebook-github-bot
Copy link
Contributor

@jamesr66a merged this pull request in 782e80e.

@facebook-github-bot facebook-github-bot deleted the gh/jamesr66a/141/head branch November 11, 2019 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants