Implement new system for defining problem-formats

[square-cylinder]

The old system for working with the different aspects of verifying a problem were not flexible enough to use for multiple formats, so a new system has been developed to make this more flexible.

Overview

• All checkable aspects of a problem now have to subclass ProblemPart
• All current classes that were used to verify parts of problems now inherit ProblemPart (except for TestCaseGroup, where a new class ProblemTestCases was added for verification)
• Some minor changes were done in most of these classes to make them work better with the new system, mostly changing how they access data between each other
• When instantiating a Problem, will now have to provide a "part_mapping" which is a dictionary from string (categories) to classes which are checked for that category
• Added a commandline argument to ensure that problems follow a certain format (or 'automatic' if it should be detected from metadata)
• Added handling of KeyboardInterrupt
• Removed pre-legacy version of \problemname{Name}, %% plainproblemname:Name

Description of new system

Definition of a problemformat

The system for describing a problem-format works by providing a dictionary of categories to classes (types), similar to how "part_mapping" worked previously. For example for the legacy configuration right now we have:

{
        'config':       [ProblemConfig],
        'statement':    [ProblemStatementLegacy, Attachments],
        'validators':   [InputValidators, OutputValidators],
        'graders':      [Graders],
        'data':         [ProblemTestCases],
        'submissions':  [Submissions],
}

This means we have the validation steps 'config', 'statement', 'validators', 'graders', 'data', and 'submissions'. These can be included via the commandline-option -p like before. The list of classes are which classes will be checked for each of these parts.

class ProblemPart

This is possible to do via new data that is defined in ProblemPart for each part that can be included. Each problem-part will have to override a class-variable PART_NAME which is a string representing the name of the part. There is a new system for communicating data between different problem-parts through the problem.

The problem has a dictionary for each problem-part where properties can be set by each ProblemPart subclass. This data can then be accessed through for example self.problem.get('statement', 'config'). To set a property for the problem-part you can write for example self.set_prop('foo', 'bar'), which would set the property 'foo' to 'bar'.

It is also possible to access problem.classes['statement'] but the intention is to try to avoid this whenever possible, and rely only on the problem-part-properties instead. Hopefully we will be able to find a way to convert the current classes to follow this in future PR's. The thought-process is that these properties will be the only communication-channel between different parts of the problem to simplify the interactions.

Since some parts require information from other parts in order to be initialized, a system for keeping track of initialization-dependencies was implemented. In each ProblemPart subclass, the method depends_on() -> set[type] is possible to be overridden. For every class that is returned from this method, the problem-class will make sure that a subclass of that class is initialized before this one. For example in the old code, ProblemConfig required some information from ProblemStatement in order to be initialized so ProblemConfig will define:

def depends_on() -> set[type]:
    return {ProblemStatement}

The Problem-class will then ensure that when setting up all the classes, it will resolve all dependencies to the classes before initializing them.

class Problem

Surprisingly little was changed for the Problem-class. The general logic was abstracted a bit to allow for multiple problem-formats, but the overall structure is similar to how it was previously. The Problem class will now require a problem-format in its constructor.

ProblemTestCases

The TestCaseGroup class was difficult to use with the new system since it recursively contains copies of itself, so a new class ProblemTestCases was added as a kind of wrapper around the TestCaseGroup class. It provides a decent example of a ProblemPart-subclass can look since it contains a dependency, is pretty small and uses the new communication-system with the Problem class.

class ProblemTestCases(ProblemPart):
    
    PART_NAME = 'testdata'
    
    @staticmethod
    def depends_on():
        return {ProblemConfig}

    def setup(self):
        self.set_prop('testcase_by_infile', {})
        self.set_prop('root_group', TestCaseGroup(self.problem, self.PART_NAME))
        self.set_prop('is_interactive', 'interactive' in self.problem.get(ProblemConfig, 'data')['validation-params'])
        self.set_prop('is_scoring', self.problem.get(ProblemConfig, 'data')['type'] == 'scoring')

    def check(self, context: Context) -> bool:
        return self.problem.get(ProblemTestCases, 'root_group').check(context)

[gkreitz]

Thanks for the PR. Overall, I like the direction you're heading in here. The PR description is excellent, and helped with navigating the code. Thanks for that!

I think a major issue is that you seem to have missed declaring depends_on for several methods where I think you do in fact depend on other components. I'm guessing this went unnoticed as Problem.get silently returns None when this happens (plus, I guess it's "lucky" that config sorts early :)). Unless I'm missing something, this will be a bit tricky to fix, as I think some of the dependencies are circular.

I also added a few more nit-picky comments (typically accessing via .classes, and/or using hard-coded strings instead of Foo.PART_NAME), which are not blockers for merging, but I wrote them as I was working my way through the code (and while I wasn't sure I understood the PR), and left them in.

[gkreitz]

The ownership of problem.data gets pretty messy when ProblemPart classes take on the responsibility for initializing like this (and even more so when directly writing into the dict in set_prop).

I would prefer if Problem didn't expose data as a dict, but instead took care of initialization, and exposed writing values as a function. Did you consider that approach and rule it out?

If keeping the current version, it would likely be wise to assert that self.PART_NAME not in self.problem.data before doing the assignment, to avoid overwriting data in case multiple parts accidentally use the same PART_NAME

[gkreitz]

'config' here should probably be ProblemConfig.PART_NAME?

[gkreitz]

Ok, now I'm starting to assume I'm missing some subtle detail. Why 'submissions' rather than a PART_NAME (related to the comment above)?

[gkreitz]

and 'config' here (I'll stop pointing these out).

[gkreitz]

This is a syntax error in earlier python versions (which is why the CI tests fail), so you'll want to rewrite this.

[gkreitz]

This also needs to be added as a depends_on

[gkreitz]

Looks like there are missing depends_on here too (ProblemTestCases , OutputValidators, ProblemConfig if I read the code in this class correctly).

[gkreitz]

This feels like a footgun to me. Do you ever use default, I don't think I spotted any calls with it? It feels extremely error prone that calling get silently returns None if part not in self.data (i.e., that part was either a typo, or wasn't initialized yet).

I would prefer if trying to access data from an uninitialized part is always an error (in problemtools, not the problem we're testing :)). If you really need this functionality, I think it makes sense to force the caller to explicitly toggle it on (e.g,. get(self, part, key, default=None, crash_if_part_uninitialized=True))

[gkreitz]

This feels a bit scary. I would love if you either initialize all classes which are subclasses of dependency, or add a check ensuring that there is only a unique class in self.aspects which is a subclass of dependency.

[gkreitz]

It would make sense to me if you did assert _class.PART_NAME not in self.data and then self.data[_class.PART_NAME] = {} just before this line (following up on my earlier comment re ownership of problem.data).

[square-cylinder]

Changes made since last time

Information channel between problem-parts

First of all, if an invalid problem-part is indexed, this will now give an assertion-error. Instead of having an extra layer of key indexing so doing Problem.get(part, key), the indexing is now instead Problem.get(part) and this will return a dictionary so keys can be handled more ergonomically. The set_prop function was removed, because it gave a hint that the data could be modified in for example the check function which was never the intention. Instead, the setup function will now return a dictionary which is the data that gets exposed from a given ProblemPart specialization. The setup of the data is now entirely handled by the Problem class.

Renamed depends_on() to setup_dependencies()

I noticed you had the impression that there were dependencies between parts when the data-access happened through for example the check function. This is not an issue as the dependencies are only relevant for the setup part, because the data should not be changed after that. I realize that the function-name might give the wrong impression, so it was changed to better reflect the intention.

Initializing only one subclass of dependency

The code was changed to make sure exactly one subclass was initialized when initializing dependency, and an additional assertion was added to make sure no two classes had the same PART_NAME when being initialized.

Indexing with string

Fixed all the indexing to be done with the constants instead of with the strings (I think), and changed all the classes[ProblemConfig.PART_NAME] to use Problem.get(ProblemConfig) instead. To make sure no class was using the ProblemConfig.get method anymore this method was removed entirely.

Nested f-string

The nested f-string was changed into to two statements which should hopefully make the tests pass now.

Changed testcase_by_infile to be a class-parameter of ProblemTestCases

Because this is changed as a part of the testcases being ran, this should not be a part of the Problem.get data. Therefore I added is as an instance-variable to ProblemTestCases and accessed it through problem.classes.

[gkreitz]

Thanks for the quick fixes and explanation!

I'm a bit unsure why the unit tests blow up in the same way as previously, they still seem to run the old github action. I fixed the tests in #288, can you try rebase:ing this PR?

I added two minor comments. Neither is a blocker.

Firstly, it seems like 50-50 whether you use .get(key) or [key] when accessing the dict returned by problem.get(...). I'm not sure if there's some subtle pattern I'm not seeing. Both work just fine, but as a reader of the code, using two different styles gets a tad confusing.

Secondly, there was a minor bug in a check, but thankfully you added defense in depth there, making the bug harmless (it should still be fixed, but it's not a blocker).

I'm setting approve now as this looks good enough, but I would love to see unit tests working. Can you try to rebase (and also delete the buggy break), and then we can merge this.

[gkreitz]

Nit-pick: this should probably have the same syntax as other accesses to config data (['type']), unless there's something subtle going on here.

[gkreitz]

This check looks broken, as you didn't remove the break above (it will only detect 0 vs. 1, but won't notice if it appears multiple times).

As you added the assert below, we will still blow up if something occurs multiple times, so this bug is mostly harmless.