CrypTen Message Handler #3676

gmuraru · 2020-06-06T22:39:55Z

Description

Add a message handler for the CrypTen framework that should route the messages to the correct method.

Affected Dependencies

List any dependencies that are required for this change.

How has this been tested?

The CrypTen tests are still passing

Checklist

I have followed the Contribution Guidelines and Code of Conduct
I have commented my code following the OpenMined Styleguide
I have labeled this PR with the relevant Type labels
My changes are covered by tests

review-notebook-app · 2020-06-06T22:40:01Z

Check out this pull request on

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

gmuraru · 2020-06-06T22:45:51Z

syft/frameworks/crypten/context.py


-            sy.local_worker.add_crypten_support()
-            sy.local_worker._set_rank_to_worker_id(rank_to_worker_id)
+            sy.local_worker.rank_to_worker_id = rank_to_worker_id


This is kind of ugly. This rank_to_worker_id is used in the load function from crypten/__init__py...Thinking how we could remove this or refactor the load function

We can add some functions that take a worker as argument

gmuraru · 2020-06-06T22:46:41Z

syft/__init__.py

 else:
    logger.info("TF Encrypted Keras not available.")

+if dependency_check.crypten_available:


Added this here and not in syft/generic/frameworks/types.py because of circular dependecies.

codecov · 2020-06-07T01:13:48Z

Codecov Report

Merging #3676 into crypten will decrease coverage by 0.06%.
The diff coverage is 93.62%.

@@             Coverage Diff             @@
##           crypten    #3676      +/-   ##
===========================================
- Coverage    94.78%   94.72%   -0.07%     
===========================================
  Files          184      193       +9     
  Lines        18214    18837     +623     
===========================================
+ Hits         17265    17843     +578     
- Misses         949      994      +45

Impacted Files	Coverage Δ
syft/execution/placeholder.py	`94.97% <ø> (-0.19%)`	⬇️
test/execution/test_placeholder.py	`100.00% <ø> (ø)`
syft/frameworks/crypten/message_handler.py	`44.44% <44.44%> (ø)`
syft/frameworks/crypten/utils.py	`92.30% <92.30%> (ø)`
syft/messaging/message.py	`92.83% <92.30%> (-0.06%)`	⬇️
syft/frameworks/crypten/hook/hook.py	`95.28% <95.28%> (ø)`
syft/frameworks/crypten/jail.py	`96.20% <96.20%> (ø)`
syft/frameworks/crypten/context.py	`97.05% <97.05%> (ø)`
syft/dependency_check.py	`88.88% <100.00%> (+1.38%)`	⬆️
syft/frameworks/torch/hook/hook.py	`92.65% <100.00%> (+0.23%)`	⬆️
... and 21 more

youben11

Wasn't the initial design to leave every worker choose the message handlers to use, instead of adding all registered message handler in syft.framework_message_handler?

gmuraru · 2020-06-07T07:09:17Z

Wasn't the initial design to leave every worker choose the message handlers to use, instead of adding all registered message handler in syft.framework_message_handler?

The initial design (like we had before) was like that (by using add_support from the BaseWorker).

Now we automatically add the support if we detect a framework (currently is only for CrypTen).

I tried to follow those steps from the discussion between us and @karlhigley:

And when adding a new messageHandler we do:
* Extend AbstractMessageHandler
* in the init_routing_table add our methods (routers) for the specific framework
* append a new instance of that class to our self.message_handlers

Or shouldn't we add the handlers automatically?

youben11 · 2020-06-07T07:15:15Z

yeah, that's what I meant, like every message_handler have a function for checking if the deps are okay, and every worker implements the message handlers he need (if the deps are okay for that specific message handler).

gmuraru · 2020-06-07T07:19:18Z

yeah, that's what I meant, like every message_handler have a function for checking if the deps are okay, and every worker implements the message handlers he need (if the deps are okay for that specific message handler).

Isn't that happening in syft/__init__.py? There I check if we have the framework installed and I add that framework to a variable framework_message_handlers

youben11 · 2020-06-07T12:56:42Z

yeah, but it is kind of decoupled from the message handler itself. I was thinking more like the message handler checking that in a static function, and workers using some handlers if their deps are okay.

gmuraru · 2020-06-07T21:58:39Z

syft/workers/base.py

        return framework.lower() in framework_packages
+
+
+BaseWorker.register_message_handlers()


@youben11 moved the logic to the worker level.
Something like this you meant?

@karlhigley what do you think about this?

youben11 · 2020-06-08T06:16:44Z

syft/workers/base.py

+    @staticmethod
+    def register_message_handlers():
+        if sy.dependency_check.crypten_available:
+            from syft.frameworks.crypten.message_handler import CryptenMessageHandler
+
+            BaseWorker._framework_message_handler["crypten"] = CryptenMessageHandler
+


No need for a function to do that, that attribute could be initialized at first, and message handlers would be appended only if the dependency check passes, which is something the message hander itself checks for (with a static function MessageHandler.is_supported() for example)

Hmmm...or couldn't we add this registry mechanism directly in the CryptenMessageHandler class file (if we import that file it means we have the dependencies hence we should add it to the _framework_message_handlers?)

@karlhigley what do you think?

LaRiffle · 2020-06-11T12:26:36Z

syft/frameworks/crypten/message_handler.py

+        rank = None
+        for r, worker_id in self.worker.rank_to_worker_id.items():
+            if worker_id == self.worker.id:
+                rank = r
+                break
+
+        assert rank is not None


This code is used twice, could be refactor. it?

Fix plans framework

49ac1a5

gmuraru changed the title ~~Gm frameworks fix~~ CrypTen Message Handler Jun 6, 2020

gmuraru changed the base branch from crypten to crypten-master June 6, 2020 22:40

gmuraru added 3 commits June 7, 2020 01:42

CrypTen Message Handler

2317602

Test fixup

96533f0

Black fixup

9b5248f

gmuraru force-pushed the gm-frameworks-fix branch from df97851 to 7f79b48 Compare June 6, 2020 22:43

gmuraru requested review from a team, Jasopaum and LaRiffle and removed request for a team June 6, 2020 22:43

gmuraru commented Jun 6, 2020

View reviewed changes

gmuraru requested review from karlhigley and youben11 June 6, 2020 22:47

gmuraru force-pushed the gm-frameworks-fix branch from 7f79b48 to 5f0b715 Compare June 7, 2020 00:36

Remove unused/undefined functions

4ff0c94

gmuraru force-pushed the gm-frameworks-fix branch from 5f0b715 to 4ff0c94 Compare June 7, 2020 00:36

youben11 reviewed Jun 7, 2020

View reviewed changes

Move message registration to worker level

4a6c9aa

gmuraru commented Jun 7, 2020

View reviewed changes

Lint

46563b6

youben11 reviewed Jun 8, 2020

View reviewed changes

youben11 changed the base branch from crypten-master to crypten June 8, 2020 09:50

Use global msgpack

4c65c75

youben11 added CrypTen Priority: 2 - High 😰 Should be fixed as quickly as possible, ideally within the current or following sprint labels Jun 11, 2020

LaRiffle reviewed Jun 11, 2020

View reviewed changes

youben11 added 2 commits June 13, 2020 19:07

factorize worker_id to rank translation

7cb4168

fix sequential model with last version of crypten

8b215ab

gmuraru merged commit 3583781 into OpenMined:crypten Jun 13, 2020

		return framework.lower() in framework_packages


		BaseWorker.register_message_handlers()

Uh oh!

CrypTen Message Handler #3676

CrypTen Message Handler #3676

Uh oh!

Conversation

gmuraru commented Jun 6, 2020

Description

Affected Dependencies

How has this been tested?

Checklist

Uh oh!

review-notebook-app bot commented Jun 6, 2020

Uh oh!

gmuraru Jun 6, 2020

Choose a reason for hiding this comment

Uh oh!

youben11 Jun 7, 2020

Choose a reason for hiding this comment

Uh oh!

gmuraru Jun 6, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

youben11 left a comment

Choose a reason for hiding this comment

Uh oh!

gmuraru commented Jun 7, 2020

Uh oh!

youben11 commented Jun 7, 2020

Uh oh!

gmuraru commented Jun 7, 2020

Uh oh!

youben11 commented Jun 7, 2020

Uh oh!

gmuraru Jun 7, 2020

Choose a reason for hiding this comment

Uh oh!

youben11 Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

gmuraru Jun 8, 2020

Choose a reason for hiding this comment

Uh oh!

LaRiffle Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jun 7, 2020 •

edited

Loading