[fix] allow saving python attr on Tensor and Parameter via torch.save #81616

kshitij12345 · 2022-07-17T11:58:57Z

TODO:

Fix for Parameter

Benchmark
(Measurable diff for small tensors)

[-------------- Save and Load --------------]
                    |  After PR  |  Before PR
1 threads: ----------------------------------
      ()            |    111.7   |     106.9 
      (4, 4)        |    114.4   |     109.2 
      (128, 128)    |    135.2   |     128.3 
      (1024, 1024)  |   1431.9   |    1431.3 

Times are in microseconds (us).

Benchmark Script

import torch
from torch.testing._internal.common_utils import BytesIOContext
from torch.utils import benchmark
import pickle

shapes = ((), (4, 4), (128, 128), (1024, 1024))

sizes = [1, 64, 1024, 10000]
results = []

def save_load_fn(t):
    with BytesIOContext() as f:
        torch.save(t, f)
        f.seek(0)
        torch.load(f)

for shape in shapes:
    t = torch.randn(shape)
    label = 'Save and Load'
    sub_label = f'{shape}'
    results.append(benchmark.Timer(
        stmt='save_load_fn(t)',
        globals={'t': t, 'save_load_fn':save_load_fn},
        label=label,
        sub_label=sub_label,
        description='Before PR',
    ).blocked_autorange(min_run_time=2))

compare = benchmark.Compare(results)
compare.print()

with open('before_pr.pkl', 'wb') as f:
    pickle.dump(results, f)

# with open('after_pr.pkl', 'rb') as f:
#     after_pr = pickle.load(f)

# with open('before_pr.pkl', 'rb') as f:
#     before_pr = pickle.load(f)

# compare = benchmark.Compare(after_pr + before_pr)
# compare.print()

NOTE : BC-Breaking : After this PR, all tensors (also regular tensors) will be serialised using _rebuild_from_type_v2.

cc @ezyang @gchanan

facebook-github-bot · 2022-07-17T11:59:04Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/81616
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

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

Expand to see more

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

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

albanD

Thanks for this fix! This code is way too complex :(
Only small nits

albanD · 2022-07-20T15:36:16Z

torch/_utils.py

+ param._backward_hooks = backward_hooks
+
+ # Restore state on Parameter like python attr.
+ param = torch._utils._set_obj_state(param, state)


nit no need for the torch._utils here as it is the current file

albanD · 2022-07-20T15:38:55Z

torch/_tensor.py

- if type(self) is Tensor:
- return self._reduce_ex_internal(proto)
- if has_torch_function_unary(self):
+ if type(self) is not Tensor and has_torch_function_unary(self):


has_torch_function_unary should always be False for regular Tensor. So the first part is not needed.

kshitij12345 · 2022-07-20T16:03:28Z

@pytorchbot merge -g

pytorchmergebot · 2022-07-20T16:04:53Z

@pytorchbot successfully started a merge job. Check the current status here

github-actions · 2022-07-20T18:46:12Z

Hey @kshitij12345.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

jeanschmidt · 2022-07-21T10:41:10Z

this commit breaks internal tests/builds - https://pastebin.com/fU9T0NMd https://pastebin.com/1fwiE9ZU

jeanschmidt · 2022-07-21T10:41:37Z

@pytorchbot revert -m "breaking internal builds" -c "ghfirst"

albanD · 2022-07-21T10:43:12Z

@jeanschmidt the first one seems to be a network error?

pytorchmergebot · 2022-07-21T10:46:20Z

@pytorchbot successfully started a revert job. Check the current status here

pytorchmergebot · 2022-07-21T10:46:26Z

@kshitij12345 your PR has been successfully reverted.

…rch.save (#81616)" This reverts commit f3f8d96. Reverted #81616 on behalf of https://github.com/jeanschmidt due to breaking internal builds

albanD · 2022-07-21T10:50:51Z

@jeanschmidt not sure what to do with the recond one. Is there any instruction to repro this?

jeanschmidt · 2022-07-21T15:56:11Z

Yeah, the first one seems to be an internal issue in the build system, the 2nd one seems to be the process dying while running tests (throwing some generic exception :( ).

I believe you don't have access to the code to reproduce it right?

torch/_utils.py

test/test_serialization.py

ezyang · 2022-11-11T02:09:57Z

If there aren't any Python attributes, it would also be smart to use the old builder function for FC

…to fix/tensor-serialization/attr

kshitij12345 · 2022-11-11T09:44:19Z

If there aren't any Python attributes, it would also be smart to use the old builder function for FC

Right!! Now we only dispatch to the new builders only if the state is present. So this should take care of compatibility! Thank you!

albanD

This is not FC if the user saves a Tensor with state though?

ezyang · 2022-11-11T14:29:02Z

Yes, there is still breakage if there are Python attributes. But there's no way around this, except maybe a kwarg on save that lets you bypass saving python attrs

albanD · 2022-11-11T20:09:13Z

Yes, there is still breakage if there are Python attributes. But there's no way around this, except maybe a kwarg on save that lets you bypass saving python attrs

No we will warn but not break on the C++ side.
The problem here is that even in python this will break today. We do want the FC window.

albanD

SGTM

kshitij12345 · 2022-11-11T20:13:05Z

@albanD would you be importing it for internal tests? Or is it ok to merge it once CI is green?

albanD · 2022-11-11T20:15:09Z

This can't be tested internally as it breaks when publishing. So let's go with this as-is!

kshitij12345 · 2022-11-11T20:18:08Z

@pytorchbot merge

pytorchmergebot · 2022-11-11T20:20:39Z

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging

Check the merge workflow status
here

Ref: #81616 (comment) Pull Request resolved: #88913 Approved by: https://github.com/albanD

…pytorch#81616) Fixes: pytorch#72129 TODO: * [x] Fix for Parameter Benchmark (Measurable diff for small tensors) ``` [-------------- Save and Load --------------] | After PR | Before PR 1 threads: ---------------------------------- () | 111.7 | 106.9 (4, 4) | 114.4 | 109.2 (128, 128) | 135.2 | 128.3 (1024, 1024) | 1431.9 | 1431.3 Times are in microseconds (us). ``` <details> <summary> Benchmark Script </summary> ```python import torch from torch.testing._internal.common_utils import BytesIOContext from torch.utils import benchmark import pickle shapes = ((), (4, 4), (128, 128), (1024, 1024)) sizes = [1, 64, 1024, 10000] results = [] def save_load_fn(t): with BytesIOContext() as f: torch.save(t, f) f.seek(0) torch.load(f) for shape in shapes: t = torch.randn(shape) label = 'Save and Load' sub_label = f'{shape}' results.append(benchmark.Timer( stmt='save_load_fn(t)', globals={'t': t, 'save_load_fn':save_load_fn}, label=label, sub_label=sub_label, description='Before PR', ).blocked_autorange(min_run_time=2)) compare = benchmark.Compare(results) compare.print() with open('before_pr.pkl', 'wb') as f: pickle.dump(results, f) # with open('after_pr.pkl', 'rb') as f: # after_pr = pickle.load(f) # with open('before_pr.pkl', 'rb') as f: # before_pr = pickle.load(f) # compare = benchmark.Compare(after_pr + before_pr) # compare.print() ``` </details> NOTE : **BC-Breaking** : After this PR, all tensors (also regular tensors) will be serialised using `_rebuild_from_type_v2`. Pull Request resolved: pytorch#81616 Approved by: https://github.com/albanD, https://github.com/kurtamohler

…rch.save (pytorch#81616)" This reverts commit 54b6188. Reverted pytorch#81616 on behalf of https://github.com/mehtanirav due to Internal publishing is broken

…pytorch#81616) Fixes: pytorch#72129 TODO: * [x] Fix for Parameter Benchmark (Measurable diff for small tensors) ``` [-------------- Save and Load --------------] | After PR | Before PR 1 threads: ---------------------------------- () | 111.7 | 106.9 (4, 4) | 114.4 | 109.2 (128, 128) | 135.2 | 128.3 (1024, 1024) | 1431.9 | 1431.3 Times are in microseconds (us). ``` <details> <summary> Benchmark Script </summary> ```python import torch from torch.testing._internal.common_utils import BytesIOContext from torch.utils import benchmark import pickle shapes = ((), (4, 4), (128, 128), (1024, 1024)) sizes = [1, 64, 1024, 10000] results = [] def save_load_fn(t): with BytesIOContext() as f: torch.save(t, f) f.seek(0) torch.load(f) for shape in shapes: t = torch.randn(shape) label = 'Save and Load' sub_label = f'{shape}' results.append(benchmark.Timer( stmt='save_load_fn(t)', globals={'t': t, 'save_load_fn':save_load_fn}, label=label, sub_label=sub_label, description='Before PR', ).blocked_autorange(min_run_time=2)) compare = benchmark.Compare(results) compare.print() with open('before_pr.pkl', 'wb') as f: pickle.dump(results, f) # with open('after_pr.pkl', 'rb') as f: # after_pr = pickle.load(f) # with open('before_pr.pkl', 'rb') as f: # before_pr = pickle.load(f) # compare = benchmark.Compare(after_pr + before_pr) # compare.print() ``` </details> NOTE : **BC-Breaking** : After this PR, all tensors (also regular tensors) will be serialised using `_rebuild_from_type_v2`. Pull Request resolved: pytorch#81616 Approved by: https://github.com/albanD, https://github.com/kurtamohler

Ref: pytorch#81616 (comment) Pull Request resolved: pytorch#88913 Approved by: https://github.com/albanD

Ref: #81616 (comment) Pull Request resolved: #88913 Approved by: https://github.com/albanD

[fix] allow saving python attr via torch.save

e1a47d6

facebook-github-bot added the cla signed label Jul 17, 2022

pytorchbot added the open source label Jul 17, 2022

kshitij12345 changed the title ~~[fix] allow saving python attr via torch.save~~ [fix] allow saving python attr on Tensor via torch.save Jul 17, 2022

kshitij12345 added 2 commits July 17, 2022 15:22

support for Param

af2495b

correct missing comment

11a1008

kshitij12345 changed the title ~~[fix] allow saving python attr on Tensor via torch.save~~ [fix] allow saving python attr on Tensor and Parameter via torch.save Jul 17, 2022

kshitij12345 marked this pull request as ready for review July 17, 2022 18:24

kshitij12345 requested review from albanD and jbschlosser as code owners July 17, 2022 18:24

bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 20, 2022

albanD approved these changes Jul 20, 2022

View reviewed changes

address review

9edff6b

albanD mentioned this pull request Jul 20, 2022

Preserve grad during pickling #81188

Closed

pytorchmergebot added the Merged label Jul 20, 2022

pytorchmergebot closed this in f3f8d96 Jul 20, 2022

pytorchmergebot added the Reverted label Jul 21, 2022

kshitij12345 added 3 commits November 10, 2022 18:18

don't use rebuild_parameter_v2 for this PR

38943a1

sync

2ecb849

don't save state as it is not used

5f226ea

kshitij12345 commented Nov 10, 2022

View reviewed changes

torch/_utils.py Outdated Show resolved Hide resolved

kshitij12345 commented Nov 10, 2022

View reviewed changes

test/test_serialization.py Outdated Show resolved Hide resolved

kshitij12345 added 4 commits November 11, 2022 09:37

use usual path if state is empty

dd7c710

Merge branch 'viable/strict' of https://github.com/pytorch/pytorch in…

690365d

…to fix/tensor-serialization/attr

remove stale comment

2e7122c

update incorrect comment

cc72e89

albanD reviewed Nov 11, 2022

View reviewed changes

dont use rebuild_parameter_state yet

a6e5e6b

albanD approved these changes Nov 11, 2022

View reviewed changes

pytorchmergebot closed this in f749463 Nov 11, 2022

kshitij12345 mentioned this pull request Nov 11, 2022

[follow-up] Python Attr Serialization #88913

Closed

pytorchmergebot pushed a commit that referenced this pull request Nov 29, 2022

[follow-up] Python Attr Serialization (#88913)

086b251

Ref: #81616 (comment) Pull Request resolved: #88913 Approved by: https://github.com/albanD

kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022

[follow-up] Python Attr Serialization (pytorch#88913)

0650aa3

Ref: pytorch#81616 (comment) Pull Request resolved: pytorch#88913 Approved by: https://github.com/albanD

pytorchmergebot pushed a commit that referenced this pull request Jan 13, 2023

[follow-up] Python Attr Serialization (#88913)

745fe35

Ref: #81616 (comment) Pull Request resolved: #88913 Approved by: https://github.com/albanD

albanD mentioned this pull request Jan 19, 2023

PyObject preservation and resurrection for StorageImpl #91395

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] allow saving python attr on Tensor and Parameter via torch.save #81616

[fix] allow saving python attr on Tensor and Parameter via torch.save #81616

kshitij12345 commented Jul 17, 2022 •

edited by pytorch-bot bot

Loading

facebook-github-bot commented Jul 17, 2022 •

edited

Loading

albanD left a comment

albanD Jul 20, 2022

albanD Jul 20, 2022

kshitij12345 commented Jul 20, 2022

pytorchmergebot commented Jul 20, 2022

github-actions bot commented Jul 20, 2022

jeanschmidt commented Jul 21, 2022

jeanschmidt commented Jul 21, 2022

albanD commented Jul 21, 2022

pytorchmergebot commented Jul 21, 2022

pytorchmergebot commented Jul 21, 2022

albanD commented Jul 21, 2022

jeanschmidt commented Jul 21, 2022 •

edited

Loading

ezyang commented Nov 11, 2022

kshitij12345 commented Nov 11, 2022

albanD left a comment

ezyang commented Nov 11, 2022

albanD commented Nov 11, 2022

albanD left a comment

kshitij12345 commented Nov 11, 2022

albanD commented Nov 11, 2022

kshitij12345 commented Nov 11, 2022

pytorchmergebot commented Nov 11, 2022

[fix] allow saving python attr on Tensor and Parameter via torch.save #81616

[fix] allow saving python attr on Tensor and Parameter via torch.save #81616

Conversation

kshitij12345 commented Jul 17, 2022 • edited by pytorch-bot bot Loading

facebook-github-bot commented Jul 17, 2022 • edited Loading

🔗 Helpful links

✅ No Failures (0 Pending)

albanD left a comment

Choose a reason for hiding this comment

albanD Jul 20, 2022

Choose a reason for hiding this comment

albanD Jul 20, 2022

Choose a reason for hiding this comment

kshitij12345 commented Jul 20, 2022

pytorchmergebot commented Jul 20, 2022

github-actions bot commented Jul 20, 2022

jeanschmidt commented Jul 21, 2022

jeanschmidt commented Jul 21, 2022

albanD commented Jul 21, 2022

pytorchmergebot commented Jul 21, 2022

pytorchmergebot commented Jul 21, 2022

albanD commented Jul 21, 2022

jeanschmidt commented Jul 21, 2022 • edited Loading

ezyang commented Nov 11, 2022

kshitij12345 commented Nov 11, 2022

albanD left a comment

Choose a reason for hiding this comment

ezyang commented Nov 11, 2022

albanD commented Nov 11, 2022

albanD left a comment

Choose a reason for hiding this comment

kshitij12345 commented Nov 11, 2022

albanD commented Nov 11, 2022

kshitij12345 commented Nov 11, 2022

pytorchmergebot commented Nov 11, 2022

Merge started

kshitij12345 commented Jul 17, 2022 •

edited by pytorch-bot bot

Loading

facebook-github-bot commented Jul 17, 2022 •

edited

Loading

jeanschmidt commented Jul 21, 2022 •

edited

Loading