Skip to content

feat: substratools methods as generic method#59

Merged
ThibaultFy merged 26 commits into
mainfrom
chore/refacto-remote-method
Jan 16, 2023
Merged

feat: substratools methods as generic method#59
ThibaultFy merged 26 commits into
mainfrom
chore/refacto-remote-method

Conversation

@ThibaultFy
Copy link
Copy Markdown
Member

@ThibaultFy ThibaultFy commented Dec 5, 2022 •
edited by guilhem-barthes
Loading

Signed-off-by: ThibaultFy 50656860+ThibaultFy@users.noreply.github.com

Companion PR

Substra/substra-tools#74

Summary

Refacto of substratools_methods.py to add flexibility and simplify the code.
The functions executed by substratools does not have name constraint anymore.

Notes

Asana card: https://app.asana.com/0/1202209633046022/1203200116896589/f

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
Copy link
Copy Markdown
Member Author

ThibaultFy commented Dec 6, 2022

/e2e -h

@Owlfred
Copy link
Copy Markdown

Owlfred commented Dec 6, 2022 

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: substra-hlf-k8s, orchestrator, substra-backend, substra-frontend, substra-tools, substrafl, substra, substra-tests, 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,frontend or NONE. (default: "sdk")
  --benchmarks-to-run, --benchmarks <benchmarks-to-run>  Comma-separated list of workflows tests to run. Valid options: mnist, camelyon or NONE. (default: "NONE")
  --orchestrator-mode, --mode <orchestrator-mode>        Comma-separated list of orchestrator modes to run tests for. Valid options: standalone,distributed (default: "standalone,distributed")
  -h, --help                                             display help for command

@ThibaultFy
Copy link
Copy Markdown
Member Author

ThibaultFy commented Dec 6, 2022

/e2e --refs substra-tools=feat/user-define-function-name,substrafl=chore/refacto-remote-method --tests substrafl --benchmarks mnist,camelyon --mode standalone

@Owlfred
Copy link
Copy Markdown

Owlfred commented Dec 6, 2022 •
edited
Loading

End to end tests: ✔️ SUCCESS

Woohoo! 🎉

@ThibaultFy ThibaultFy mentioned this pull request Dec 6, 2022
Merged
@ThibaultFy ThibaultFy changed the title remove remote data method feat: substratools methods are now generic Dec 6, 2022
@ThibaultFy ThibaultFy changed the title feat: substratools methods are now generic feat: substratools methods as generic method Dec 6, 2022
@ThibaultFy ThibaultFy marked this pull request as ready for review December 6, 2022 10:32
@ThibaultFy
Copy link
Copy Markdown
Member Author

ThibaultFy commented Dec 16, 2022

/e2e --refs substra-tools=feat/user-define-function-name,substrafl=chore/refacto-remote-method --tests substrafl --benchmarks mnist,camelyon --mode standalone

@Owlfred
Copy link
Copy Markdown

Owlfred commented Dec 16, 2022 •
edited
Loading

End to end tests: ✔️ SUCCESS

You rock!

guilhem-barthes
guilhem-barthes approved these changes Jan 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@guilhem-barthes guilhem-barthes 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 work!

Comment thread substrafl/remote/register/register.py

class Metric:
def score(self, datasamples, predictions_path):
def score(self, datasamples, predictions_path, _skip=True):
Copy link
Copy Markdown
Contributor

@guilhem-barthes guilhem-barthes Jan 10, 2023

Choose a reason for hiding this comment

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

Compare with the use of **kwargs

guilhem-barthes
guilhem-barthes reviewed Jan 10, 2023
View reviewed changes
Comment thread substrafl/remote/substratools_methods.py

if InputIdentifiers.shared in inputs:
loaded_inputs["shared_state"] = (
self.load_model(inputs[InputIdentifiers.shared]) if inputs[InputIdentifiers.shared] else None
Copy link
Copy Markdown
Contributor

@guilhem-barthes guilhem-barthes Jan 10, 2023

Choose a reason for hiding this comment

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

I am not sure about readability

@ThibaultFy
remove remote data method
8ddd13d 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
remove remote data method
cb66478 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
remove remote data method
bbf4df0 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
remove remote data method
eacb868 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
remove remote data method
1433dc4 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
remove remote data method
ada1783 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
fix tests
0aaf37d 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
256cb92 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
simplify instance save and load
18b565f 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
changelog
a3d9448 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
changelog
ccb96a6 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
changelog
60c09c7 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
changelog
874aaf7 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
remove comment
87b173a 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
f782b2b 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
comment
cfc833b 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
c843e29 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
lint
68e024e 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
diff
3e6cf59 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
c57d497 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
e63ea9d 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
0df9856 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
332384d 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
docstring
9110071 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
try expect for _skip
2478991 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
_skip in metrics
2ada17f 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy ThibaultFy force-pushed the chore/refacto-remote-method branch from 3fe3e36 to 2ada17f Compare January 16, 2023 10:54
@ThibaultFy ThibaultFy merged commit 7fdd604 into main Jan 16, 2023
@ThibaultFy ThibaultFy deleted the chore/refacto-remote-method branch January 16, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guilhem-barthes guilhem-barthes guilhem-barthes approved these changes

@Esadruhn Esadruhn Awaiting requested review from Esadruhn

@Fabien-GELUS Fabien-GELUS Awaiting requested review from Fabien-GELUS

@louishulot louishulot Awaiting requested review from louishulot

@oleobal oleobal Awaiting requested review from oleobal

@Milouu Milouu Awaiting requested review from Milouu

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 @guilhem-barthes