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

Pytest framework implementation with Task Runner GitHub Workflow #1133

Merged
merged 20 commits into from
Nov 12, 2024

Conversation

noopurintel
Copy link
Contributor

@noopurintel noopurintel commented Nov 8, 2024

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:

  1. model names - [torch_cnn_mnist, keras_cnn_mnist] # Other models have bugs associated with them
  2. python version - [3.8, 3.9, 3.10]

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

Copy link
Collaborator

@MasterSkepticista MasterSkepticista left a 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.

@noopurintel noopurintel marked this pull request as draft November 9, 2024 08:44
@noopurintel noopurintel marked this pull request as ready for review November 9, 2024 08:51
@noopurintel
Copy link
Contributor Author

@MasterSkepticista - this name openfl_e2e was decided during one of the discussions.
But, the name end_to_end would be easier to comprehend as you have suggested, let me check and get back on this.

Copy link
Collaborator

@theakshaypant theakshaypant left a 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.)

test-requirements.txt Outdated Show resolved Hide resolved
tests/end_to_end/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@MasterSkepticista MasterSkepticista left a 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.

tests/end_to_end/README.md Show resolved Hide resolved
tests/end_to_end/__init__.py Outdated Show resolved Hide resolved
test-requirements.txt Show resolved Hide resolved
tests/end_to_end/conftest.py Outdated Show resolved Hide resolved
tests/end_to_end/pytest.ini Outdated Show resolved Hide resolved
Copy link
Collaborator

@teoparvanov teoparvanov left a 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.

test-requirements.txt Outdated Show resolved Hide resolved
tests/end_to_end/README.md Outdated Show resolved Hide resolved
tests/end_to_end/models/participants.py Show resolved Hide resolved
tests/end_to_end/utils/federation_helper.py Outdated Show resolved Hide resolved
tests/end_to_end/utils/federation_helper.py Outdated Show resolved Hide resolved
tests/end_to_end/models/participants.py Outdated Show resolved Hide resolved
tests/end_to_end/models/participants.py Outdated Show resolved Hide resolved
@noopurintel
Copy link
Contributor Author

@theakshaypant - Fyi, we are planning to add accuracy and other details after every test run in subsequent PRs.

Copy link
Collaborator

@teoparvanov teoparvanov left a 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...

tests/end_to_end/models/participants.py Outdated Show resolved Hide resolved
tests/end_to_end/models/participants.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@MasterSkepticista MasterSkepticista left a comment

Choose a reason for hiding this comment

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

Minor changes.

.github/workflows/task_runner_e2e.yml Show resolved Hide resolved
tests/end_to_end/README.md Show resolved Hide resolved
.github/workflows/task_runner_e2e.yml Outdated Show resolved Hide resolved
tests/end_to_end/README.md Outdated Show resolved Hide resolved
tests/end_to_end/README.md Outdated Show resolved Hide resolved
tests/end_to_end/utils/logger.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@MasterSkepticista MasterSkepticista left a 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.

tests/end_to_end/utils/logger.py Outdated Show resolved Hide resolved
tests/end_to_end/utils/federation_helper.py Outdated Show resolved Hide resolved
@teoparvanov teoparvanov merged commit 0e16aff into securefederatedai:develop Nov 12, 2024
28 checks passed
@noopurintel noopurintel deleted the pytest-for-openfl branch December 12, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants