Skip to content

__deepcopy__ for DualGraphModule #8708

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

Merged
merged 5 commits into from
Nov 28, 2024

Conversation

andreasfloros
Copy link
Contributor

@andreasfloros andreasfloros commented Nov 2, 2024

DualGraphModule inherits deepcopy behavior from GraphModule, which ignores the train_graph and eval_graph.

This PR implements __deepcopy__ for DualGraphModule.

Fixes #8634

Copy link

pytorch-bot bot commented Nov 2, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8708

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 4e10111 with merge base 22e86bd (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@NicolasHug
Copy link
Member

Thank you for the PR @andreasfloros . To be completely transparent here, I was hoping we could find a much more obvious fix for this problem. The current fix in this PR involves importing and relying on various private structures from torch.fx.graph_module, and I'll admit I'm not confident enough in my understanding of that code to be able to land it right now.

Basically, if that code were to break (because e.g. of an upstream change in some internal of torch.fx.graph_module), I wouldn't be able to fix this again.

Do you think we can address the original problem from #8634 in a more obvious way?

@andreasfloros
Copy link
Contributor Author

@NicolasHug the issue is with the hooks. Other than that, deepcopy can be implemented without private structures.
A more obvious implementation is to copy the entire __dict__:

def __deepcopy__(self, memo):
    res = type(self).__new__(type(self))
    memo[id(self)] = res
    res.__dict__ = copy.deepcopy(self.__dict__, memo)
    return res

However, this is not done in GraphModule and there is a comment regarding bc (https://github.com/pytorch/pytorch/blob/main/torch/fx/graph_module.py#L880).

What do you think?

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Oh, I didn't realize the fix was actually mostly taken from the implementation in the base class (you did write that as a comment, I missed it!)
I guess that's fine. Let's hope this file doesn't get updated too often. If it does, we might have to revert the fix, but for now this is hopefully OK.

Thanks a lot for the fix @andreasfloros . I pushed a small non-regression test and I'll merge the PR soon

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@NicolasHug NicolasHug merged commit e21cd0b into pytorch:main Nov 28, 2024
1 check passed
@andreasfloros andreasfloros deleted the dual-graph-deepcopy branch November 29, 2024 22:56
facebook-github-bot pushed a commit that referenced this pull request Jan 13, 2025
Reviewed By: vmoens

Differential Revision: D68021959

fbshipit-source-id: 8ab640cedd3b85d135af0164375da36848602b84

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <nh.nicolas.hug@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deepcopy of torchvision.models.feature_extraction.DualGraphModule breaks eval
3 participants