Better grader error display

[Matistjati]

This PR does 3 things:

• Rewrites some awkward open() code to be crash-safe
• Prints stderr in case the grader crashes
• Removes support for multiple graders

Sample outputs:

Differing grader outputs:

ERROR Different graders gave different results:
ERROR grader.py: AC 0.0
ERROR grader2.py: AC 1.0

Grader stderr (bizarre error, I know):

Checking submissions
ERROR Judge error: grader.py (Python 3 (w/PyPy3)) crashed
ERROR Grader stderr:
==134096==ERROR: AddressSanitizer failed to allocate 0xdfff0001000 (15392894357504) bytes at address 2008fff7000 (errno: 12)
==134096==ReserveShadowMemoryRange failed while trying to map 0xdfff0001000 bytes. Perhaps you're using ulimit -v


ERROR Judge error: grader.py (Python 3 (w/PyPy3)) crashed
ERROR Grader stderr:
==134097==ERROR: AddressSanitizer failed to allocate 0xdfff0001000 (15392894357504) bytes at address 2008fff7000 (errno: 12)
==134097==ReserveShadowMemoryRange failed while trying to map 0xdfff0001000 bytes. Perhaps you're using ulimit -v
...

[Matistjati]

Let's hold off with merging this until we get some input on the intended behavior for multiple graders from here Kattis/problem-package-format#529.

[Matistjati]

Me and Fredrik agree that there should be at most one grader, and that we should add it to the spec. We decided not to rename the folder to grader for backwards compatibility, and thus I didn't rename the class name.

[Matistjati]

We should really document what invariants we have for check, setup and what their purposes are.

This check is already performed in check. Do we want to have the an invariant that check is always called before grade? To me, it feels strange that compilation happens as part of check, does it not make more sense to compile as part of setup?

My inferred view is that we keep setup cheap so that we don't have to pay for problem parts we don't use. But then, are we guaranteed that check is always called before calling the part's "do work" function? I would assume yes, in which case we can remove this check.

[gkreitz]

Yes, we should document this better. As I mentioned on Slack, I'm looking to do a larger refactor at some point (separating the parsing of a problem package from the various checks we have), which should hopefully resolve that.

Until then, my understanding is that setup is basically a constructor. It should probably only fail when things are in such a bad state that we're not sure we're making sense of what we're seeing. It is also called from the constructor, so it is safe to assume it has been run before check. That said, setup should basically never do any actual checks (as we may need to run setup on aspects which the user has explictly told us not to check).

[Matistjati]

In TestcaseGroup, there are already checks for this. Do we want to assume that TestcaseGroup's check function must be called before running grade, so these checks can be removed? In that case, should we document that dependency?

The code there is

if self.config['grading'] == 'custom' and self._problem.graders._grader is None:
    self._problem.graders.fatal(f'{self} has custom grading but no custom graders provided')
if self.config['grading'] == 'default' and Graders._default_grader is None:
    self._problem.graders.fatal(f'{self} has default grading but I could not find default grader')

[gkreitz]

Order of checks is still a bit of a mess. I think with the current state of the code base, it's better to be defensive and duplicate checks like this, particularly fatal checks (so we're not gonna spam twice when this happens, as we're going to stop execution).