eth-easl / modyn

Modyn is a research-platform for training ML models on growing datasets.
MIT License
22 stars 3 forks source link

Move `after_pipeline|training_evaluation_workers` to pipeline config #511

Closed XianzheMa closed 1 month ago

XianzheMa commented 1 month ago

Per the title, this solves https://github.com/eth-easl/modyn/issues/508.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.18%. Comparing base (6d621ad) to head (6c0e9c2).

Files Patch % Lines
.../internal/pipeline_executor/evaluation_executor.py 33.33% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #511 +/- ## ========================================== - Coverage 82.21% 82.18% -0.04% ========================================== Files 214 214 Lines 9956 9961 +5 ========================================== + Hits 8185 8186 +1 - Misses 1771 1775 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 1 month ago

Line Coverage: -% ( % to main) Branch Coverage: -% ( % to main)

XianzheMa commented 1 month ago

I need to resolve the comment to merge the PR, so I copy-paste my question here: We somehow don't have a unit test file for the quite large evaluation_executor.py. https://github.com/eth-easl/modyn/pull/490

@robinholzi Is there a reason? If the missing unit test is not intentional please add one!

I will for the moment also make patch action informational. As it does not make sense for me to create a huge unit test only to cover these trivial changes.

robinholzi commented 1 month ago

@XianzheMa I guess that depends on what you define as a unit. We are testing the logic in evaluation_executor with the logic from test_pipeline_supervisor just like we did before the introduction of evaluation_executor. As there are some uncovered parts though, I will add dedicated tests for them! Thanks for the pointer!