Use `PlaceholderIds` in `Actions` and `Plans` (instead of full `Placeholders`) #3223

Jasopaum · 2020-03-18T22:32:20Z

Introduce a class that could be used as id instead of the current string | int.

The rationale behind that was to disambiguate id and string | int in messages and actions.
For instance, we could have only placeholder ids (as ObjectIds) in Plan's actions instead of the whole placeholders and we wouldn't take them for string or integers.

LaRiffle · 2020-03-20T12:39:34Z

syft/execution/placeholder.py

-            worker._tmp_placeholders[tensor_id] = tensor
-
-        return worker._tmp_placeholders[tensor_id]
+        return PlaceHolder(owner=worker, id=tensor_id, tags=tags, description=description)


How do you make sure you don't create 2 PlaceHolder with the same tags?

I don't, but I don't think it was done before neither

The check to avoid duplicate placeholders moves after the deserialization. Since Actions no longer directly contain Placeholders (which move to a field on Plan), we don't have to worry about deserializing the same Placeholder multiple times as long as the property on Plan doesn't contain duplicates.

LaRiffle · 2020-03-20T12:40:33Z

syft/execution/plan.py

        """
        if isinstance(obj, (tuple, list)):
-            r = [self.replace_with_placeholders(o, **kw) for o in obj]
+            r = [self.replace_with_placeholder_ids(o, **kw) for o in obj]


I'm not sure I understand the strategy: you're now storing ObjectIds instead of placeholders right? What are the benefits?

You remember the self.worker._tmp_placeholders variable that held placeholders during deserialization in order to avoid creating multiple identical Placeholders, because duplicate Placeholders breaks instantiation with actual tensors? This is a strategy for getting rid of that temp variable, by storing Placeholders at the Plan level and using that store to avoid duplication.

Using ids here makes it possible to deserialize without creating duplicate Placeholders and then index into the Placeholder store on the Plan to make sure that all the places that reference the same id use the same Placeholder object.

syft/execution/plan.py

karlhigley · 2020-03-21T17:09:08Z

syft/execution/plan.py

+        # Replace non-instanciated placeholders from plan.placeholders by instanciated placeholders
+        # from state.state_placeholders
+        # TODO this definitely isn't the right strategy. Maybe state shouldn't contain
+        # instanciated placeholders but values directly?


I'm kinda confused by this part. Not sure I have my head fully wrapped around why State contains Placeholders. 🤔

I asked Théo who told me that it was to avoid tensor-placeholder operations.
If state contains placeholders, then we can do operation between the plan's placeholders and the ones in its state.
But I agree, Placeholders shouldn't have anything to do with States as they're static

I think I'm okay with this strategy for now. Based on Slack conversations, sounds like we should leave State containing instantiated Placeholders for the time being.

codecov · 2020-03-21T22:40:47Z

Codecov Report

Merging #3223 into master will increase coverage by 0.04%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master    #3223      +/-   ##
==========================================
+ Coverage   94.54%   94.59%   +0.04%     
==========================================
  Files         146      147       +1     
  Lines       15897    15954      +57     
==========================================
+ Hits        15030    15091      +61     
+ Misses        867      863       -4

Impacted Files	Coverage Δ
syft/execution/computation.py	`93.33% <84.21%> (-1.12%)`	⬇️
syft/execution/placeholder_id.py	`96.42% <96.42%> (ø)`
syft/execution/placeholder.py	`91.22% <100.00%> (-0.96%)`	⬇️
syft/execution/plan.py	`92.79% <100.00%> (+2.91%)`	⬆️
syft/execution/state.py	`91.22% <100.00%> (+0.07%)`	⬆️
syft/serde/msgpack/serde.py	`95.94% <100.00%> (+0.02%)`	⬆️
syft/serde/protobuf/proto.py	`100.00% <100.00%> (ø)`
syft/serde/protobuf/serde.py	`63.81% <100.00%> (+1.23%)`	⬆️
test/serde/msgpack/test_msgpack_serde_full.py	`100.00% <100.00%> (ø)`
test/serde/protobuf/test_protobuf_serde_full.py	`100.00% <100.00%> (ø)`
... and 8 more

iamtrask

I am too stretched thin to do a blocking review today.

karlhigley

Not 100% sure that changes are required, but left a few comments for discussion and re-blocking this PR after premature approval.

syft/execution/computation.py

karlhigley · 2020-03-23T13:59:51Z

syft/execution/placeholder.py

@@ -20,6 +21,10 @@ def __init__(self, owner=None, id=None, tags: set = None, description: str = Non
        """
        super().__init__(id=id, owner=owner, tags=tags, description=description)

+        # TODO currently we only use ObjectId on placeholder but if we use them, we
+        # should use them everywhere


ObjectId might be too general as a name for this concept, given the current scope of use. Maybe we could call them PlaceholderIds for now?

I was hoping for a quick extension to all objects because I don't want to have Placeholder as a different object... But I can change the naming for now and change it back when we extend it to other objects.
What do you think?

I think it's still beneficial to know what type the id goes with somehow. Using a separate class allows us to put that information into type annotations, which is kinda nice. On the other hand, a proliferation of tiny id classes might not be great. 🤷‍♂

Could there be a base Id class that defines the fields/behaviors and then a bunch of sub-classes that only exist to capture/enforce id types?

Hum, I don't think Ids will have lots of functionalities so I'm not a fan of having a Id class for each PySyft object...

I think we could kick this can down the road by calling it PlaceholderId for now. Since a one-to-one correspondence is required between Python classes and Protobuf schemas, that would require adding a schema for PlaceholderId though.

It's not perfect and we may just refactor it out later, but it keeps the scope of this PR limited and make it easier to think through the potential implications.

karlhigley · 2020-03-23T14:00:37Z

syft/execution/plan.py

@@ -274,6 +275,20 @@ def replace_with_placeholders(self, obj, arg_ids, result_ids, **kw):
            else:
                return None

+    def replace_ids_with_placeholders(self, obj):
+        """
+        Replace in an object all ids with Placeholders


This only applies to ObjectIds/PlaceholderIds now, right?

Yep! All objects in Plan actions should be placeholders (except constants) and they are all encoded with an ObjectId

karlhigley · 2020-03-23T14:01:55Z

syft/execution/plan.py

+        # Replace non-instanciated placeholders from plan.placeholders by instanciated placeholders
+        # from state.state_placeholders
+        # TODO this definitely isn't the right strategy. Maybe state shouldn't contain
+        # instanciated placeholders but values directly?


I think I'm okay with this strategy for now. Based on Slack conversations, sounds like we should leave State containing instantiated Placeholders for the time being.

karlhigley · 2020-03-23T14:02:36Z

syft/execution/plan.py

+        # Replace non-instanciated placeholders from plan.placeholders by instanciated placeholders
+        # from state.state_placeholders
+        # TODO this definitely isn't the right strategy. Maybe state shouldn't contain
+        # instanciated placeholders but values directly?


I think the approach is okay, but wonder if this makes sense to extract as a method since it happens in multiple places.

syft/execution/state.py

karlhigley · 2020-03-23T14:06:52Z

syft/serde/protobuf/proto.py

@@ -56,6 +57,7 @@
    torch.Size: SizePB,
    # Syft types
    AdditiveSharingTensor: AdditiveSharingTensorPB,
+    ObjectId: IdPB,  # TODO harmonize naming between syft and proto


One way to do this without breaking everything would be to create an ObjectId/PlaceholderId schema on the Protobuf side that contains an Id field.

How bad would it be to rename protobuf's Id to ObjectId/PlaceholderId?

Kinda bad? It's not really intended for either of those specific purposes.

Even if ObjectId was used everywhere?

If we were going to do that, we'd probably want to rename the Python class to Id, because I'm not sure everything that uses an Id is actually using it as the id for an object.

(Still seems like we might want a way to immediately tell what type an id goes with from the class though.)

I don't really get why we would need to know which type the goes to 🤔

Two reasons I can think of:

It's handy for serialization/deserialization.

It prevents people from misusing the ids from one class to load objects of another class (to prevent weird collisions of both accidental and intentional natures.)

Do you think serialization/deserialization could differ between different id classes?

Hum, I agree on this point

Depends on whether they're represented by separate classes, I suppose. If they are, then seems like the serialization could be different.

karlhigley

A few small nits, but this looks really really good. Thanks for taking this experiment and running with it! 😃

syft/execution/computation.py

syft/execution/placeholder_id.py

syft/execution/plan.py

syft/serde/protobuf/proto.py

Jasopaum requested review from karlhigley, iamtrask and LaRiffle March 18, 2020 22:32

Jasopaum requested a review from a team as a code owner March 18, 2020 22:32

LaRiffle reviewed Mar 20, 2020

View reviewed changes

karlhigley reviewed Mar 21, 2020

View reviewed changes

syft/execution/plan.py Show resolved Hide resolved

karlhigley reviewed Mar 21, 2020

View reviewed changes

Jasopaum added 4 commits March 21, 2020 21:38

start experimenting a ObjectId class

49661c1

stop using _smp_placeholders for msgpack

92c7155

use id instead of ObjectId for Plan.placeholders keys

048c7a4

fix some inconsistencies

77806cd

Jasopaum changed the title ~~[WIP] Start experimenting a ObjectId class~~ Start experimenting with an ObjectId class Mar 21, 2020

changes to support protobuf serde

da9ed69

Jasopaum force-pushed the jason/id-class branch from f0a01d9 to da9ed69 Compare March 21, 2020 22:26

karlhigley added plans Type: Refactor 🔨 A complete overhaul of a file, feature, or codebase labels Mar 22, 2020

Jasopaum added 5 commits March 22, 2020 21:32

remove print

38d902f

fix find_placeholders

a77e9fe

remove unused import

2f3472f

remove ObjectStorage properties from Plan

54f8ae5

use child id for placeholder when reading inner state

6ef0a5b

iamtrask approved these changes Mar 23, 2020

View reviewed changes

karlhigley suggested changes Mar 23, 2020

View reviewed changes

Jasopaum added 4 commits March 23, 2020 21:35

factorize replace_non_instanciated_placeholders in static method

09e48bc

start renaming ObjectId -> PlaceholderId

9aec17b

fix protobuf inconsistencies

4a63b77

add possibility to bufferize int | string as computationAction target

80fbddf

karlhigley approved these changes Mar 24, 2020

View reviewed changes

syft/execution/computation.py Show resolved Hide resolved

syft/execution/placeholder_id.py Outdated Show resolved Hide resolved

syft/execution/plan.py Show resolved Hide resolved

syft/serde/protobuf/proto.py Show resolved Hide resolved

karlhigley changed the title ~~Start experimenting with an ObjectId class~~ Use PlaceholderIds in Actions and Plans (instead of full Placeholders) Mar 24, 2020

karlhigley added 4 commits March 24, 2020 18:22

Fix the generated field names for ArgPB to match the schema

c7dd923

Rename serde test helper for PlaceholderId to match the class name

f51851c

Update docstring in syft/execution/placeholder_id.py

864c5be

Update syft-proto dependency to point at new release

cd10eb1

karlhigley merged commit 1097935 into OpenMined:master Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `PlaceholderIds` in `Actions` and `Plans` (instead of full `Placeholders`) #3223

Use `PlaceholderIds` in `Actions` and `Plans` (instead of full `Placeholders`) #3223

Jasopaum commented Mar 18, 2020

LaRiffle Mar 20, 2020

Jasopaum Mar 20, 2020

karlhigley Mar 21, 2020

LaRiffle Mar 20, 2020 •

edited

karlhigley Mar 20, 2020 •

edited

karlhigley Mar 20, 2020

karlhigley Mar 21, 2020

Jasopaum Mar 21, 2020

karlhigley Mar 23, 2020

codecov bot commented Mar 21, 2020 •

edited

iamtrask left a comment

karlhigley left a comment

karlhigley Mar 23, 2020

Jasopaum Mar 23, 2020

karlhigley Mar 23, 2020

Jasopaum Mar 23, 2020

karlhigley Mar 23, 2020

karlhigley Mar 23, 2020

Jasopaum Mar 23, 2020

karlhigley Mar 23, 2020

karlhigley Mar 23, 2020

karlhigley Mar 23, 2020

Jasopaum Mar 23, 2020

karlhigley Mar 23, 2020

Jasopaum Mar 23, 2020

karlhigley Mar 23, 2020

karlhigley Mar 23, 2020

Jasopaum Mar 23, 2020

karlhigley Mar 23, 2020

Jasopaum Mar 23, 2020

karlhigley Mar 23, 2020

karlhigley left a comment

Use PlaceholderIds in Actions and Plans (instead of full Placeholders) #3223

Use PlaceholderIds in Actions and Plans (instead of full Placeholders) #3223

Conversation

Jasopaum commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaRiffle Mar 20, 2020 • edited

Choose a reason for hiding this comment

karlhigley Mar 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 21, 2020 • edited

Codecov Report

iamtrask left a comment

Choose a reason for hiding this comment

karlhigley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlhigley left a comment

Choose a reason for hiding this comment

Use `PlaceholderIds` in `Actions` and `Plans` (instead of full `Placeholders`) #3223

Use `PlaceholderIds` in `Actions` and `Plans` (instead of full `Placeholders`) #3223

LaRiffle Mar 20, 2020 •

edited

karlhigley Mar 20, 2020 •

edited

codecov bot commented Mar 21, 2020 •

edited