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

Fix jit.trace mis-handling of InterfaceType #53052

Closed
wants to merge 1 commit into from
Closed

Fix jit.trace mis-handling of InterfaceType #53052

wants to merge 1 commit into from

Conversation

gmagogsfm
Copy link
Contributor

@gmagogsfm gmagogsfm commented Mar 1, 2021

jit.trace recursively gathers all named attributes in module at beginning of
tracing. This is fine in a pure-tracing environment, but breaks when a
scripted module that contains an InterfaceType'd submodule is involved.
Because InterfaceType, by design, is not allowed to have any attribute,
thus some of the gathered attributes will turn into fatal errors in
following some graph rewrite passes.

This PR fixes this bug by distinguishing InterfaceType'd submodules from
normal ClassType'd submodules.

@facebook-github-bot facebook-github-bot added cla signed oncall: jit Add this issue/PR to JIT oncall triage queue labels Mar 1, 2021
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 1, 2021

💊 CI failures summary and remediations

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


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-scanned failure(s)

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 to the (internal) Dr. CI Users group.

Copy link
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.

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

@gmagogsfm gmagogsfm changed the title [WIP] Fix jit.trace handling of InterfaceType Fix jit.trace mis-handling of InterfaceType Mar 1, 2021
@gmagogsfm gmagogsfm marked this pull request as ready for review March 1, 2021 23:45
@gmagogsfm gmagogsfm requested a review from wanchaol March 1, 2021 23:45
Copy link
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.

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

Copy link
Contributor

@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 to me. Thanks for the fix!

torch/csrc/jit/frontend/tracer.cpp Show resolved Hide resolved
`jit.trace` recursively gathers all named attributes in module at beginning of
tracing. This is fine in a pure-tracing environment, but breaks when a
scripted module that contains an InterfaceType'd submodule is involved.
Because InterfaceType, by design, is not allowed to have any attribute,
thus some of the gathered attributes will turn into fatal errors in
following some graph rewrite passes.

This PR fixes this bug by distinguishing InterfaceType'd submodules from
normal ClassType'd submodules.
Copy link
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.

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

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #53052 (49b18c8) into master (e00e42d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #53052   +/-   ##
=======================================
  Coverage   78.00%   78.01%           
=======================================
  Files        1848     1848           
  Lines      179724   179726    +2     
=======================================
+ Hits       140200   140205    +5     
+ Misses      39524    39521    -3     

@facebook-github-bot
Copy link
Contributor

@gmagogsfm merged this pull request in f448c59.

aocsa pushed a commit to Quansight/pytorch that referenced this pull request Mar 15, 2021
Summary:
`jit.trace` recursively gathers all named attributes in module at beginning of
tracing. This is fine in a pure-tracing environment, but breaks when a
scripted module that contains an InterfaceType'd submodule is involved.
Because InterfaceType, by design, is not allowed to have any attribute,
thus some of the gathered attributes will turn into fatal errors in
following some graph rewrite passes.

This PR fixes this bug by distinguishing InterfaceType'd submodules from
normal ClassType'd submodules.

Pull Request resolved: pytorch#53052

Reviewed By: wanchaol

Differential Revision: D26735566

Pulled By: gmagogsfm

fbshipit-source-id: a14aee6f1fe8000f80c2dc60bdf19acee6225090
xsacha pushed a commit to xsacha/pytorch that referenced this pull request Mar 31, 2021
Summary:
`jit.trace` recursively gathers all named attributes in module at beginning of
tracing. This is fine in a pure-tracing environment, but breaks when a
scripted module that contains an InterfaceType'd submodule is involved.
Because InterfaceType, by design, is not allowed to have any attribute,
thus some of the gathered attributes will turn into fatal errors in
following some graph rewrite passes.

This PR fixes this bug by distinguishing InterfaceType'd submodules from
normal ClassType'd submodules.

Pull Request resolved: pytorch#53052

Reviewed By: wanchaol

Differential Revision: D26735566

Pulled By: gmagogsfm

fbshipit-source-id: a14aee6f1fe8000f80c2dc60bdf19acee6225090
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