Skip to content

Set proper scope on nodes added by JIT#12400

Closed
orionr wants to merge 1 commit intopytorch:masterfrom
orionr:export-D10224380
Closed

Set proper scope on nodes added by JIT#12400
orionr wants to merge 1 commit intopytorch:masterfrom
orionr:export-D10224380

Conversation

@orionr
Copy link
Copy Markdown
Contributor

@orionr orionr commented Oct 5, 2018

Summary:
In order to support tensorboardX and other visualization tools, we need to make sure a non-empty scope is set on all nodes added by the JIT. This attempts to do this, but is still a WIP.

This is a new version of #10749

Differential Revision: D10224380

@orionr orionr changed the title Set proper scope on nodes added by JIT [WIP] Set proper scope on nodes added by JIT Oct 5, 2018
@orionr
Copy link
Copy Markdown
Contributor Author

orionr commented Oct 5, 2018

According to my testing I haven't been able to find all the places where scope needs to be set in the JIT rewrite. @apaszke and @zdevito any thoughts on how to find what places I overlooked? Thanks.

@orionr
Copy link
Copy Markdown
Contributor Author

orionr commented Oct 5, 2018

Also, for reference, this issue is being experienced at lanpa/tensorboardX#229

apaszke
apaszke approved these changes Oct 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@apaszke apaszke 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 so far!

@lanpa
Copy link
Copy Markdown
Collaborator

lanpa commented Oct 14, 2018

@orionr I opened a branch at
https://github.com/lanpa/tensorboardX/tree/fix-graph
which prints nodes that does not have a scope name.

Among the models I have tested, onnx::Constant, onnx::Gemm, onnx::Add need to be fixed.

minimal testcase:

class LinearLanpa(nn.Module):
    def __init__(self):
        super(LinearLanpa, self).__init__()
        self.l = nn.Linear(3, 5)

    def forward(self, x):
        return self.l(x)

dummy_input = (torch.zeros(1, 3),)
with SummaryWriter(comment='LinearModel') as w:
    w.add_graph(LinearLanpa(), dummy_input, True)



result with torch 1.0.0a0+0ac865e:

graph(%0 : Float(1, 3)
      %1 : Float(5, 3)
      %2 : Float(5)) {
  %3 : Dynamic = onnx::Constant[value={0}]()
  %4 : Dynamic = onnx::Gemm[alpha=1, beta=0, transB=1](%0, %1, %3)
  %5 : Float(1, 5) = onnx::Add(%2, %4)
  return (%5);
}

%3 : Dynamic = onnx::Constant[value={0}]()
 has empty scope name. FIXME!
%4 : Dynamic = onnx::Gemm[alpha=1, beta=0, transB=1](%0, %1, %3)
 has empty scope name. FIXME!
%5 : Float(1, 5) = onnx::Add(%2, %4)
 has empty scope name. FIXME!

lanpa
lanpa reviewed Oct 15, 2018
View reviewed changes
Comment thread torch/csrc/jit/ir.h Outdated
return name_;
}
std::string namesFromRoot(const std::string& separator="/") {
// TODO: I think the answer is we shouldn't have used Symbol here

This comment was marked as off-topic.

Sign in to view

@orionr
Copy link
Copy Markdown
Contributor Author

orionr commented Oct 15, 2018

I had a good conversation with @zdevito and we actually want to move the scope into metadata via intrusive pointer rather than keeping it separate like this. I haven't had cycles to move this forward, though, so it will likely take a while. Anybody else interested in taking it on?

@lanpa
Copy link
Copy Markdown
Collaborator

lanpa commented Oct 17, 2018

Just send a PR to orionr:export-D10224380. It should fix the issue I mentioned.

@orionr orionr changed the title [WIP] Set proper scope on nodes added by JIT Set proper scope on nodes added by JIT Oct 17, 2018
facebook-github-bot
facebook-github-bot reviewed Oct 17, 2018
View reviewed changes
Copy link
Copy Markdown
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.

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

@orionr
Copy link
Copy Markdown
Contributor Author

orionr commented Oct 17, 2018

Failures are intermittent and not caused by this PR. Landing.

zdevito
zdevito previously requested changes Oct 17, 2018
View reviewed changes
Comment thread torch/csrc/jit/passes/onnx.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@orionr orionr dismissed zdevito’s stale review October 22, 2018 19:38

Confirm that converting to a ScopePtr via intrusive ptr takes care of the concern here.

facebook-github-bot
facebook-github-bot reviewed Oct 22, 2018
View reviewed changes
Copy link
Copy Markdown
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.

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

@orionr @facebook-github-bot
Set proper scope on nodes added by JIT (pytorch#12400)
f0e19c4 
Summary:
In order to support tensorboardX and other visualization tools, we need to make sure a non-empty scope is set on all nodes added by the JIT. This attempts to do this, but is still a WIP.

This is a new version of pytorch#10749
Pull Request resolved: pytorch#12400

Differential Revision: D10224380

fbshipit-source-id: ae99180f98b71510e2a6261c5afe66fb79b1c857
zdevito
zdevito approved these changes Oct 22, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Coo looks good, but I have one question about setting the scopes in symbolic variable.

Comment thread torch/csrc/jit/symbolic_variable.h
ScopePtr s;
for(auto n : inputs) {
size_t d = n.value()->node()->scope()->getDepth();
if(d > max_depth) {

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@miaoqiz miaoqiz mentioned this pull request Feb 26, 2019
Closed
@ezyang ezyang added the merged label Jun 26, 2019
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@orionr @facebook-github-bot
Set proper scope on nodes added by JIT (pytorch#12400)
b468b7a 
Summary:
In order to support tensorboardX and other visualization tools, we need to make sure a non-empty scope is set on all nodes added by the JIT. This attempts to do this, but is still a WIP.

This is a new version of pytorch#10749
Pull Request resolved: pytorch#12400

Reviewed By: ezyang

Differential Revision: D10224380

Pulled By: orionr

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

Reviewers

@zdevito zdevito zdevito approved these changes

@lanpa lanpa lanpa left review comments

@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

+2 more reviewers

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

@apaszke apaszke apaszke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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