[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 5 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?

[fagu]

I think something like sum(user wall time) + (controller cpu time limit) + 1 < (controller wall time) would be better. (The controller cpu time could be much larger than sum(user wall time).)

I'd be happy to try to implement it.

[veluca93]

I think something like sum(user wall time) + (controller cpu time limit) + 1 < (controller wall time) would be better. (The controller cpu time could be much larger than sum(user wall time).)

I'd be happy to try to implement it.

Sure, go ahead!

[fagu]

I've started working on this. https://github.com/fagu/cms/tree/improve-interactive-keeper
It implements the approach we discussed above and fixes some other issues, but I noticed more things that I wanted to discuss before proceeding (see also the todos added in the commit):

1. The keeper starts the solutions with close_fds=False. Doesn't this allow a solution process to use any pipes that are open at the time it is started? I haven't tested it, but that seems like a security issue... (For example, when solution process i is started, the pipes for processes i+1,i+2,... are all still open.)
2. The logging situation is confusing. I think that log messages created by the keeper were previously always discarded. (Or at least I was unable to find them.) I've changed this so that at least if the keeper crashes, its stderr output is shown as an error message in the worker log. Still seems suboptimal...
3. Related: The keeper previously saved error messages in the text and admin_text fields, but I don't see where those would ever be displayed when success=False, except in a "Parsed keeper result" INFO log message on the worker. I've changed this so error messages are saved in a new field "errors" and logged as ERROR by the worker.
4. Other task types keep sandboxes around if the evaluation job failed, allow archiving sandboxes, etc. This can be useful for debugging but is currently not implemented for interactive tasks. (Implementing archiving is a bit annoying because the keeper doesn't have the file_cacher object and the worker doesn't have the sandbox object.)
5. I'm still not a fan of the mismatch between the user_stats and the verdict. For example, the MLE verdict always depends on max(memory usage) but the memory usage shown to the contestant is sum(memory usage) in the concurrent case. For CPU time limits, the situation is even more confusing: in communication tasks, TLE depends on sum(running time) and this is what is shown to the contestant, but in interactive tasks, TLE depends on max(running time). In my opinion, the contestants (and admins preparing a task) would really just care about the value of whatever quantity is actually used to compute the verdict.

[fagu]

1. The keeper starts the solutions with `close_fds=False`. Doesn't this allow a solution process to use any pipes that are open at the time it is started? I haven't tested it, but that seems like a security issue... (For example, when solution process i is started, the pipes for processes i+1,i+2,... are all still open.)

My concern about the pipes for processes i+1,i+2,... being visible to process i was probably unjustified because the ends of those pipes that are still open when the solution processes are started have not been made inheritable. I'd still be a bit worried that for example the python runtime or some library could have opened another inheritable file descriptor. Why do we even need close_fds=False for the solution sandbox? (I didn't immediately notice any problems when changing it to close_fds=True.)