Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable RRef proxy helpers to work with ScriptModule #48294

Closed
rohan-varma opened this issue Nov 20, 2020 · 1 comment
Closed

Enable RRef proxy helpers to work with ScriptModule #48294

rohan-varma opened this issue Nov 20, 2020 · 1 comment
Labels
better-engineering Relatively self-contained tasks for better engineering contributors module: rpc Related to RPC, distributed autograd, RRef, and distributed optimizer oncall: jit Add this issue/PR to JIT oncall triage queue triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@rohan-varma
Copy link
Member

rohan-varma commented Nov 20, 2020 •
edited by pytorch-probot bot

馃殌 Feature

We are adding support for transferring ScriptModule over the wire in RPC, see #48293. However, this does not currently work with RRef helper, due to the check on this line: https://github.com/pytorch/pytorch/blob/master/torch/distributed/rpc/rref_proxy.py#L18.

There is a workaround, however, which is to implement the helper without the check in user code:

def run(rref, func_name, args, kwargs):
    return getattr(rref.local_value(), func_name)(*args, **kwargs)

def rref_isinstance(rref, cls_to_check):
    return isinstance(rref.local_value(), cls_to_check)

# Ensure rref is a scriptModule.
ret = rpc.rpc_sync(remote_script_module.owner(), rref_isinstance, args=(remote_script_module, torch.jit.ScriptModule))
assert ret
# Run remote forward pass
remote_forward_output = rpc.rpc_sync(remote_script_module.owner(), run, args=(remote_script_module, "forward", (), {}))

Proposal is instead of checking for the exact type, such as type(x) is A which is what we are doing right now, we check the equivalent of isinstance(x, A) for the rref.

cc @pietern @mrshenli @pritamdamania87 @zhaojuanmao @satgera @gqchen @aazzolini @rohan-varma @jjlilley @osalpekar @jiayisuse @gmagogsfm
@rohan-varma rohan-varma added triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module better-engineering Relatively self-contained tasks for better engineering contributors module: rpc Related to RPC, distributed autograd, RRef, and distributed optimizer labels Nov 20, 2020
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Nov 20, 2020
@github-actions github-actions bot added this to Need triage in JIT Triage Nov 20, 2020
@rohan-varma rohan-varma mentioned this issue Nov 20, 2020
Closed
rohan-varma added a commit that referenced this issue Nov 20, 2020
@rohan-varma
RRef proxy support for ScriptModule methods
62e7e90 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented:
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Nov 20, 2020
@rohan-varma
Update on "RRef proxy support for ScriptModule methods"
541b0c1 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented:
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Nov 20, 2020
@rohan-varma
RRef proxy support for ScriptModule methods
ff2d7ec 
Pull Request resolved: #48339

Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented:
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```
ghstack-source-id: 117283506

Differential Revision: [D25134423](https://our.internmc.facebook.com/intern/diff/D25134423/)
rohan-varma added a commit that referenced this issue Nov 29, 2020
@rohan-varma
Update base for Update on "RRef proxy support for ScriptModule methods"
0732b0d 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Nov 29, 2020
@rohan-varma
Update on "RRef proxy support for ScriptModule methods"
3168e26 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Nov 29, 2020
@rohan-varma
RRef proxy support for ScriptModule methods
685e5e0 
Pull Request resolved: #48339

Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```
ghstack-source-id: 117428758

Differential Revision: [D25134423](https://our.internmc.facebook.com/intern/diff/D25134423/)
rohan-varma added a commit that referenced this issue Nov 29, 2020
@rohan-varma
Update base for Update on "RRef proxy support for ScriptModule methods"
10aa47d 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Nov 29, 2020
@rohan-varma
Update on "RRef proxy support for ScriptModule methods"
8550026 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Nov 29, 2020
@rohan-varma
RRef proxy support for ScriptModule methods
ddb409a 
Pull Request resolved: #48339

Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```
ghstack-source-id: 117431386

Differential Revision: [D25134423](https://our.internmc.facebook.com/intern/diff/D25134423/)
@gmagogsfm
Copy link
Contributor

Doesn't seem like this issue needs any triage from JIT side. Removing from JIT triage queue.

@gmagogsfm gmagogsfm removed this from Need triage in JIT Triage Dec 1, 2020
rohan-varma added a commit that referenced this issue Dec 2, 2020
@rohan-varma
Update base for Update on "RRef proxy support for ScriptModule methods"
986c3bf 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Dec 2, 2020
@rohan-varma
Update on "RRef proxy support for ScriptModule methods"
5fe95ab 
Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```

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

[ghstack-poisoned]
rohan-varma added a commit that referenced this issue Dec 2, 2020
@rohan-varma
RRef proxy support for ScriptModule methods
ffb5cdd 
Pull Request resolved: #48339

Closes #48294
#48293 added creation and transfer of ScriptModule over RPC in python, but it did not work with ScriptModule.

This PR makes the above work with ScriptModule as per a discussion with @mrshenli:
1) We remove the `hasattr()` check and just let Python throw the exception as it would when accessing the py function with `getattr`
2) We  condition on `issubclass(type, ScriptModule)` when checking if it is wrapped with async_function, because `ScriptModule` does not have getattr implemented (this is because ScriptModule forward/function is not a python function, it is a torchscript specific function):
```
torch/jit/_script.py", line 229, in __get__
    return self.__getattr__("forward")  # type: ignore
AttributeError: '_CachedForward' object has no attribute '__getattr__'
```
ghstack-source-id: 117631795

Differential Revision: [D25134423](https://our.internmc.facebook.com/intern/diff/D25134423/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
better-engineering Relatively self-contained tasks for better engineering contributors module: rpc Related to RPC, distributed autograd, RRef, and distributed optimizer oncall: jit Add this issue/PR to JIT oncall triage queue triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gmagogsfm @facebook-github-bot @rohan-varma