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.

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

Jump to bottom

[TorchScript] Support user defined classes as constants #45556

Closed
wants to merge 1 commit into from
Closed

[TorchScript] Support user defined classes as constants #45556

wants to merge 1 commit into from

Conversation

bwasti
Copy link
Contributor

@bwasti bwasti commented Sep 30, 2020

Summary: User defined classes can be used as constants. This is useful when freezing and removing the module from the graph.

Test Plan: waitforsadcastle

Differential Revision: D23994974

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 30, 2020
@dr-ci
Copy link

dr-ci bot commented Sep 30, 2020 •
edited

💊 CI failures summary and remediations

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

💚 💚 Looks good so far! There are no failures yet. 💚 💚

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 111 times.

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

bzinodev
bzinodev reviewed Oct 12, 2020
View reviewed changes
aten/src/ATen/core/ivalue.cpp
@@ -459,6 +459,9 @@ std::ostream& IValue::repr(
return out << enum_holder->qualifiedClassName() << "." <<
enum_holder->name();
}
case IValue::Tag::Object: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be TORCH_CHECK? we don't expect to handle this case in the future.

eellison
eellison reviewed Oct 12, 2020
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this if it's going to silently break the ability of a user to save and load their model in combination with torch.jit.freeze.

However, I think it would be very easy to add serialization support in combination with #37631, which is accepted but not merged.

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

bzinodev
bzinodev suggested changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@bzinodev bzinodev left a comment

Choose a reason for hiding this comment

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

Where the tests?

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

bzinodev
bzinodev reviewed Oct 14, 2020
View reviewed changes
torch/csrc/jit/serialization/python_print.cpp Outdated
@@ -864,6 +864,7 @@ struct PythonPrintImpl {

void printConstant(TaggedStringStream& stmt, const IValue& v) {
const auto customFormatter = [&](std::ostream& ss, const IValue& v) {
// TODO Add support for serialization of custom classes
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be an assertion here?
To make sure serialization is not invoked where there is a custom class as a constant

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

hlu1 pushed a commit to hlu1/pytorch that referenced this pull request Oct 21, 2020
@bwasti @facebook-github-bot
[TorchScript] Support user defined classes as constants (pytorch#45556)
cfd398d 
Summary:
Pull Request resolved: pytorch#45556

User defined classes can be used as constants.  This is useful when freezing and removing the module from the graph.

Test Plan: waitforsadcastle

Differential Revision: D23994974

fbshipit-source-id: 0bf853cf20b92c5621db92088f758b5789446a3c
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@jerryzh168
Copy link
Contributor

Thanks @bwasti if this is enabled by default, you can also remove https://github.com/pytorch/pytorch/blob/master/torch/csrc/jit/passes/quantization/finalize.cpp#L99 and see if anything breaks (the pass is added specifically to fold constant objects)

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@neginraoof neginraoof mentioned this pull request Nov 5, 2020
Closed
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@bwasti bwasti mentioned this pull request Nov 11, 2020
Closed
bwasti added a commit to bwasti/glow-1 that referenced this pull request Nov 11, 2020
@bwasti @facebook-github-bot
Support user defined classes as constants (#45556)
26cf85c 
Summary:
Pull Request resolved: pytorch/pytorch#45556

User defined classes can be used as constants.  This is useful when freezing and removing the module from the graph.

Differential Revision: D23994974

fbshipit-source-id: c7683c44ae9288db56444e32181583bc89d9ed93
@bwasti @facebook-github-bot
[TorchScript] Support user defined classes as constants (pytorch#5062)
b71b970 
Summary:
Pull Request resolved: pytorch/glow#5062

Pull Request resolved: pytorch#45556

User defined classes can be used as constants.  This is useful when freezing and removing the module from the graph.

Test Plan: waitforsadcastle

Reviewed By: eellison

Differential Revision: D23994974

fbshipit-source-id: 6c494269e222b7e1b5ecddf0c460ae8c09ac0556
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23994974

@codecov
Copy link

codecov bot commented Nov 16, 2020 •
edited

Codecov Report

Merging #45556 (b71b970) into master (95ea778) will decrease coverage by 0.02%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master   #45556      +/-   ##
==========================================
- Coverage   81.33%   81.30%   -0.03%     
==========================================
  Files        1839     1839              
  Lines      198589   198645      +56     
==========================================
- Hits       161527   161518       -9     
- Misses      37062    37127      +65

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 43a9d6f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZolotukhinM ZolotukhinM ZolotukhinM left review comments

@eellison eellison eellison left review comments

@bzinodev bzinodev bzinodev requested changes

@apaszke apaszke Awaiting requested review from apaszke

Assignees
No one assigned
Labels
cla signed fb-exported Merged oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bwasti @facebook-github-bot @jerryzh168 @ZolotukhinM @eellison @bzinodev