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

feat!: merge predict and test tasks #177

Merged
merged 18 commits into from
Feb 27, 2024
Merged

feat!: merge predict and test tasks #177

merged 18 commits into from
Feb 27, 2024

Conversation

ThibaultFy
Copy link
Member

@ThibaultFy ThibaultFy commented Oct 11, 2023 •
edited
Loading

Related issue

closes FL-1205

Summary

  • The predict task does not exist anymore. The evaluation of a model is done in a single task.
  • Strategy implement an evaluate method, with the @remote_data decorator, to compute the evaluation of the model. The evaluate method is the same for all strategies.
  • BREAKING: the perform_predict method of Strategy changed in favor of perform_evaluation that calls the new evaluate method.
  • BREAKING: metric_functions are now passed to the Strategy instead of the TestDataNode.
  • BREAKING: the predict method of Algo has no @remote_data decorator anymore. It signatures does not take prediction_path anymore, and the predictions are return by the method.

Can be reviewed commit by commit.

Companions

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

@linear
Copy link

linear bot commented Oct 11, 2023

FL-1205 Merge predict & evaluate in TestDataNodes

Context

Specification

Acceptance criteria

@ThibaultFy
Copy link
Member Author

/e2e --tests substrafl

@Owlfred
Copy link

Owlfred commented Oct 12, 2023 •
edited
Loading

End to end tests: ✔️ SUCCESS

Congratulations!

@ThibaultFy
Copy link
Member Author

/e2e --tests sustrafl,camelyon,doc --refs substra-documentation=feat/merge-predict-test

@Owlfred
Copy link

Owlfred commented Oct 13, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

What a surprise... 🙄

@ThibaultFy
Copy link
Member Author

/e2e --tests substrafl

@Owlfred
Copy link

Owlfred commented Oct 13, 2023 •
edited
Loading

End to end tests: ✔️ SUCCESS

@ThibaultFy
Copy link
Member Author

/e2e --tests camelyon

@Owlfred
Copy link

Owlfred commented Oct 13, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

I'm sorry.

@ThibaultFy ThibaultFy mentioned this pull request Oct 13, 2023
Merged
@ThibaultFy
Copy link
Member Author

ThibaultFy commented Oct 13, 2023 •
edited
Loading

/e2e --tests camelyon

EDIT: Camelyon broken on main for now

@Owlfred
Copy link

Owlfred commented Oct 13, 2023 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

“Success is not final; failure is not fatal: It is the courage to continue that counts.” ―- Winston S. Churchill

@ThibaultFy ThibaultFy mentioned this pull request Oct 16, 2023
Merged
@ThibaultFy ThibaultFy marked this pull request as ready for review October 16, 2023 14:19
@ThibaultFy ThibaultFy requested a review from a team as a code owner October 16, 2023 14:19
SdgJlbl
SdgJlbl reviewed Oct 18, 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 for the PR :)

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
substrafl/algorithms/algo.py Show resolved Hide resolved
substrafl/algorithms/pytorch/torch_base_algo.py Show resolved Hide resolved
substrafl/algorithms/pytorch/torch_base_algo.py Outdated Show resolved Hide resolved
substrafl/strategies/strategy.py Outdated Show resolved Hide resolved
tests/algorithms/pytorch/test_fed_pca_algo.py Outdated Show resolved Hide resolved
tests/algorithms/pytorch/test_newton_raphson.py Show resolved Hide resolved
substrafl/strategies/fed_pca.py Show resolved Hide resolved
substrafl/strategies/newton_raphson.py
Comment on lines +60 to +63
metric_functions (Optional[Union[Dict[str, Callable], List[Callable], Callable]]):
list of Functions that implement the different metrics. If a Dict is given, the keys will be used to
register the result of the associated function. If a Function or a List is given, function.__name__
will be used to store the result.
Copy link
Contributor

Choose a reason for hiding this comment

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

Food for thought: seeing all these docstrings duplicated, I'm thinking about way to generate docstrings dynamically to avoid having to keep everything updated everywhere all the time (using __doc__)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good food... That would be great to find smthg like this !

@ThibaultFy ThibaultFy changed the title fix: merge predict and test tasks feat!: merge predict and test tasks Oct 20, 2023
@ThibaultFy
Copy link
Member Author

/e2e --tests substrafl,camelyon,doc --refs substra-documentation=feat/merge-predict-test

@Owlfred
Copy link

Owlfred commented Oct 20, 2023 •
edited
Loading

End to end tests: ✔️ SUCCESS

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

@Substra Substra deleted a comment from Owlfred Oct 20, 2023
@Substra Substra deleted a comment from Owlfred Oct 20, 2023
SdgJlbl
SdgJlbl approved these changes Oct 24, 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.

One remaining unaddressed comment, all good else 👍
Thanks

@ThibaultFy
Copy link
Member Author

/e2e --tests substrafl,camelyon,doc --refs substra-documentation=feat/merge-predict-test

@Owlfred
Copy link

Owlfred commented Feb 22, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

ThibaultFy and others added 18 commits February 26, 2024 15:37
@ThibaultFy
chore: changelog
2ceb535 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: return predictions instead of saving them
08676c1 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: pydantic v2 artefact
261a2f2 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: remove predict task and metrics registering from test data node
c831967 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: update benchmark
fcc8386 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
rebase
a79ee49 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
test: move metric function test to strategy and add evaluate test
b1ee596 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
test: update tests
26bf60a 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: rebase
dd16930 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: sarah's review
176fbd1 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: sarah's review
2934852 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: typo
f92e4c4 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: pydantic add @classmethod
f0d9fb1 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
fix: test_linear_nodes_pca docstring
90f78c2 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: typo in docstrings
d5c5597 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: add metric functions to strategy
bf52260 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
rebase
ed5be2d 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
chore: changelog
7186314 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests substrafl,camelyon,doc --refs substra-documentation=feat/merge-predict-test

@Owlfred
Copy link

Owlfred commented Feb 26, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

“It’s alive! It’s alive!” ― Henry Frankenstein, Frankenstein

@ThibaultFy ThibaultFy merged commit ff7f043 into main Feb 27, 2024
7 checks passed
@ThibaultFy ThibaultFy deleted the feat/merge-predict-test branch February 27, 2024 15:41
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.
3 participants
@ThibaultFy @Owlfred @SdgJlbl