Skip to content

Conversation

zzzwen
Copy link
Contributor

@zzzwen zzzwen commented Nov 20, 2019

Stack from ghstack:

To enable share_memory over RPC, add an internal helper that overrides the default RPC pickler.
Replace D18598974

Differential Revision: D18621372

To enable share_memory over RPC, add an internal helper that overrides the default RPC pickler.
Replace D18598974

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

[ghstack-poisoned]
zzzwen pushed a commit that referenced this pull request Nov 20, 2019
To enable share_memory over RPC, add an internal helper that overrides the default RPC pickler.
Replace D18598974

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

ghstack-source-id: 94299660
Pull Request resolved: #30185
@zzzwen
Copy link
Contributor Author

zzzwen commented Nov 20, 2019

Replaced #30173

@zzzwen zzzwen requested a review from aazzolini November 20, 2019 22:10
Copy link
Contributor

@pritamdamania87 pritamdamania87 left a comment

Choose a reason for hiding this comment

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

@pietern @satgera We're going with this approach in the very short term to unblock @zzzwen since there are some issues he is running into with _apply and we need to complete the integration soon. We'll try to resolve the issues with the _apply method in the mean time and remove this API eventually.

@@ -14,6 +14,7 @@
from common_utils import load_tests
import dist_utils
from dist_utils import dist_init
from torch.distributed.rpc.api import _use_rpc_pickler
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we put this in torch.distributed.rpc and not torch.distributed.rpc.api?

Copy link
Contributor

@mrshenli mrshenli left a comment

Choose a reason for hiding this comment

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

Let's land this. I will rebase #30217 when this is in.

@zzzwen zzzwen deleted the gh/zzzwen/1/head branch November 21, 2019 18:02
@pietern
Copy link
Contributor

pietern commented Nov 22, 2019

@pritamdamania87 Thanks for clarifying, makes sense. Can you file an issue so we don't lose track of removing this eventually? Retaining this is a liability in my opinion.

@pritamdamania87
Copy link
Contributor

@pietern Filed #30633

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants