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.

Sign up for GitHub

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

Jump to bottom

chore: use protocols for nodes #185

Merged
merged 25 commits into from
Feb 28, 2024
Merged

chore: use protocols for nodes #185

merged 25 commits into from
Feb 28, 2024

Conversation

ThibaultFy
Copy link
Member

@ThibaultFy ThibaultFy commented Nov 10, 2023 •
edited
Loading

Related issue

Companion PR:

closes FL-1290

Summary

Notes

Please check if the PR fulfills these requirements

  • If the feature has an impact on the user experience, the changelog has been updated
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • The commit message follows the conventional commit specification

@ThibaultFy ThibaultFy changed the base branch from main to feat/merge-predict-test November 10, 2023 10:23
@linear Linear
Copy link

linear bot commented Nov 21, 2023

FL-1290 Create Nodes Protocols in SubstraFL

Context

To prepare the arrival of the simulate experiment feature that will introduce SimuNodes, we want to clarify what a Node is by using protocols as define in https://peps.python.org/pep-0544/

Specification

Acceptance criteria

@ThibaultFy ThibaultFy marked this pull request as ready for review November 21, 2023 09:06
@ThibaultFy ThibaultFy requested a review from a team as a code owner November 21, 2023 09:06
SdgJlbl
SdgJlbl approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@SdgJlbl SdgJlbl left a comment

Choose a reason for hiding this comment

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

Thanks ❤️

substrafl/evaluation_strategy.py Outdated
if not all(isinstance(node, TestDataNode) for node in test_data_nodes):
raise TypeError("test_data_nodes must include objects of TestDataNode type")
if not all(isinstance(node, TestDataNodeProtocol) for node in test_data_nodes):
raise TypeError("test_data_nodes must respect the TestDataNodeProtocol")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise TypeError("test_data_nodes must respect the TestDataNodeProtocol")
raise TypeError("test_data_nodes must implement the TestDataNodeProtocol")

tests/test_evaluation_strategy.py
# tests that each next() returns expected True or False
# tests that next called > num_rounds raises StopIteration
n_nodes = 3
num_rounds = 10
# test rounds as frequencies give expected result
# mock the test nodes
test_data_node = Mock(spec=TestDataNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@ThibaultFy ThibaultFy mentioned this pull request Nov 21, 2023
Merged
4 tasks
@ThibaultFy ThibaultFy mentioned this pull request Dec 12, 2023
Closed
@ThibaultFy ThibaultFy force-pushed the feat/merge-predict-test branch 2 times, most recently from 7dc6c25 to 7186314 Compare February 26, 2024 14:42
@guilhem-barthes
Copy link
Contributor

While we are at renaming nodes, what is the reason that made us call TrainDataNode & TestDataNode and not TrainNode and TestNode ?

@ThibaultFy
Copy link
Member Author

While we are at renaming nodes, what is the reason that made us call TrainDataNode & TestDataNode and not TrainNode and TestNode ?

Is to emphasize that these nodes are linked to datasamples. But I guess this is a valid question and both naming have there plus and cons...

@guilhem-barthes
Copy link
Contributor

While we are at renaming nodes, what is the reason that made us call TrainDataNode & TestDataNode and not TrainNode and TestNode ?

Is to emphasize that these nodes are linked to datasamples. But I guess this is a valid question and both naming have there plus and cons...

Could we have Test or Train without data? but indeed that maybe a topic to take further out off this PR

Base automatically changed from feat/merge-predict-test to main February 27, 2024 15:41
ThibaultFy and others added 16 commits February 27, 2024 16:49
@ThibaultFy
chore: changelog
6901aa5 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: return predictions instead of saving them
4c95ece 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: remove predict task and metrics registering from test data node
111b017 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: rebase
e232664 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: sarah's review
743d980 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: add metric functions to strategy
e6e57e9 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
rebase
459db20 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: changelog
f533351 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: protocol
fc5f30f 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: protocol
a215f4c 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: protocol
a3f7b36 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: protocol
5c0fc84 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: changelog
e7c0b50 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: protocol
40afda1 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: adjust protocols
09b8e1a 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: changelog
8afa3bc 
Signed-off-by: ThibaultFy <[email protected]>
ThibaultFy and others added 6 commits February 27, 2024 16:53
@ThibaultFy
chore: changelog
226cdd7 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
rebase
4817724 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: changelog
a0ba9f9 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: changelog
3490eee 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: rebase legacy
be54e97 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: add abstract decorator
33cea1d 
Signed-off-by: ThibaultFy <[email protected]>
@Substra Substra deleted a comment from Owlfred Feb 27, 2024
@ThibaultFy
fix: benchmark
60a9156 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy ThibaultFy force-pushed the chore/node-protocol branch 2 times, most recently from 104e738 to 070f814 Compare February 27, 2024 16:18
@ThibaultFy
fix: doc
59b2957 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
Copy link
Member Author

/e2e --tests substrafl,camelyon,doc

@Owlfred
Copy link

Owlfred commented Feb 27, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

“I’m sorry, Dave. I’m afraid I can’t do that.” ― Hal, 2001: A Space Odyssey

@ThibaultFy
Copy link
Member Author

/e2e --help

@Owlfred
Copy link

Owlfred commented Feb 28, 2024 

Usage: /e2e [options] [help]

/e2e may appear anywhere as long as it is on its own line

Options:
  --refs <value>                          Extra refs (branch or tag) with format REPO=GIT_REF,REPO=GIT_REF.
  Supported repositories: orchestrator, substra-backend, substra-frontend, substra-tools, substrafl, substra, substra-tests, substra-documentation, substra-ci.
  Example: /e2e --refs substra-backend=some_branch,orchestrator=some_tag (default: {})
  --tests-to-run, --tests <tests-to-run>  Comma-separated list of tests to run. Valid options: sdk, substrafl, doc, frontend, mnist, camelyon or NONE. (default: "sdk")
  -h, --help                              display help for command

@ThibaultFy ThibaultFy mentioned this pull request Feb 28, 2024
Merged
@ThibaultFy
chore: renaming
a37b883 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
Copy link
Member Author

/e2e --refs substra-documentation=chore/rename-testnode --tests doc

@Owlfred
Copy link

Owlfred commented Feb 28, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

Oh noes.

@Substra Substra deleted a comment from Owlfred Feb 28, 2024
@ThibaultFy
Copy link
Member Author

/e2e --refs substra-documentation=chore/rename-testnode --tests doc

@Owlfred
Copy link

Owlfred commented Feb 28, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

“Shaken, not stirred.” ― James Bond, Goldfinger

@ThibaultFy ThibaultFy merged commit 6476e19 into main Feb 28, 2024
7 checks passed
@ThibaultFy ThibaultFy deleted the chore/node-protocol branch February 28, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SdgJlbl SdgJlbl SdgJlbl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ThibaultFy @guilhem-barthes @Owlfred @SdgJlbl