-
Notifications
You must be signed in to change notification settings - Fork 210
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
Pytest framework implementation with Task Runner GitHub Workflow #1133
Pytest framework implementation with Task Runner GitHub Workflow #1133
Conversation
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest naming it as tests/end_to_end
, and instances of e2e
as end_to_end
.
Signed-off-by: noopur <[email protected]>
@MasterSkepticista - this name |
Signed-off-by: noopur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Just a few general questions.
Also how do we feel about having the model accuracy to be logged along with test success (or failure) in the workflow? (Can be easily picked up from the aggregator logs.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed the actual tests yet.
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thank you @noopurintel ! Very nice abstractions around ModelOwner
, Collaborator
and Aggregator
, which contribute to a great experience both reading and writing the actual tests.
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
@theakshaypant - Fyi, we are planning to add accuracy and other details after every test run in subsequent PRs. |
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A final comment on method naming...
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes.
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with reservations - I am personally not confident about the scope of what this test framework will achieve for "Taskrunner" interface, and how it helps beyond existing tests. I may be wrong about this, so I agree with merging this PR.
I do see the value of this framework for Workflow API though, to be clear.
Thanks for the additions @noopurintel and @payalcha.
Signed-off-by: noopur <[email protected]>
Signed-off-by: noopur <[email protected]>
This PR aims to add the
pytest
framework based test cases in OpenFL repository.Changes done:
Added end_to_end folder inside openfl/tests. Please go through
README.md
file for complete structure and related details.Added
task_runner_e2e.yml
workflow file inside .github/workflows - it will run various models for Task Runner API approach.For now, the workflow is expected to run every night with below matrix combination:
Successful test run (11th Nov 07:20 pm IST) - https://github.com/securefederatedai/openfl/actions/runs/11779770937
Final (12th Nov 04:35 pm IST) - https://github.com/securefederatedai/openfl/actions/runs/11795815852