Skip to content

Do not use deepcopy to copy nn.modules (including entire models) #2177

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
mikekgfb opened this issue Jan 21, 2023 · 10 comments
Closed

Do not use deepcopy to copy nn.modules (including entire models) #2177

mikekgfb opened this issue Jan 21, 2023 · 10 comments

Comments

@mikekgfb
Copy link
Contributor

mikekgfb commented Jan 21, 2023

copy.deepcopy is not defined for nn.module() and does not reliably copy an nn.module hierrchy, such as a model or partial model. Our tutorials should using copy.deepcopy() as this will induce our users to make incorrect use of the primitive.

Please update https://pytorch.org/tutorials/beginner/transformer_tutorial.html and other tutorials to avoid the use. Recommended way for snapshotting is to use torch.save() for trained models. to create multiple clones of an untrained model, create multiple copies from first principles from the model architectural parameters.

cc @pytorch/team-text-core @Nayef211

@mikekgfb
Copy link
Contributor Author

cc: @mthrok

@Nayef211
Copy link
Contributor

Thanks for creating this issue @mikekgfb. Will submit a PR with the fixes soon.

@Nayef211
Copy link
Contributor

Nayef211 commented Jan 25, 2023

@svekars I don't think this issue should be closed. The PR I sent out fixed the issue for https://pytorch.org/tutorials/beginner/transformer_tutorial.html. But there are several other places where it's still being used.

I.e. https://github.com/search?q=repo%3Apytorch%2Ftutorials%20copy.deepcopy(&type=code

@Nayef211 Nayef211 reopened this Jan 25, 2023
@svekars
Copy link
Contributor

svekars commented Jan 30, 2023

Can the owners of the tutorials please take a look and submit corrections? If some of these tutorials describe an obsolete feature, please submit a PR to remove them:

@vkuzo
Copy link
Contributor

vkuzo commented Jan 30, 2023

For quantization, we were assuming that copy.deepcopy is defined on nn.Module. Is it really not defined - are we sure that it's not just a bug that it's not working in some cases?

@jerryzh168
Copy link
Contributor

cc @qihqi

@qihqi
Copy link
Contributor

qihqi commented Jan 30, 2023

Q: copy.deepcopy is not defined for nn.module() and does not reliably copy an nn.module hierrchy -- I believe in truthiness of this statement. However, why is the CTA "stop using deepcopy" and not "let's fix deepcopy for nn.Module"? Like is there a fundamental reason the latter is impossible or undesired, or it's really, "until deepcopy start working properly don't use it"?

@chedatomasz
Copy link

Are you sure deepcopy is not supposed to be defined for nn.modules? https://github.com/pytorch/pytorch/blob/master/torch/optim/swa_utils.py appears to be using deepcopy.

@carljparker
Copy link
Contributor

I'm closing this issue because sekyondaMeta has opened up targeted issues to fix the specific tutorials that need updating.

#2335 | #2334 | #2333 | #2332 | #2331 | #2330

@bot66
Copy link

bot66 commented Nov 6, 2023

Thanks for the information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants