Deprecate TypedStorage, its derived classes, and all of their public methods #85303
Conversation
kurtamohler
added
the
module: python frontend
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85303
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 FailuresAs of commit 9ec684c: The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
kurtamohler
force-pushed
the
deprecate-TypedStorage
branch
2 times, most recently
from
0532a15
to
bc5fe01
Compare
|
@ezyang, I've added a test to check that it only gets raised once unless warnings are cleared. I didn't add all of the functions to the test, but I can if we want to be that thorough |
|
nah that's good enough |
|
A good follow up would be to make sure pytorch proper doesn't raise these deprecations |
|
@pytorchbot merge |
|
@pytorchbot successfully started a merge job. Check the current status here. |
Merge failedReason: The following mandatory check(s) failed (Rule Dig deeper by viewing the failures on hud Details for Dev Infra teamRaised by workflow job |
By this, do you mean because a |
kurtamohler
force-pushed
the
deprecate-TypedStorage
branch
from
bc5fe01
to
a8a2070
Compare
Different: what I mean is that we shouldn't trigger the deprecation warning if the user didn't explicitly use typed storage. If pytorch internally is hitting the DeprecationWarning, we should fix it (because otherwise it will spam users) |
|
I see, good point |
kurtamohler
force-pushed
the
deprecate-TypedStorage
branch
from
a8a2070
to
51a529e
Compare
|
@ezyang , there are some places where internal calls are raising the warning. For instance, when serializing tensors. I'm not sure what would be the best way to avoid internally generated warnings, but I thought of two options. The first one is to use the The other is to add an underscored version of each function which does everything except for raising the warning. The public function would just raise the warning and then call the underscored function. We would change all the internal call sites to use the underscored version to avoid raising the warning. Something like this: def _func(...):
# do stuff
def func(...):
_warn_typed_storage_removal()
return _func(...)I think I like this solution, but what do you think? Is there a better way? I tried googling a common solution for this and haven't found anything yet |
|
No warn variants sgtm, esp if you only need a few / we have a strategy for getting rid of them |
|
Actually, I just realized that I can't do this for dunder functions. So instead, I think I'll have to add a kwarg |
|
/easycla As part of the transition to the PyTorch Foundation, this project now requires contributions be covered under the new CLA. See #85559 for additional details. This comment will trigger a new check of this PR. If you are already covered, you will simply see a new "EasyCLA" check that passes. If you are not covered, a bot will leave a new comment with a link to sign. |
kurtamohler
force-pushed
the
deprecate-TypedStorage
branch
from
51a529e
to
9f654a8
Compare
kurtamohler
requested review from
mrshenli,
zhaojuanmao,
pritamdamania87,
rohan-varma and
mingzhe09088
as code owners
lezcano
added
ciflow/trunk
|
Looking through the logs, the warnings aren't very noisy anymore. Most of the test logs have less than 30 of them, and most of these are just from separate test file executions (separate processes). Only two of the jobs have more than 100 warnings. I am sifting through all the logs now and soon I will post an overview of all the duplicate counts for each job and what I know about them. I think there's only a handful of root causes left now, and I think almost all of them can probably be investigated/fixed after this PR is merged (for instance, the duplicates from in-between But I am sifting through the logs that generate the most duplicates now, and I'll post a summary soon |
|
I think this is good enough, thank you for the detailed investigation. The |
|
I might as well post what I have right now. Below are counts for how many times the warning was raised for each of the test CI jobs in the inductor, periodic, pull, and trunk workflows. inductor workflow
periodic workflow
pull workflow
trunk workflow
Most of these warnings are raised because test code explicitly uses the deprecated functions, and there are multiple warnings in one job because they get triggered by different test files, which are run as separate processes. But some of the logs show actual duplicates--multiple warnings generated by the execution of just one test file. There seem to be only a few root causes remaining for these. Here are some that I looked into, but haven't looked into all of them yet. click to expand75 from
58 from
46 from
46 from
44 from
42
33
31
Also, not show above, crossref and dynamo jobs have an unusual amount of duplicates between tests in a single file. I'm not sure why. It does reproduce locally if I set |
|
re the crossref failures, part of the crossref process involves monkeying around with storage api. Right now it's using the deprecated api which we should fix Probably dynamo is similar. |
|
@ezyang, where is that code snippet from? Having trouble finding it |
|
torch/_subclasses/meta_utils.py |
|
Oh, I already changed that to use the internal functions, so that's not where the duplicates are coming from. And actually, I misspoke--the latest update I made fixed most of the duplicates from crossref. As the table for the pull workflow above shows, the two crossref jobs only have 26 and 36 warnings now--and looking at the logs, I can see that these are mostly due to The two dynamo jobs, on the other hand, still have the highest warning counts. Here's a summary of most of the duplicates from one of them: 216 from
Looking at tracebacks added to the warning messages, most (if not all) of these are triggered by calls to the public TypedStorage function calls in the test code, not by any public function calls in |
kurtamohler
force-pushed
the
deprecate-TypedStorage
branch
from
a5a4dd6
to
893ba7d
Compare
|
The |
|
There is somewhat of a workaround, but it won't work as shown below in practice because if there are warnings raised inside the click to expandimport warnings
for _ in range(3):
try:
saved_registry = __warningregistry__
except NameError:
saved_registry = None
with warnings.catch_warnings():
pass
if saved_registry is not None:
saved_registry['version'] += 2
__warningregistry__ = saved_registry
warnings.warn('warning')
warnings.warn(RuntimeWarning('another warning'))Output: EDIT: I guess we could find the expected version number by raising a sort of dummy warning after exiting |
|
dummy warning after catch warning doesn't seem like a huge problem |
|
Yeah we can just filter out the dummy warning and it will still update the version number. This seems to basically work: click to expandimport warnings
class my_catch_warnings(warnings.catch_warnings):
def __enter__(self):
global __warningregistry__
try:
self._saved_registry = __warningregistry__.copy()
except NameError:
self._saved_registry = None
super().__enter__()
def __exit__(self, *args, **kwargs):
super().__exit__(*args, **kwargs)
if self._saved_registry is not None:
global __warningregistry__
dummy_message = 'dummy warning to find out warning registry version number'
warnings.filterwarnings('ignore', message=dummy_message)
warnings.warn(dummy_message)
self._saved_registry['version'] = __warningregistry__['version']
__warningregistry__ = self._saved_registry
for i in range(3):
with my_catch_warnings():
warnings.warn('inside catch_warnings')
warnings.warn('this should only emit once')
warnings.warn(RuntimeWarning('and this should only emit once too'))Output: |
Summary: As part of my PR pytorch/pytorch#85303, I'm renaming `TypedStorage._storage` to `TypedStorage._untyped_storage` for better clarity, and since MultiPy depends on the old name, a CI job is failing on my PR. There is a public function `TypedStorage.untyped()` which is probably better to use in this case, and doing so will fix the CI failure for me cc ezyang Pull Request resolved: #221 Reviewed By: priyaramani Differential Revision: D41086182 Pulled By: PaliC fbshipit-source-id: 494a05c51b2e9ce29724f9bfa2728eee26e43ff7
kurtamohler
force-pushed
the
deprecate-TypedStorage
branch
from
893ba7d
to
7da5857
Compare
kurtamohler
force-pushed
the
deprecate-TypedStorage
branch
from
7da5857
to
9ec684c
Compare
|
@pytorchbot merge -f "upstream CI failure" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
BTW, before merging, I checked to make sure that the most recent update had similar warning counts in CI |
…methods (pytorch#85303) Part of pytorch#85302 Pull Request resolved: pytorch#85303 Approved by: https://github.com/ezyang
#85303 added a patch to `torch.testing.assert_close` to handle `torch.storage.TypedStorage`'s. This change is not reflected in the docs and is not intended for the public API. This PR removes the patch ones again and moves the behavior to `TestCase.assertEqual` instead. Meaning, `TypedStorage`'s are again not supported by the public API, but the behavior is the same for all internal use cases. Pull Request resolved: #89557 Approved by: https://github.com/kurtamohler, https://github.com/mruberry
Part of #85302
cc @albanD @mlazos @soumith @voznesenskym @yanboliang @penguinwu @anijain2305 @EikanWang @jgong5 @Guobing-Chen @chunyuan-w @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @desertfire @jansel @lezcano @fdrocha
BC-breaking note
Deprecate
torch.Tensor.storage()in favor oftorch.Tensor.untyped_storage()Version 1.13
Version 2.0
Deprecate
torch.TypedStorageand all its methods in favor oftorch.UntypedStorageVersion 1.13
Version 2.0
If you need to access individual elements in a storage as a particular dtype, you can simply create a tensor to view it: