Running crypten computation either with plans or jailing by youben11 · Pull Request #3410 · OpenMined/PySyft

youben11 · 2020-04-26T16:07:06Z

Description

At the moment we have two ways of running crypten computations, either using plans or jails, instead of having two different branches that have similar functionalities, better to be able to use both in parallel.

This PR introduce a new message type CryptenInitJail to start a JailRunner on other syft workers, need to merge into syft-proto OpenMined/syft-proto#93. This way we can send messages to initiate either plans or jails.

Type of change

Please mark options that are relevant.

Added/Modified tutorials
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

I have added tests for my changes
I did follow the contribution guidelines
I have commented my code following Google style.

LaRiffle

Tiny changes and we're done :)

LaRiffle · 2020-04-26T20:08:36Z

syft/messaging/message.py

+    so they can communicate and exchange tensors and shares while doing computation. This message
+    allows the exchange of information such as the ip and port of the master party to connect to,
+    as well as the rank of the party to run and the number of parties involved. Compared to
+    CryptenInitPlan, this message also send two extra field, a JailRunner and a crypten model."""


Suggested change

CryptenInitPlan, this message also send two extra field, a JailRunner and a crypten model."""

CryptenInitPlan, this message also sends two extra fields, a JailRunner and a Crypten model."""

LaRiffle · 2020-04-26T20:10:09Z

syft/messaging/message.py

+        return (self.crypten_context,)
+
+    @staticmethod
+    def simplify(worker: AbstractWorker, ptr: "CryptenInitJail") -> tuple:


Maybe rename ptr -> message ?

LaRiffle · 2020-04-26T20:11:15Z

syft/messaging/message.py

+        context = msg_tuple[0:4]
+        jail_runner = msg_tuple[4]
+        model = msg_tuple[5]


Suggested change

context = msg_tuple[0:4]

jail_runner = msg_tuple[4]

model = msg_tuple[5]

*context, jail_runner, model = msg_tuple

LaRiffle · 2020-04-26T20:13:21Z

syft/workers/base.py

+                rank = r
+                break
+
+        assert rank != None


Suggested change

assert rank != None

assert rank is not None

LaRiffle · 2020-04-26T20:14:41Z

syft/messaging/message.py

+    CryptenInitPlan, this message also send two extra field, a JailRunner and a crypten model."""
+
+    def __init__(self, crypten_context, jail_runner, model=None):
+        self.crypten_context = crypten_context


I guess one example of Crypten context would be useful

gmuraru · 2020-04-28T22:50:20Z

syft/frameworks/torch/hook/hook.py

@@ -141,7 +141,8 @@ def __init__(
        self._hook_native_tensor(torch.Tensor, TorchTensor)

        if dependency_check.crypten_available:


Q: Should this be removed?

Oh, I shouldn't have pushed that, I was only commenting this to not hook mpc tensor to get cat and stuff working, will just re-enable it for the moment

* Context of computation for crypten (#2963) * New message type for crypten party initialization * Handle CryptenInit message by running local party * Define a new context of computation for crypten The new context of computation can run parties that are distributed across syft workers. The communication of crypten parties remains the same, however, syft workers handle initialization and the serialization of return values. * updated docs * testing crypten context * fix: was setting the bad env variable DISTRIBUTED_BACKEND should have been set and not BACKEND * test serde of CryptenInit message * add crypten as core deps, this should change to extra * delete useless comment * fix: list CryptenInit in OBJ_SIMPLIFIER_AND_DETAILERS * fix msgpack tests * run black * don't cover empty function * Init Crypten Plan * Remove python 3.6 from github actions * Remove SyftCrypTensor as wrapper from MPCTensor * Remove SyftCrypTensor from export * Check for is_wrapper in placeholder * Add placeholder attribute * Bug Fix Message * Add serde test * Old CrypTen version * Add crypten to execution * wrap our load function around crypten.load * Update syft/frameworks/crypten/__init__.py Co-Authored-By: Ayoub Benaissa <ayouben9@gmail.com> * Rework crypten integration for plans * linter fix * Remove duplicate test Co-Authored-By: Ayoub Benaissa <ayouben9@gmail.com> * Crypten track master * Remove SyftCryptensor as a dependency * Remove python 3.6 * Use correct commit * Add MPCTensor as FrameworkTensor and remove SyftCrypTensor * remove duplicated CrypTenInit * Review pass1 Co-authored-by: Ayoub Benaissa <ayouben9@gmail.com>

respectively

LaRiffle

Thanks for the changes!

pip-dep/requirements.txt

gmuraru · 2020-05-05T17:43:35Z

syft/messaging/message.py

+    def detail(worker: AbstractWorker, msg_tuple: tuple) -> "CryptenInitJail":
+        """
+        This function takes the simplified tuple version of this message and converts
+        it into an CryptenInitJail. The simplify() method runs the inverse of this method.


TYPO: an -> a.

gmuraru · 2020-05-05T17:44:22Z

syft/workers/base.py

                break

-        assert rank != None
+        assert rank is not None


Thank you for fixing this :D

gmuraru

LGTM!

youben11 added the CrypTen label Apr 26, 2020

youben11 requested review from a team, LaRiffle and gmuraru April 26, 2020 16:07

youben11 mentioned this pull request Apr 26, 2020

CryptenInit messages OpenMined/syft-proto#93

Merged

youben11 added the work-in-progress label Apr 26, 2020

LaRiffle suggested changes Apr 26, 2020

View reviewed changes

youben11 requested a review from LaRiffle April 27, 2020 04:25

gmuraru reviewed Apr 28, 2020

View reviewed changes

youben11 removed the work-in-progress label Apr 28, 2020

gmuraru force-pushed the crypten branch 2 times, most recently from 91acf35 to 5b69d19 Compare April 29, 2020 05:30

gmuraru and others added 6 commits May 1, 2020 05:12

new message CryptenInitJail and CryptenInit -> CryptenInitPlan

10b3ea7

choice to run jails or plans by decorating a function or a plan

2ea60c2

respectively

typos and small changes

e209d45

reformatting

0bdbe59

re-enable mpc hooking

6499207

youben11 force-pushed the youben11/run-jail branch from 6ead606 to 6499207 Compare May 1, 2020 04:13

LaRiffle approved these changes May 3, 2020

View reviewed changes

youben11 added 6 commits May 3, 2020 13:38

Merge branch 'crypten' into youben11/run-jail

9b47602

lint

3670df8

update tests

4ffbff0

remove old comments

6cdb131

fix serialization CryptenInitJail and tests

9a06726

lint

248cb2c

youben11 commented May 5, 2020

View reviewed changes

pip-dep/requirements.txt Outdated Show resolved Hide resolved

Update pip-dep/requirements.txt

016dce4

gmuraru reviewed May 5, 2020

View reviewed changes

syft/workers/base.py

break

assert rank != None

assert rank is not None

Copy link

Member

gmuraru May 5, 2020

Choose a reason for hiding this comment

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

Thank you for fixing this :D

typos

8b2933c

gmuraru approved these changes May 5, 2020

View reviewed changes

gmuraru merged commit 7a5420b into crypten May 5, 2020

gmuraru deleted the youben11/run-jail branch May 5, 2020 18:16

	CryptenInitPlan, this message also send two extra field, a JailRunner and a crypten model."""
	CryptenInitPlan, this message also sends two extra fields, a JailRunner and a Crypten model."""

		@@ -141,7 +141,8 @@ def __init__(
		self._hook_native_tensor(torch.Tensor, TorchTensor)

		if dependency_check.crypten_available:

Uh oh!

Conversation

youben11 commented Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist:

Uh oh!

LaRiffle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaRiffle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmuraru left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

youben11 commented Apr 26, 2020 •

edited

Loading