Batch evaluation intervals into a single request and a single evaluation process #554
Conversation
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
==========================================
- Coverage 82.98% 82.63% -0.35%
==========================================
Files 221 218 -3
Lines 10342 10235 -107
==========================================
- Hits 8582 8458 -124
- Misses 1760 1777 +17 ☔ View full report in Codecov by Sentry. |
XianzheMa
changed the title
|
Thank you so much! I will go through this tomorrow and also ping @robinholzi to review this since it affects his evaluation stuff |
There was a problem hiding this comment.
Thanks so much @XianzheMa for putting some thought into the evaluator side of the evaluation logic! I can surely adjust the batching in supervisor to comply with the new interface! Just a couple of questions before we finalise the grpc interface changes
|
Sorry, now I understand your comment. This changes the server interface to support batching but the client does not yet send batched requests :D Now I get it, sorry |
There was a problem hiding this comment.
Thank you so much Xianzhe! I hope the changes are not too much work. We can chat with Robin about the status queue thingy, but I think we need a way of e.g. transferring exceptions to the supervisor because we should have those in the logs (I've used them before for debugging, helpful to have it in a single place). I think there are some minor issues in the interface where we need to adjust the evaluation started request, but the overall flow looks nice!
|
The integration test definitely passed on our server. Will check tomorrow why |
I think there probably is some flakyness somewhere. Sorry, I am too tired to review this properly tonight, I will do it tomorrow morning as well. But I don't think it's because of release/debug/... mode, I think this is rather some concurrency issue somewhere |
There was a problem hiding this comment.
Thank you! My only big comment besides nitpicky stuff is on the interval_idx. I think we should switch to using the interval boundaries cause looking at the supervisor, it would simplify the logic for the follow up PR if we directly have the interval bounds instead of some idx. I did not immediately find the reason for the integrationtest failure, though :(
modyn/supervisor/internal/pipeline_executor/evaluation_executor.py
Outdated
Show resolved
Hide resolved
modyn/supervisor/internal/pipeline_executor/evaluation_executor.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Maximilian Böther <2116466+MaxiBoether@users.noreply.github.com>
everything should be fixed now. Let's see |
There was a problem hiding this comment.
LGTM! Let's wait for @robinholzi's review (and clarification regarding the docstring) and the integration tests, but I do not have further comments. Thank you so much!
|
The last commit only involves a comment, and before the commit the CI passed. So I just merged without waiting more. |
Per the title, this PR enables batching evaluations on many intervals into a single evaluation request. This resolves #536. The integration test is adjusted to cover this new functionality.
How to review
The best way to review is to first take a look at modyn/protos/evaluator.proto to see on the API level what is changed.
Miscellaneous
After this PR, batching is only enabled on server side, on client side still one interval is passed at a time, as I am confused on the current way to generate
EvalRequest.After this PR, @robinholzi could you make a PR to collect the intervals associated with the same
id_modelanddataset_id, and pack them in one evaluation round? I think just looking at the function_single_batched_evaluationin this file modyn/supervisor/internal/pipeline_executor/evaluation_executor.py should be enough to understand the change / how to make that PR. It should be a very easy and straightforward PR. I am just confused with the data models there. Thank you so much!