Removed wrap() on non-tensor pointers in respond_to_search()
#3685
Conversation
AlanAboudib
added
Priority: 1 - Immediate 🔥
syft/workers/message_handler.py
Outdated
| # Wrap only if the pointer points to a tensor. | ||
| # If it points to a generic object, do not wrap. | ||
| if isinstance(obj, PointerTensor): | ||
| ptr.wrap() |
There was a problem hiding this comment.
Q: if you remove this if completely are the tests failing?
There was a problem hiding this comment.
Just pushed that to test
There was a problem hiding this comment.
Yeah...it is failling :(, it was ok before 👍
There was a problem hiding this comment.
I reverted my changes and it still giving an error :O
There was a problem hiding this comment.
@AlanAboudib it seemed to be an intermittent and was not the result of your change.It was happening because of timeout when waiting for an async response. I re-ran the tests and all tests passed. You can make your change and then we can debug further if the issue happens again.
There was a problem hiding this comment.
I think @shubham3121 referred to the changes with
# Wrap only if the pointer points to a tensor.
# If it points to a generic object, do not wrap.
if isinstance(obj, PointerTensor):
ptr.wrap()
There was a problem hiding this comment.
I was trying your idea of just remove .wrap() without adding my code. I just push my changes back now
There was a problem hiding this comment.
@AlanAboudib , Ques: You want to wrap Tensor objects (not necessarily PointerTensors objects) and not non Tensor objects, is that the behaviour you're looking for ??
If so, then the check should be isinstance(obj, torch.Tensor). Because the currently the test is failing because the obj is an Tensor instance and not a PointerTensor object, as a result we are not wrapping it and hence results in error on calling .shape.
There was a problem hiding this comment.
Oh, my bad, I actually wanted to type:
if isinstance(ptr, PointerTensor):
Your solution works too. I will do
Thank you so much
There was a problem hiding this comment.
@AlanAboudib ptr.wrap() should prt = ptr.wrap() as it is not an inline operation.
Codecov Report
@@ Coverage Diff @@
## master #3685 +/- ##
==========================================
+ Coverage 94.77% 94.80% +0.03%
==========================================
Files 184 185 +1
Lines 18231 18339 +108
==========================================
+ Hits 17278 17387 +109
+ Misses 953 952 -1
|
syft/workers/message_handler.py
Outdated
| # Wrap only if the pointer points to a tensor. | ||
| # If it points to a generic object, do not wrap. | ||
| if isinstance(obj, PointerTensor): | ||
| ptr.wrap() |
There was a problem hiding this comment.
@AlanAboudib ptr.wrap() should prt = ptr.wrap() as it is not an inline operation.
Description
in
syft/workers/message_handlers:respond_to_search(), all pointers are wrapped before being returned. This should be the case only onPointerTensorobjects, not only all pointers.This causes a fail in SyferText where we need to exchange object pointers that are not tensors.
Affected Dependencies
None
How has this been tested?
All existing tests
Checklist