domjudge icon indicating copy to clipboard operation
domjudge copied to clipboard

Send less event updates for judgements in parallel mode.

Open nickygerritsen opened this issue 1 year ago • 10 comments

We used to send event updates for all testcases that came in after we already determined the final verdict for a problem. Now we check if we already have a previous end time for the judgement, and then we don't do this anymore.

Note that race conditions can still make it such that we send multiple update events, where two judgehosts know the result at the same time. However, this happens way less often.

Do we still want to update the end time? We currently do but then never send an update event anymore, even though the end time changed. So is this actually valid? Or do we not want to merge this and accept that we send updates (in Luxor we had submissions with 47 updates, while only 1 is really needed)? Or maybe in lazy mode we do not take into account the run time of any testcase that came after the first wrong one?

Found by @johnbrvc.

nickygerritsen avatar May 01 '24 14:05 nickygerritsen

Does it hurt?

meisterT avatar May 01 '24 14:05 meisterT

@johnbrvc does it hurt for you, more than that you run a codepath to check if the judgement changed?

I wonder if this is a CLICS discussion as well, can a judgement change after an end_time has been set?

nickygerritsen avatar May 01 '24 14:05 nickygerritsen

IIRC, we did this intentionally to make the event feed and endpoints match.

Cc @eldering

meisterT avatar May 01 '24 14:05 meisterT

IIRC, we did this intentionally to make the event feed and endpoints match.

Cc @eldering

Yeah this is what I'm thinking as well, hence my last sentence.

nickygerritsen avatar May 01 '24 14:05 nickygerritsen

It doesn't hurt per se, but it is quite confusing, and pollutes the logs (in addition, it bloats the event feed). Which judgement for a specific judgement_id are we supposed to believe? the first? the 20th? the last? It seems that once a judgement_type_id is set (WA/AC, etc), then that's it, unless the judge_type_id changes (which does not happen very often, if at all - manual intervention). I'm not sure what use anything after that is? The "runs" entries have each test case's disposition and execution time, so why does the final judgement record update?

On Wed, May 1, 2024 at 10:28 AM Nicky Gerritsen @.***> wrote:

@johnbrvc https://github.com/johnbrvc does it hurt for you, more than that you run a codepath to check if the judgement changed?

I wonder if this is a CLICS discussion as well, can a judgement change after an end_time has been set?

— Reply to this email directly, view it on GitHub https://github.com/DOMjudge/domjudge/pull/2523#issuecomment-2088545665, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLFGEYP74MV4PAGLYBFXYLZAD3YBAVCNFSM6AAAAABHCBD5O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYGU2DKNRWGU . You are receiving this because you were mentioned.Message ID: @.***>

johnbrvc avatar May 01 '24 16:05 johnbrvc

It doesn't hurt per se, but it is quite confusing, and pollutes the logs

You could decide to only log if there is any contradicting information in the update.

Which judgement for a specific judgement_id are we supposed to believe? the first? the 20th? the last?

The last as we are sending updated information.

I think this is in line with https://ccs-specs.icpc.io/draft/contest_api#general-requirements which specifically allows duplicate events and "when an object changes one or more times a notification will be sent" as well "the latest notification sent for any object is the correct and current state of that object."

I'm not sure what use anything after that is? The "runs" entries have each test case's disposition and execution time, so why does the final judgement record update?

Technically something was judged after the previously recorded endtime, so we update it to reflect that. With that it is exposed in the specific API endpoint. To not have conflicting information in the feed and the API, we also send it in the feed.

meisterT avatar May 01 '24 17:05 meisterT

I'm not sure what use anything after that is? The "runs" entries have each test case's disposition and execution time, so why does the final judgement record update?

Technically something was judged after the previously recorded endtime, so we update it to reflect that. With that it is exposed in the specific API endpoint. To not have conflicting information in the feed and the API, we also send it in the feed.

I guess this is my question. How can you have something that was already judged (and given a judgment (WA/AC/TLE, etc.), be judged again, the same way? It seems (IMHO) that the judgment record should be sent when the judging is complete, or, don't include the judgement_type_id. This could be my philosophical or intuitive understanding of what a "judgement" is, that is, is it "right" or "wrong"; I don't need to now it is "wrong" 43 times, even though technically, wrong is wrong, even if it is wrong 43 times.

It doesn't matter to me (or PC2, for that matter). I was just pointing it out is all...

johnbrvc avatar May 01 '24 20:05 johnbrvc

I guess this is my question. How can you have something that was already judged (and given a judgment (WA/AC/TLE, etc.), be judged again, the same way?

That's just the nature of the first incorrect test case (in order) determines the final verdict. When that is known, we let teams and downstream clients know although there is still judging going on. But these remaining test cases cannot influence the final verdict.

meisterT avatar May 04 '24 11:05 meisterT

I guess this is my question. How can you have something that was already judged (and given a judgment (WA/AC/TLE, etc.), be judged again, the same way?

That's just the nature of the first incorrect test case (in order) determines the final verdict. When that is known, we let teams and downstream clients know although there is still judging going on. But these remaining test cases cannot influence the final verdict.

I agree with @johnbrvc that it doesn't really make sense to update the judgement after it has a verdict. Even though it is still running on our side, that really should/does not change the judgement anymore.

I think what we should do, is simply still send the runs, not the judgements anymore. Since these don't affect the judgement, there's no need to update our judgement end time (either internally or externally), so we don't have to send out an event for that. Indeed, otherwise we should have, since the event feed should match the contents of the API endpoints.

eldering avatar May 12 '24 11:05 eldering

Note that we also update the max run time to be the max of all runs. This then also needs to change to only take the max of runs that added to the verdict, which might be hard. If we run in parallel, we might already have run some later test cases which would then be in the max run time during running but not anymore after it is done.

nickygerritsen avatar May 12 '24 12:05 nickygerritsen

After discussion IRL, we agreed to set the judging endtime only once, together with the verdict.

eldering avatar Nov 22 '24 08:11 eldering