Skip to content

fix overspecializing constants in compilation #22816

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

Closed
eellison wants to merge 3 commits into from
Closed

fix overspecializing constants in compilation #22816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f101e3
fix overspecializing constants
Jul 12, 2019
678cd04
fix python 2
Jul 12, 2019
727f335
py2 incompat
Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions test/test_jit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7695,6 +7695,32 @@ def list_tensors(x):
# testing that tensor type of lists is unified
self.getExportImportCopy(m)

def test_import_constants_not_specialized(self):
class Mod(torch.nn.Module):
def forward(self, x):
return torch.cat(2 * [x], dim=0)

class ScriptMod(torch.jit.ScriptModule):
def __init__(self, mod):
super(ScriptMod, self).__init__()
x = torch.zeros(1, 3)
mod_fn = lambda : mod(x) # noqa: E731
self.mod = torch.jit.trace(mod_fn, tuple())

@torch.jit.script_method
def forward(self):
return self.mod()

cm = ScriptMod(Mod())
# specialized tensor in graph
FileCheck().check("Double(1, 3)").run(cm.forward.graph)
buffer = io.BytesIO()
torch.jit.save(cm, buffer)
buffer.seek(0)
# when tensor is loaded as constant it isnt specialized
cm_load = torch.jit.load(buffer)
FileCheck().check_not("Double(1, 3)").run(cm_load.forward.graph)

def test_type_annotations_repeated_list(self):
@torch.jit.script
def float_fn(x, y):
Expand Down
4 changes: 4 additions & 0 deletions torch/csrc/jit/import_source.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ struct ConstantTableValue : public SugaredValue {
<< constants_.size() << " entries).";
}
Value* value = m.graph()->insertConstant(constants_[offset], nullptr, loc);

// specializing tensor type on compilation messes up typing relations
Copy link
Contributor

@driazati driazati Jul 12, 2019

Choose a reason for hiding this comment

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

What typing relations does it mess up? Shouldn't we fix where those types are checked instead of throwing away the shape info?

Copy link
Contributor Author

@eellison eellison Jul 12, 2019

Choose a reason for hiding this comment

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

The shape info gets re-specialized again later anyway. It messes up things like in the example, where a list of complete tensor types does not subtype a list of tensor types, and you get error messages like:

  aten::cat(Tensor[] tensors, int dim=<default>) -> Tensor:
  Expected a value of type 'List[Tensor]' for argument 'tensors' but instead found type 'List[Tensor]'.

Copy link
Contributor

@zdevito zdevito Jul 15, 2019

Choose a reason for hiding this comment

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

This doesn't seem like the right place to fix this. Afterall, this same bug will exist anywhere insertConstant is used. insertConstant would be better, with shape propagation introducing a shape for a constant tensor.

Copy link
Contributor Author

@eellison eellison Jul 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

i don't think there's another pathway to create a specialized tensor type during typechecking. the shape analysis prim::Constant refinement already exists.

value->setType(unshapedType(value->type()));

return std::make_shared<SimpleValue>(value);
}

Expand Down