Add Crypten changes to master #3377
Conversation
|
What bug(s) does this fix? |
| @@ -60,6 +60,11 @@ def trace_wrapper(*args, **kwargs): | |||
| recording sub actions | |||
| """ | |||
|
|
|||
| if not hasattr(syft, "hook") or syft.hook == None: | |||
| # Make sure that in the "hooking process" we do not access the | |||
| # syft.hook because it is not initialized | |||
There was a problem hiding this comment.
When was this happening?
There was a problem hiding this comment.
Copy pasted from the Crypten branch merge :3
The trace for the error was this:
@pytest.fixture(scope="session", autouse=True)
def hook():
> hook = TorchHook(torch)
test/conftest.py:130:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
syft/frameworks/torch/hook/hook.py:145: in __init__
self._hook_native_tensor(crypten.mpc.MPCTensor, TorchTensor)
syft/frameworks/torch/hook/hook.py:277: in _hook_native_tensor
self._hook_native_methods(tensor_type)
syft/generic/frameworks/hook/hook.py:102: in _hook_native_methods
native_method = getattr(tensor_type, attr)
../../../miniconda3/envs/pysyft/lib/python3.7/site-packages/crypten/cryptensor.py:39: in __getattribute__
dummy = cls(None)
../../../miniconda3/envs/pysyft/lib/python3.7/site-packages/crypten/mpc/mpc.py:73: in __init__
super().__init__(requires_grad=requires_grad)
../../../miniconda3/envs/pysyft/lib/python3.7/site-packages/crypten/cryptensor.py:132: in __init__
self._reset_gradients()
What (I think is happening) -- when we hook we use the getattr -- but that method is changed in the CrypTensorMetaclass (metaclass for the MPCTensor). The problem is when we hook a method and that method is called in the getattr (in getattr we call an already hooked method and we do not have the syft.hook initialized -- syft.hook is initialized at the end of the TorchHook init method)
| @@ -27,6 +27,19 @@ def __init__(self, owner=None, id=None, tags: set = None, description: str = Non | |||
| self.expected_shape = tuple(shape) if shape is not None else None | |||
| self.child = None | |||
|
|
|||
| def __getattribute__(self, name): | |||
There was a problem hiding this comment.
Remind me what this was for? IIRC, it was proposed as a way to do tracing, but maybe it has other uses too. 🤔
There was a problem hiding this comment.
This one is for accessing methods that are necessary in the AbstractTensor class (the PlaceHolder is a child of that class).
In case we have PlaceHolder > CrypTensor, we might call methods from CrypTensor that are not necessary for PlaceHolder.
Another way to do this is to hook all methods from CrypTensor (this to be done in the crypten branch), but in that scenario, we might have methods that might not be generic -- like get_plain_text (this is implemented only in CrypTensor)
There was a problem hiding this comment.
Medium-term plan for Placeholder is to make it a plain wrapper that no longer inherits from AbstractTensor and uses roughly this approach to forwarding method calls, so I'm not opposed in general. Just wondering why we need it in master if it's currently only used for Crypten, which couldn't be merged into master last I knew.
There was a problem hiding this comment.
I was thinking it is a good idea to sync with master the changes that affect the plans, roles or core functionalities (such that the master will have those changes as fast as possible).
Those changes are some (possible) solutions to some problems we faced when merging some PRs.
I was thinking it might be a good idea to have those changes (that affect the syft core) into the master branch.
Codecov Report
@@ Coverage Diff @@
## master #3377 +/- ##
==========================================
+ Coverage 94.66% 94.72% +0.05%
==========================================
Files 150 151 +1
Lines 15917 16337 +420
==========================================
+ Hits 15068 15475 +407
- Misses 849 862 +13
|
| @@ -164,5 +164,5 @@ def distribute_dataset(data, workers): | |||
| print("Done!") | |||
|
|
|||
|
|
|||
| def hook(gbs): | |||
| def make_hook(gbs): | |||
There was a problem hiding this comment.
It is not necessarily a bug, but it might cause a name collision when import hook from syft (in case we do not have the hook initialized syft.hook will be a function)
gmuraru
force-pushed
the
gm-crypten-master
branch
from
a11398f
to
40daaef
Compare
gmuraru
force-pushed
the
gm-crypten-master
branch
from
40daaef
to
0b0f3df
Compare
Description
Update master with the core changes made on the crypten branch
Type of change
Please mark options that are relevant.
Checklist: