Skip to content

[jit] Enable copy.deepcopy and copy.copy for RecursiveScriptModule #32685

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

Closed
wants to merge 15 commits into from

Conversation

jerryzh168
Copy link
Contributor

@jerryzh168 jerryzh168 commented Jan 28, 2020

Stack from ghstack:

Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

Differential Revision: D21220755

@jerryzh168 jerryzh168 requested a review from apaszke as a code owner January 28, 2020 02:26
jerryzh168 added a commit that referenced this pull request Jan 28, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 192e7b1
Pull Request resolved: #32685
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jan 28, 2020
@jerryzh168 jerryzh168 requested review from suo and ZolotukhinM January 28, 2020 02:36
@kostmo
Copy link
Member

kostmo commented Jan 28, 2020

💊 CI failures summary and remediations

As of commit 80b0641 (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 176 times.

suo
suo previously approved these changes Mar 9, 2020
@suo
Copy link
Member

suo commented Mar 9, 2020

Can you mark this PR as BC-breaking

@jerryzh168 jerryzh168 added the module: bc-breaking Related to a BC-breaking change label Mar 9, 2020
jerryzh168 added a commit that referenced this pull request Mar 18, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 3eae171
Pull Request resolved: #32685
jerryzh168 added a commit that referenced this pull request Mar 19, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 623313a
Pull Request resolved: #32685
jerryzh168 added a commit that referenced this pull request Mar 20, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 9a03aec
Pull Request resolved: #32685
jerryzh168 added a commit that referenced this pull request Mar 24, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 954e556
Pull Request resolved: #32685
okly366 pushed a commit to okly366/pytorch that referenced this pull request Apr 26, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 429f79e
Pull Request resolved: pytorch/pytorch#32685
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
jerryzh168 added a commit that referenced this pull request Apr 28, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 622bdf1
Pull Request resolved: #32685
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
jerryzh168 added a commit that referenced this pull request Apr 28, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: 7eaa906
Pull Request resolved: #32685
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
jerryzh168 added a commit that referenced this pull request Apr 29, 2020
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: ef958db
Pull Request resolved: #32685
Summary:
att

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
Summary:
Enable `copy.deepcopy` and `copy.copy` and remove `copy`

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@jerryzh168 jerryzh168 changed the title [jit] Remove copy from public API [jit] Update copy API for RecursiveScriptModule Jun 19, 2020
…ScriptModule"

Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@jerryzh168 jerryzh168 changed the title [jit] Update copy API for RecursiveScriptModule [jit] Enable copy.deepcopy and copy.copy for RecursiveScriptModule Jun 19, 2020
@jerryzh168 jerryzh168 requested a review from suo June 19, 2020 19:37
@jerryzh168 jerryzh168 removed the module: bc-breaking Related to a BC-breaking change label Jun 19, 2020
@jerryzh168 jerryzh168 requested review from jamesr66a and zdevito June 19, 2020 19:37
…ScriptModule"

Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
jerryzh168 added a commit that referenced this pull request Jun 19, 2020
Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

ghstack-source-id: f4152cb
Pull Request resolved: #32685
…ScriptModule"

Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@jerryzh168
Copy link
Contributor Author

Got this:

Jun 19 23:07:12 ERROR [0.004s]: test_model_save_error (test_jit.TestJit) 
Jun 19 23:07:12 ---------------------------------------------------------------------- 
Jun 19 23:07:12 Traceback (most recent call last): 
Jun 19 23:07:12   File "/var/lib/jenkins/workspace/test/test_jit.py", line 355, in test_model_save_error 
Jun 19 23:07:12     torch.save(FooToPickle(), fname) 
Jun 19 23:07:12   File "/opt/conda/lib/python3.6/site-packages/torch/serialization.py", line 364, in save 
Jun 19 23:07:12     _save(obj, opened_file, pickle_module, pickle_protocol) 
Jun 19 23:07:12   File "/opt/conda/lib/python3.6/site-packages/torch/serialization.py", line 468, in _save 
Jun 19 23:07:12     pickler.dump(obj) 
Jun 19 23:07:12 RuntimeError: Tried to serialize object __torch__.torch.jit.ScriptModule which does not have a __getstate__ method defined! 
Jun 19 23:07:12  

I think it's because I removed __getstate__ from RecursiveScriptModule, can we remove this test now since we do have __getstate__ defined in script::Object and script::Module now? cc @zdevito, @jamesr66a

@zdevito
Copy link
Contributor

zdevito commented Jun 22, 2020

That test is checking that there is an error with a reasonable error message for saving a module with TorchScript submodules. It looks like this change broke the friendly error message and made it something less understandable.

…ScriptModule"

Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@jerryzh168
Copy link
Contributor Author

That test is checking that there is an error with a reasonable error message for saving a module with TorchScript submodules. It looks like this change broke the friendly error message and made it something less understandable.

Fixed.

…ScriptModule"

Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
…ScriptModule"

Summary:

Test Plan:
.

Reviewers:
.
Subscribers:

Tasks:

Tags:

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

[ghstack-poisoned]
@jerryzh168 jerryzh168 dismissed suo’s stale review June 22, 2020 23:46

code changed

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

Looks good

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in f652abc.

@facebook-github-bot facebook-github-bot deleted the gh/jerryzh168/196/head branch June 27, 2020 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants