Skip to content

Remove datatype from Storage and StorageImpl#38038

Closed
kurtamohler wants to merge 7 commits intopytorch:masterfrom
kurtamohler:untyped-StorageImpl-remove-dtype
Closed

Remove datatype from Storage and StorageImpl#38038
kurtamohler wants to merge 7 commits intopytorch:masterfrom
kurtamohler:untyped-StorageImpl-remove-dtype

Conversation

@kurtamohler
Copy link
Copy Markdown
Collaborator

@kurtamohler kurtamohler commented May 7, 2020 •
edited
Loading

  • Removed dtype data member from StorageImpl
  • Removed any methods or method arguments in Storage/StorageImpl that deal with dtypes
  • Update all callers of the changed API

Closes #33950

@kurtamohler kurtamohler requested a review from ezyang May 7, 2020 19:35
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label May 7, 2020
@dr-ci
Copy link
Copy Markdown

dr-ci Bot commented May 7, 2020 •
edited
Loading

💊 CI failures summary and remediations

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

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

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.

See how this bot performed.

This comment has been revised 45 times.

@kurtamohler
Copy link
Copy Markdown
Collaborator Author

kurtamohler commented May 7, 2020

Failures are real. I never noticed the tests in torch/test before. I had to do this to run those binaries locally: $ LD_LIBRARY_PATH=build/lib ./torch/test/blob_test
Is there a more standard/friendly way to run these tests?

@ezyang ezyang requested a review from anjali411 May 8, 2020 01:56
@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 8, 2020 

May 07 21:42:25 /var/lib/jenkins/workspace/caffe2/core/blob_test.cc:675: Failure
May 07 21:42:25 Expected equality of these values:
May 07 21:42:25   tensor_proto.data_type()
May 07 21:42:25     Which is: 0
May 07 21:42:25   TypeMetaToDataType(TypeMeta::Make<bool>())
May 07 21:42:25     Which is: 5
May 07 21:42:25 [  FAILED  ] EmptyTensorTest.TensorSerialization_bool (0 ms)
May 07 21:42:25 [ RUN      ] EmptyTensorTest.TensorSerialization_double

is real

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 8, 2020 


[ RUN      ] TensorGPUDeathTest/1.CannotAccessDataWhenEmpty
..\caffe2\core\blob_gpu_test.cc(121): error: Expected: tensor.data<TypeParam>() throws an exception of type EnforceNotMet.
  Actual: it throws nothing.
[  FAILED  ] TensorGPUDeathTest/1.CannotAccessDataWhenEmpty, where TypeParam = int (0 ms)
[----------] 1 test from TensorGPUDeathTest/1 (0 ms total)

too

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 8, 2020 


23:04:49 C++ exception with description "Cannot access data pointer of Tensor that doesn't have initialized dtype (e.g., caffe2::Tensor x(CPU), prior to calling mutable_data<T>() on x) (Error from operator: 
23:04:49 input: "mask1" input: "values1" input: "mask2" input: "values2" output: "unmasked_data" name: "test" type: "BooleanUnmask")
23:04:49 Exception raised from data at /var/lib/jenkins/workspace/c10/core/TensorImpl.h:621 (most recent call first):
23:04:49 frame #0: c10::Error::Error(c10::SourceLocation, std::string) + 0x4d (0x7fe0cea2f34d in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/lib/libc10.so)
23:04:49 frame #1: <unknown function> + 0x1e536a4 (0x7fe0fed146a4 in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/lib/libtorch_cpu.so)
23:04:49 frame #2: <unknown function> + 0x1ce56d5 (0x7fe0feba66d5 in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/lib/libtorch_cpu.so)
23:04:49 frame #3: caffe2::BooleanUnmaskTest_Test_Test::TestBody() + 0x50c (0x416a7c in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test)
23:04:49 frame #4: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 0x3a (0x49569a in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test)
23:04:49 frame #5: /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test() [0x48b776]
23:04:49 frame #6: /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test() [0x48bd2d]
23:04:49 frame #7: /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test() [0x48bfad]
23:04:49 frame #8: testing::internal::UnitTestImpl::RunAllTests() + 0xc69 (0x48d089 in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test)
23:04:49 frame #9: testing::UnitTest::Run() + 0x6e (0x48d2fe in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test)
23:04:49 frame #10: main + 0x37 (0x415427 in /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test)
23:04:49 frame #11: __libc_start_main + 0xf5 (0x7fe0cdcea505 in /lib64/libc.so.6)
23:04:49 frame #12: /var/lib/jenkins/.local/lib/python3.6/site-packages/torch/test/boolean_unmask_ops_test() [0x415bcd]

Theme seems to be Caffe2 compatibility code.

@kurtamohler kurtamohler force-pushed the untyped-StorageImpl-remove-dtype branch from c23b21c to 0e8e1fc Compare May 11, 2020 22:09
@kurtamohler
Copy link
Copy Markdown
Collaborator Author

kurtamohler commented May 12, 2020

The test_run_mypy failures in pytorch_linux_bionic_py3_6_clang9_test and pytorch_linux_xenial_py3_6_gcc5_4_test are happening in upstream, not introduced by this PR.

@kurtamohler kurtamohler force-pushed the untyped-StorageImpl-remove-dtype branch from 0e8e1fc to 8850815 Compare May 12, 2020 06:03
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/ATen/native/cuda/MiscUtils.h Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/ATen/native/TensorShape.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/ATen/native/cuda/TensorShapeCUDA.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/ATen/native/quantized/QTensor.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/ATen/quantized/QTensorImpl.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/TH/THStorageFunctions.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/TH/generic/THStorage.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/TH/generic/THStorage.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/TH/generic/THStorage.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/TH/generic/THStorage.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/TH/generic/THStorage.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/TH/generic/THTensor.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/THC/generic/THCStorage.cpp Outdated
ezyang
ezyang reviewed May 12, 2020
View reviewed changes
Comment thread aten/src/THC/generic/THCStorage.cpp Outdated
facebook-github-bot
facebook-github-bot reviewed May 13, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 13, 2020

This is definitely going to need fbcode-side changes; I'll let you know when I'm taking it over.

peterbell10
peterbell10 requested changes May 13, 2020
View reviewed changes
Comment thread aten/src/TH/generic/THTensor.cpp Outdated
Comment thread aten/src/TH/generic/THTensor.cpp Outdated
Comment thread aten/src/TH/generic/THTensor.cpp Outdated
Comment thread torch/csrc/generic/Storage.cpp Outdated
Comment thread torch/csrc/utils/tensor_new.cpp Outdated
Comment thread torch/csrc/utils/tensor_new.cpp Outdated
Comment thread tools/autograd/templates/python_variable_methods.cpp Outdated
Comment thread tools/autograd/templates/python_variable_methods.cpp Outdated
Comment thread tools/autograd/templates/python_variable_methods.cpp Outdated
@kurtamohler kurtamohler force-pushed the untyped-StorageImpl-remove-dtype branch from 32830f9 to 9b3618c Compare May 13, 2020 21:20
@kurtamohler kurtamohler force-pushed the untyped-StorageImpl-remove-dtype branch from 9b3618c to 52b7325 Compare May 14, 2020 01:03
facebook-github-bot
facebook-github-bot reviewed May 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@kurtamohler kurtamohler mentioned this pull request May 14, 2020
Closed
@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 15, 2020

OK, I'll be working on handling fbcode side tomorrow!

@kurtamohler
Copy link
Copy Markdown
Collaborator Author

kurtamohler commented May 15, 2020

@ezyang, awesome, thanks!

facebook-github-bot
facebook-github-bot reviewed May 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang ezyang added the module: bc-breaking Related to a BC-breaking change label May 19, 2020
@ezyang ezyang mentioned this pull request May 21, 2020
Closed
ezyang added a commit that referenced this pull request May 21, 2020
@ezyang
[pytorch][PR] Remove datatype from Storage and StorageImpl
7a0e353 
* Removed dtype data member from StorageImpl
* Removed any methods or method arguments in Storage/StorageImpl that deal with dtypes
* Update all callers of the changed API

Part of issue #33950
Original PR: #38038

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21549645/)!

[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 21, 2020
@ezyang
[pytorch][PR] Remove datatype from Storage and StorageImpl
42346b1 
* Removed dtype data member from StorageImpl
* Removed any methods or method arguments in Storage/StorageImpl that deal with dtypes
* Update all callers of the changed API

Part of issue #33950
Original PR: #38038

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21549645/)!

ghstack-source-id: 104434358
Pull Request resolved: #38870
ezyang added a commit that referenced this pull request May 21, 2020
@ezyang
Update on "[pytorch][PR] Remove datatype from Storage and StorageImpl"
90856e5 
* Removed dtype data member from StorageImpl
* Removed any methods or method arguments in Storage/StorageImpl that deal with dtypes
* Update all callers of the changed API

Part of issue #33950
Original PR: #38038

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21549645/)!

[ghstack-poisoned]
ezyang added a commit that referenced this pull request May 21, 2020
@ezyang
[pytorch][PR] Remove datatype from Storage and StorageImpl
aa45245 
Pull Request resolved: #38870

* Removed dtype data member from StorageImpl
* Removed any methods or method arguments in Storage/StorageImpl that deal with dtypes
* Update all callers of the changed API

Part of issue #33950
Original PR: #38038

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

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D21549645/)!
ghstack-source-id: 104517155
facebook-github-bot pushed a commit that referenced this pull request May 21, 2020
@kurtamohler @facebook-github-bot
Remove datatype from Storage and StorageImpl (#38870)
f9eb882 
Summary:
Pull Request resolved: #38870

* Removed dtype data member from StorageImpl
* Removed any methods or method arguments in Storage/StorageImpl that deal with dtypes
* Update all callers of the changed API

Part of issue #33950
Original PR: #38038

Reviewed By: albanD

Differential Revision: D21549645

Pulled By: ezyang

fbshipit-source-id: 4289b356c55ff6b9530376a79343b99b540ee3de
@ezyang
Copy link
Copy Markdown
Contributor

ezyang commented May 26, 2020

Superseded by #38870

@ezyang ezyang closed this May 26, 2020
laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request Apr 24, 2026
@kurtamohler @facebook-github-bot
Remove datatype from Storage and StorageImpl (pytorch#38870)
194e23c 
Summary:
Pull Request resolved: pytorch#38870

* Removed dtype data member from StorageImpl
* Removed any methods or method arguments in Storage/StorageImpl that deal with dtypes
* Update all callers of the changed API

Part of issue pytorch#33950
Original PR: pytorch#38038

Reviewed By: albanD

Differential Revision: D21549645

Pulled By: ezyang

fbshipit-source-id: 4289b356c55ff6b9530376a79343b99b540ee3de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

@peterbell10 peterbell10 peterbell10 approved these changes

@apaszke apaszke Awaiting requested review from apaszke

@mrshenli mrshenli Awaiting requested review from mrshenli

@pietern pietern Awaiting requested review from pietern

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

@anjali411 anjali411 Awaiting requested review from anjali411

+1 more reviewer

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: bc-breaking Related to a BC-breaking change oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make StorageImpl untyped for non-POD types

6 participants

@kurtamohler @ezyang @anjali411 @facebook-github-bot @peterbell10 @pytorchbot