[jlohse-mpi-sws]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.82%. Comparing base (3277ca5) to head (98560c7).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
cms/grading/tasktypes/interactive_keeper.py	0.00%	13 Missing ⚠️
cmscontrib/loaders/italy_yaml.py	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1733      +/-   ##
==========================================
+ Coverage   56.72%   56.82%   +0.10%     
==========================================
  Files         344      344              
  Lines       28314    28374      +60     
==========================================
+ Hits        16060    16123      +63     
+ Misses      12254    12251       -3     

Flag	Coverage Δ
functionaltests	0.00% <0.00%> (ø)
unittests	56.82% <0.00%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[veluca93]

Looks like some test was relying on the previous behaviour, could you check?

[jlohse-mpi-sws]

Yes sorry. The issue was that my change prioritized controller failures over contestant failures, but it has to be the other way around in case the controller fails because of waiting for the contestant.

[fagu]

If the controller fails because of waiting for the contestant, doesn't that indicate a bug in the controller or a too-low controller wall time limit? So I would expect "controller failed" in this case.
Also, it seems reasonable to me that the submission is allowed to crash if the controller failed. (Or are we expecting the submission stubs to detect incomplete inputs in all cases?)

[fagu]

Somewhat off-topic: Are the sandboxes intentionally not cleaned up in the "invalid command from controller" branch?
https://github.com/jlohse-mpi-sws/cms/blob/75f190ed042b1b5c92a13cab6918791c193d2ee0/cms/grading/tasktypes/interactive_keeper.py#L183-L191

[jlohse-mpi-sws]

If the controller fails because of waiting for the contestant, doesn't that indicate a bug in the controller or a too-low controller wall time limit? So I would expect "controller failed" in this case. Also, it seems reasonable to me that the submission is allowed to crash if the controller failed. (Or are we expecting the submission stubs to detect incomplete inputs in all cases?)

In the current implementation there is only one wall time limit (controller_wall_limit) and the contestant gets the same wall limit. Therefore, if the contestant sleeps, the controller (who is started first) will likely time out first.

People have been saying it is too hard to reliably find out in general whether the controller died because the contestant took too long, it guess this is why it is implemented like this @veluca93 ?

[fagu]

Thanks for your explanation. I hadn't realized that the solution and the controller have the same wall time limit.

So with the current implementation, I suppose a solution can force (success=False,"Controller Failed") by sleeping for a little less than the wall time limit and then quitting. (Not sure how problematic this is if done intentionally, but couldn't something to this effect also happen accidentally and then the participant would never learn the result of their submission?)

This implementation also means that the stub has to check every input from the controller for completeness before handing it to the contestant's code as the controller could have been killed while writing, right?

[veluca93]

People have been saying it is too hard to reliably find out in general whether the controller died because the contestant took too long, it guess this is why it is implemented like this @veluca93 ?

Yes, hence we should prioritize contestant failures over controller failures.

Thanks for your explanation. I hadn't realized that the solution and the controller have the same wall time limit.

So with the current implementation, I suppose a solution can force (success=False,"Controller Failed") by sleeping for a little less than the wall time limit and then quitting. (Not sure how problematic this is if done intentionally, but couldn't something to this effect also happen accidentally and then the participant would never learn the result of their submission?)

I don't think this is true (time limits are somewhat extended), but I am happy to be proven wrong with an adversarial example.

This implementation also means that the stub has to check every input from the controller for completeness before handing it to the contestant's code as the controller could have been killed while writing, right?

I don't understand this. If the controller fails, there is clearly a problem with the task.

[fagu]

Thanks for your explanation. I hadn't realized that the solution and the controller have the same wall time limit.
So with the current implementation, I suppose a solution can force (success=False,"Controller Failed") by sleeping for a little less than the wall time limit and then quitting. (Not sure how problematic this is if done intentionally, but couldn't something to this effect also happen accidentally and then the participant would never learn the result of their submission?)

I don't think this is true (time limits are somewhat extended), but I am happy to be proven wrong with an adversarial example.

I'm not sure what you mean by time limits being extended or how that would help. (The controller and solution have exactly the same wall time limit. There is an extra_time setting in isolate, but that only applies to the cpu time, not wall time.)

Here's a concrete example (encountered by Lukas Michel):
Controller:

#include <cstdio>
#include <ctime>

int main() {
	// Wait for 0.5 seconds before starting the solution.
	// (In practice, the controller would do other things, like read input or run several solutions in a row.)
	timespec t{0,500000000};
	nanosleep(&t, nullptr);
	
	printf("START_SOLUTION\n");
	fflush(stdout);
	int fdin, fdout;
	if (scanf("%d %d", &fdin, &fdout) != 2)
		return 1;
	FILE* to_solution = fdopen(fdin, "w");
	FILE* from_solution = fdopen(fdout, "r");
	
	int a;
	if (fscanf(from_solution, "%d", &a) == 1 && a == 42) {
		fprintf(stderr, "SCORE: 1\n");
		fprintf(stderr, "USER_MESSAGE: translate:success\n");
	} else {
		fprintf(stderr, "SCORE: 0\n");
		fprintf(stderr, "USER_MESSAGE: translate:wrong\n");
	}
	
	fclose(to_solution);
	fclose(from_solution);
	return 0;
}

Correct solution:

#include <cstdio>

int main() {
	printf("42\n");
	return 0;
}

Wrong solution:

#include <cstdio>

int main() {
	int a;
	scanf("%d", &a);
	return 0;
}

Empty input file.

The solution and controller wait for input until the controller is killed due to exceeding its wall limit. Then, the solution terminates gracefully because its stdin pipe reads EOF. This is reported as a controller failure.

[fagu]

This implementation also means that the stub has to check every input from the controller for completeness before handing it to the contestant's code as the controller could have been killed while writing, right?

I don't understand this. If the controller fails, there is clearly a problem with the task.

I think that the controller failing should indicate a problem with the task, but that is not the case with the current implementation. See above for an example. Also, if the controller couldn't fail, why would we have to prioritize contestant failures over controller failures? I'm probably misunderstanding what you wrote...

My suggestion would be that we have a separate controller wall time limit that is sufficiently large compared to the solution wall limit that no matter what the solution does, the controller will never run out of time. We would then report any controller failure as a failed evaluation (success=False). This would bring the situation to the attention of the contest administrators. (A failed controller would indeed indicate a problem with the task that should be fixed!)

[veluca93]

@fagu I had a LLM code up the following:

1. a solution has a default wall time limit of 2*time limit + 1s.
2. this is unless this wall time limit would be within 1s of the controller's wall time limit. In that case, it gets cut short.
3. the importer warns if controller_wall_limit <= 2*time_limit + 2s

WDYT?

[fagu]

Thanks. That's an interesting idea. I haven't really thought it through yet, so just a few quick comments for now:

1. If the number of concurrent solution processes is larger than the number of cores or if the controller is slow, a wall_limit of 2*time_limit+1 might be insufficient.
2. I would want to double-check that isolate can cope with the somewhat strange case wall_limit < time_limit.
3. This approach makes the relationship between the verdict "wall limit exceeded" and the reported wall time rather unclear. (The latter is influenced by the "concurrent" setting, the former is not.)
4. After the solution finishes, the controller will potentially only have a hardcoded 1 second left. It's not clear whether that is always enough. Maybe we should add controller_time_limit to the wall_limits?

What about the following simpler approach (very close to the status quo) also inspired by your new idea:
Use the same wall_limit for the solution and the controller and compute the verdict as follows:

• If the controller exceeds its wall limit, report "wall limit exceeded" (not controller failed!), no matter what happened to the solution (even if the solution crashed).
• Otherwise, if the controller failed, gave an invalid command, or didn't report a score, report "controller failed" (success=False).
• Otherwise, if the solution failed, report that.
• Otherwise, forward the score reported by the controller.
• In all cases, the reported wall time should be the maximum of the controller wall time and the wall times of the solutions. (Get rid of the "concurrent" setting.)

The reasoning is as follows: We are always unable to distinguish whether a deadlock is the controller's fault or the solution's fault, so (assuming the controller to be correct) we blame the solution for any exceeded wall limit. If the controller fails to due wall limit, that might cause the solution to crash due to receiving incomplete input from the controller or due to SIGPIPE (and the correct verdict would be "wall limit exceeded"). In the other direction, a crashed solution cannot cause the controller to exceed its wall limit.

What do you think about this approach? (I would also like to think about it more carefully myself. It feels a bit scary to allow the solution to crash due to the controller exceeding the wall time limit, but I don't see a problem with it right now.)

[veluca93]

I think this is reasonable, although I would propose two changes:

1. let's not change concurrent, at least for now
2. if sum(user wall time) * 2 + 1 < (controller wall time) it is probably enough indication of a bug in the controller, and we should mark that as controller failed. I am not sure I can think of cases in which this would fail, and it would at least help catch the most blatant controller bugs.

WDYT?