Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract a Role class from Plans #3201

Merged
merged 19 commits into from
Mar 31, 2020
Merged

Conversation

Jasopaum
Copy link
Member

No description provided.

@karlhigley karlhigley added plans Type: Refactor 🔨 A complete overhaul of a file, feature, or codebase labels Mar 17, 2020
@Jasopaum Jasopaum changed the title [WIP] Start extracting a Role class from Plans Extracting a Role class from Plans Mar 28, 2020
@Jasopaum Jasopaum marked this pull request as ready for review March 28, 2020 16:02
@Jasopaum Jasopaum requested a review from a team as a code owner March 28, 2020 16:02
@codecov
Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #3201 into master will increase coverage by 0.06%.
The diff coverage is 97.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3201      +/-   ##
==========================================
+ Coverage   94.60%   94.67%   +0.06%     
==========================================
  Files         149      148       -1     
  Lines       15917    16023     +106     
==========================================
+ Hits        15058    15169     +111     
+ Misses        859      854       -5     
Impacted Files Coverage Δ
...orks/torch/tensors/interpreters/additive_shared.py 92.20% <75.00%> (+0.03%) ⬆️
syft/execution/state.py 90.26% <80.00%> (-0.97%) ⬇️
syft/execution/placeholder.py 91.42% <93.33%> (+0.51%) ⬆️
syft/execution/plan.py 91.71% <96.87%> (-1.09%) ⬇️
syft/execution/role.py 98.84% <98.84%> (ø)
syft/generic/object.py 91.25% <100.00%> (ø)
syft/serde/msgpack/serde.py 95.97% <100.00%> (+0.02%) ⬆️
syft/serde/protobuf/proto.py 100.00% <100.00%> (ø)
syft/serde/protobuf/serde.py 64.05% <100.00%> (+0.23%) ⬆️
test/execution/test_plan.py 100.00% <100.00%> (ø)
... and 21 more

@Jasopaum Jasopaum changed the title Extracting a Role class from Plans Extract a Role class from Plans Mar 29, 2020
# TODO isn't it weird that state placeholders are both in state and plan?
self.placeholders[tensor.id] = placeholder

def fecth_placeholders_from_ids(self, obj):
Copy link
Member

Choose a reason for hiding this comment

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

typo fetch_placeholders_from_ids

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Just fixed that

Copy link
Contributor

@karlhigley karlhigley left a comment

Choose a reason for hiding this comment

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

❤️

@@ -17,7 +17,7 @@ def __init__(
self,
id: int = None,
owner: "sy.workers.AbstractWorker" = None,
tags: List[str] = None,
tags: Set[str] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

# out += line

# out += " return "
# out += ", ".join(f"_{extract_tag(p)}" for p in self.find_placeholders("output"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this now? 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to because I don't find it very useful to print a list of actions like that... What do you think?

@karlhigley karlhigley merged commit c2544d1 into OpenMined:master Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Refactor 🔨 A complete overhaul of a file, feature, or codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants