Skip to content

Don't serialize hooks #11705

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.

Sign up for GitHub

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

Jump to bottom
Closed
ezyang wants to merge 8 commits into from
Closed

Don't serialize hooks #11705

ezyang wants to merge 8 commits into from

Conversation

@ezyang
Copy link
Contributor

@ezyang ezyang commented Sep 14, 2018

Fixes #11683.

Signed-off-by: Edward Z. Yang ezyang@fb.com

@ezyang ezyang requested a review from apaszke September 14, 2018 16:12
@ezyang
Copy link
Contributor Author

ezyang commented Sep 14, 2018

CC @PetrochukM

facebook-github-bot
facebook-github-bot reviewed Sep 14, 2018
View reviewed changes
Copy link
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.

apaszke
apaszke reviewed Sep 14, 2018
View reviewed changes
torch/multiprocessing/reductions.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ssnl
Copy link
Collaborator

ssnl commented Sep 25, 2018

@ezyang @apaszke How should we proceed with this?

@ezyang
Copy link
Contributor Author

ezyang commented Sep 27, 2018

I'm going to add a test for saving a nccl-parallelized model, and then we'll call it a day.

@ezyang ezyang force-pushed the pr/dont-serialize-hooks branch from 6b2ec7c to d18b2d7 Compare September 28, 2018 13:58
apaszke
apaszke suggested changes Sep 28, 2018
View reviewed changes
Copy link
Contributor

@apaszke apaszke left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See my comment + accidental submodule update

test/test_distributed.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@ezyang ezyang changed the title Don't serialize hooks when transmitting tensors with multiprocessing. Don't serialize hooks Oct 1, 2018
@ezyang ezyang force-pushed the pr/dont-serialize-hooks branch 2 times, most recently from 2907fd1 to 4379934 Compare October 1, 2018 17:04
facebook-github-bot
facebook-github-bot reviewed Oct 1, 2018
View reviewed changes
Copy link
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
Copy link
Contributor Author

ezyang commented Oct 1, 2018

This PR is still missing warning reporting for when we would have serialized a hook but don't, but all the other pieces are here.

@ezyang ezyang force-pushed the pr/dont-serialize-hooks branch 2 times, most recently from 9cb0292 to 455019a Compare October 5, 2018 04:04
@ezyang
Copy link
Contributor Author

ezyang commented Oct 5, 2018

Warnings added

facebook-github-bot
facebook-github-bot reviewed Oct 5, 2018
View reviewed changes
Copy link
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
Copy link
Contributor Author

ezyang commented Oct 5, 2018

@pytorchbot retest this please

@ezyang ezyang mentioned this pull request Oct 5, 2018
Closed
facebook-github-bot
facebook-github-bot reviewed Oct 5, 2018
View reviewed changes
Copy link
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.

ssnl
ssnl reviewed Oct 8, 2018
View reviewed changes
test/test_distributed.py Outdated

This comment was marked as off-topic.

Sign in to view

test/test_thd_distributed.py Outdated

This comment was marked as off-topic.

Sign in to view

torch/utils/hooks.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/utils/hooks.py Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ezyang added 5 commits October 8, 2018 15:36
@ezyang
Don't serialize hooks.
06fbc9e 
Fixes pytorch#11683.

Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Skip gloo test
b39104c 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Preliminary hooks warnings, to flush out errors
25359d8 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
switch to warnings maybe hu
b9666c4 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
Suppress one more warning site
636e53e 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
ezyang added 2 commits October 8, 2018 15:36
@ezyang
one more
fd7bdf2 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang
CR
f8cac60 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
@ezyang ezyang force-pushed the pr/dont-serialize-hooks branch from 791a4ec to f8cac60 Compare October 8, 2018 23:26
apaszke
apaszke reviewed Oct 9, 2018
View reviewed changes
Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

Some tests leak resources

torch/nn/parallel/deprecated/distributed.py Outdated
self.bucket_events[bucket_idx][device_idx] = event
self._queue_reduction(bucket_idx)

distributed_data_parallel_hook._torch_unserializable = True

This comment was marked as off-topic.

Sign in to view

test/test_multiprocessing.py Outdated
pass

# Shut up warnings
hook._torch_unserializable = True

This comment was marked as off-topic.

Sign in to view

test/test_distributed.py Outdated
# TODO: It should be possible to save the entire model,
# but this doesn't work at the moment. Update this test
# when it does work.
tmp_file = tempfile.TemporaryFile()

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

test/test_thd_distributed.py Outdated
# Test that saving and loading work
# gloo serialization doesn't work, see #12261
if BACKEND != "gloo":
tmp_file = tempfile.TemporaryFile()

This comment was marked as off-topic.

Sign in to view

@ezyang
CR
1af342e 
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
facebook-github-bot
facebook-github-bot reviewed Oct 16, 2018
View reviewed changes
Copy link
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 mentioned this pull request Oct 16, 2018
Closed
@ezyang
Copy link
Contributor Author

ezyang commented Oct 16, 2018

@apaszke Ready to go?

@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@ssnl ssnl ssnl approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+1 more reviewer

@apaszke apaszke apaszke requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't pickle local object 'DistributedDataParallel._register_nccl_grad_hook.

4 participants

@ezyang @ssnl @apaszke @facebook-github-bot