Skip to content
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.

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

Properly serialize types that only appear at function input #47775

Closed
wants to merge 3 commits into from

Conversation

suo
Copy link
Member

@suo suo commented Nov 11, 2020

Stack from ghstack:

When serializing graphs, we check every node for named types referenced,
so that we can register them as dependencies. We were skipping this
check for the graph inputs themselves. Since types used at input are
almost always used somewhere in the graph, we never noticed this gap
until a user reported an issue with NamedTuples.

Differential Revision: D24896289

When serializing graphs, we check every node for named types referenced,
so that we can register them as dependencies. We were skipping this
check for the graph inputs themselves. Since types used at input are
almost always used somewhere in the graph, we never noticed this gap
until a user reported an issue with NamedTuples.

[ghstack-poisoned]
@suo suo requested a review from apaszke as a code owner November 11, 2020 19:33
@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Nov 11, 2020
@suo suo requested a review from jamesr66a November 11, 2020 19:34
Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

What about at the output? I can sneakily construct a counterexample like so:

import torch
from typing import NamedTuple, Optional

class Point(NamedTuple):
  x : int
  y : int

class Foo(torch.nn.Module):
  def __init__(self):
    super().__init__()

  def forward(self) -> Optional[Point]:
    return None

script = torch.jit.script(Foo())
torch.jit.save(script, 'foo.zip')
torch.jit.load('foo.zip')

"""
RuntimeError: 
Unknown type name '__torch__.Point':
Serialized   File "code/__torch__.py", line 5
  __buffers__ = []
  training : bool
  def forward(self: __torch__.Foo) -> Optional[__torch__.Point]:
                                               ~~~~~~~~~~~~~~~ <--- HERE
    return None


"""

When serializing graphs, we check every node for named types referenced,
so that we can register them as dependencies. We were skipping this
check for the graph inputs themselves. Since types used at input are
almost always used somewhere in the graph, we never noticed this gap
until a user reported an issue with NamedTuples.

Differential Revision: [D24896289](https://our.internmc.facebook.com/intern/diff/D24896289)

[ghstack-poisoned]
@dr-ci
Copy link

dr-ci bot commented Nov 11, 2020

💊 CI failures summary and remediations

As of commit b540718 (more details on the Dr. CI page):


  • 1/3 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)
  • 2/3 broken upstream at merge base 4cb73f5 on Nov 11 from 12:30am to 11:54am PDT (12 commits; 0650a61 - dac0192)

🚧 2 fixed upstream failures:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is newer than viable/strict, you can try basing on an older, stable commit:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase --onto FETCH_HEAD $(git merge-base origin/master HEAD)

If your commit is older than viable/strict:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.


ci.pytorch.org: 1 failed


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 7 times.

When serializing graphs, we check every node for named types referenced,
so that we can register them as dependencies. We were skipping this
check for the graph inputs themselves. Since types used at input are
almost always used somewhere in the graph, we never noticed this gap
until a user reported an issue with NamedTuples.

Differential Revision: [D24896289](https://our.internmc.facebook.com/intern/diff/D24896289)

[ghstack-poisoned]
suo added a commit that referenced this pull request Nov 11, 2020
When serializing graphs, we check every node for named types referenced,
so that we can register them as dependencies. We were skipping this
check for the graph inputs themselves. Since types used at input are
almost always used somewhere in the graph, we never noticed this gap
until a user reported an issue with NamedTuples.

ghstack-source-id: 50b22f2c7b1f1a39c374ab125e3981a06bf9b2f7
Pull Request resolved: #47775
Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

ezgif com-gif-maker

@facebook-github-bot
Copy link
Contributor

@suo merged this pull request in d4fa84b.

@facebook-github-bot
Copy link
Contributor

@suo merged this pull request in d4fa84b.

@facebook-github-bot facebook-github-bot deleted the gh/suo/378/head branch November 15, 2020 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla signed 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.

None yet

3 participants