Bugfix/type safety #19
Conversation
| @@ -8,6 +8,7 @@ class MyConvModule(nn.Module): | |||
|
|
|||
| def __init__(self) -> None: | |||
| """Contains some ConvNet components""" | |||
| super().__init__() | |||
There was a problem hiding this comment.
note: this is not strictly mandatory for us but it is in torch. so just copied this here so that the syntax is exactly equal
There was a problem hiding this comment.
Makes sense, but at the same time I think having a direct coupling makes it much easier.
Setting this as a reminder to update the Nillion Docs on this regard.
There was a problem hiding this comment.
wdym by this?
examples/linear_regression/main.py
Outdated
| @@ -162,7 +157,7 @@ async def main(): | |||
| nillion.Secrets({}), | |||
| ) | |||
| # Rescale the obtained result by the quantization scale | |||
| outputs = [na_client.float_from_rational(result["my_output_0"])] | |||
| outputs = [na_client.float_from_rational(result["my_output"])] | |||
There was a problem hiding this comment.
note: this occurs b/c we decided to mirror NumPy's behaviour of matrix ops not necessarily returning other matrices
There was a problem hiding this comment.
It's okay. I think that the more we mimick Numpy the better.
|
Note: ci/cd fails because it relies on pre-release nada-algebra changes |
| @@ -22,4 +23,4 @@ def nada_main(): | |||
| result = my_model.forward(my_input) | |||
|
|
|||
| # Step 6: We can use result.output() to produce the output for Party1 and variable name "my_output" | |||
| return result.output(parties[1], "my_output") | |||
| return [Output(result.value, "my_output", parties[1])] | |||
There was a problem hiding this comment.
Suggestion here. We have the na.output function which prevents from having to put result.value if it is a rational number.
na.output(result, "my_output", parties[1])
There was a problem hiding this comment.
I will try this but if it works then IMO the type hinting for na.output needs to change as currently type na.NadaArray is listed as the only accepted input dtype (which is why i didn't use it initially)
There was a problem hiding this comment.
I agree with changing the type hinting not to have the NadaArray only. I think is good to have a single point of output that handles everything no matter what you put inside.
There was a problem hiding this comment.
fixed this but flagging to address that type hint in a separate nada-numpy PR at some point
| @@ -11,16 +11,16 @@ def nada_main(): | |||
|
|
|||
| class TestModule1(Module): | |||
| def __init__(self) -> None: | |||
| self.param1 = Parameter((3, 2)) | |||
| self.param2 = Parameter(3) | |||
| self.param1 = Parameter(na.zeros((3, 2), na.Rational)) | |||
There was a problem hiding this comment.
Leaving this comment here, but it may apply to other places. Also, feel free to disagree. I have seen that most (if not all) of the times, we initialize Parameter(na.zeros(..., ensure_cleartext(...))). Wouldn't it make sense to do the na.zeros(, ensure_cleartext()) in the Parameter.__init__.
Again, please, feel free to disagree.
There was a problem hiding this comment.
we could but pytorch also does it this way
(side note: realistically it's very uncommon for users to interface with the Parameter class directly)
|
|
||
| def test_parameters_9(self): | ||
| class TestModule(Module): | ||
| def __init__(self) -> None: | ||
| self.param = Parameter((2, 3)) | ||
| self.param = Parameter((2, 2)) | ||
| self.param = Parameter(na.zeros((2, 3))) |
There was a problem hiding this comment.
Just saw this that contradicts my previous comment. So, no problem if you feel like its should be this way.
Enforce strong type guardrails introduced in latest version of
nada-algebra/nada-numpy