Skip to content

rework problem and battle wrapper classes #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 185 commits into from
Feb 9, 2023

Conversation

ImogenBits
Copy link
Collaborator

This PR completes the changes to the problem and battle wrapper classes we discussed. as they are pretty extensive we'll probably have to talk about them in person more, but I'll give a basic overview here:

From the point of view of the Problem class I essentially kept the same interface but simplified it greatly. Instead of having various methods for each step of the parsing and verifying process and dealing with the raw data streams we now have only 4 basic methods:

  • decode takes a path to the folder where the docker container placed the problem instance (or a solution to one) and parses it into a problem object
  • encode does the reverse, taking a path to a folder where the container expects the data and places a representation of itself there
  • check_semantics verifies that the constructed object is semantically correct
  • calculate_score returns a value in [0, 1] that tells us how well a solution performs for a problem instance

All of them take not only the raw data / object but also other relevant info such as instance size or what role we're encoding things for. We also provide default implementations for all of them so that in practice the end user has to rarely touch them.
Of note here are the ProblemModel and SolutionModel classes and the Scored protocol. The exact details of how they work is a little hard to explain and very pythonic but if you look at the new pairsum and biclique problem definitions the semantics should be pretty intuitive.

I also slightly modified what we expect problems to look like. Previously the generator always output a problem instance and a solution and then the solver's task was to provide a better solution. We now just expect the generator to create a problem instance and for the solver to provide a solution. This lets us implement basic decision problems directly (though the calculate_score method might be rather hard to write efficiently then), and even a natural approach to problems in NP \cap co-NP where we don't need the generator to provide a witness and can rely entirely on the solvers.
To still make this style of problem with a generator provided solution easier to write I added the with_solution flag, with it set (the default) the problem behaves more or less like the current ones.

The side of the battle wrappers also changed significantly. The broad takeaway here is an additional layer of abstraction over raw docker images, the Program class and its children Generator and Solver. These essentially handle all of the low level details of parsing between the file systems and the python environment. A battle wrapper gets passed a generator and a solver and can then call their run methods with the appropriate data. We also provide a run_programs method on the base wrapper that safely does precisely that (essentially behaving like a much more powerful FightHandler) and returns an object summarizing the results.

Of note here is that these methods don't take just the instance size or generated instance, but also docker config options such as runtime, space, and number of cpu cores. These don't need to be passed most of the time, the program objects already have the settings as parsed from the cli/config file, they just enable the wrappers to not only adjust the instance size during their run but also other parameters. This lets us make wrappers that eg test the generator to see how long it takes to generate an instance too hard for the solver.

They also take arguments specifying the battle_data. This is essentially any data that we can pass to a container and retrieve from it, as specified by the wrapper instead of the program. This lets us make wrappers that eg let each team remember some fixed amount of data between runs, lets the teams adjust their strategy based on how well they are doing, or even based on previous instances or solutions.

@ImogenBits
Copy link
Collaborator Author

oh i forgot to mention this today and it's a really weird bug to track down if you run into it yourself, the old style of testing docker images don't work with the new docker interaction anymore! because we now use drive mounts the files that we want in the output need to be written there during the execution of the container, so

COPY source output/target
ENTRYPOINT echo 1

won't work, but

COPY source /target
ENTRYPOINT mv /target output/target

will

@Benezivas Benezivas self-assigned this Jan 20, 2023
@Benezivas
Copy link
Collaborator

I have finally got a machine with python3.11 running, so I will start testing these changes soon.

Two things I have noticed while getting the machine up and running on this PR:

  • The default text output for iterated battles does not carry information about the instance size anymore. While in principle the output text needs to be reworked anyway, I would like to do this as a polishing step later and would for now keep the instance size in the output of this wrapper
  • The --display ui option only starts displaying the table containing the results once the first of the default 5 rounds is completed.

More feedback will come once I have time to properly dig into these changes. Thank you already for tackling this bigger rework.

@ImogenBits
Copy link
Collaborator Author

on the first point: it still does this, but only when using verbose logging, I thought that was the previous behaviour too but you're right that it's not.

on the second: yeah, the ui display is pretty broken at the moment (even before this PR), but fixing most of the issues would require pretty extensive reworks that I'd like to only do when I know what the web server side of things actually looks like.

@Benezivas
Copy link
Collaborator

I have started playing around with the new problem model, currently trying to port one of the algobattle-problems problems to the new format.

If I want to upper bound the Field values of a solution attribute by the instance size, I would need access to it already in the SolutionModel subclass I am defining, not in the check_semantics method. Is there a way to access this field when defining the class attributes and their corresponding bounds?

@ImogenBits
Copy link
Collaborator Author

ImogenBits commented Feb 2, 2023

If I'm understanding you correctly you're trying to do something like this:

class MyProblem(ProblemModel):
    size: int

    class MySolution(SolutionModel):
        val: int = Field(le=size)

This is indeed not possible. size is an attribute of a MyProblem instance, its value is not known at the time that MySolution and its fields get defined. The way to achieve this is by using the check_semantics method like this:

class MyProblem(ProblemModel):
    size: int

    class MySolution(SolutionModel):
        val: int

        def check_semantics(self, instance: MyProblem, size: int) -> bool:
            return self.val <= instance.size

But this is a good opportunity to consider this kinda situation more abstractly:
Maybe check_semantics is badly named. The idea is that .decode is responsible for generating a python representation of the student program output. check_semantics then confirms that this is indeed a valid output. Pydantic (the lib we use to automatically parse json into python objects using the class and Field definitions) operates in a way that somewhat overlaps both of these steps. It not only parses json data into a python object, but also validates some properties of this. For some very basic properties this is unavoidable, e.g. pydantic must validate that the json is well formed, that it contains the correct keys with correctly typed values, etc.
But for things like size bounds we might expect them to happen during the check_semantics step. But letting pydantic handle it is so much more user friendly that it is far easier to just pass it off to it. But then some things, like bounds that are dependent on MyProblem instance attributes, still have to be verified in check_semantics.

Really this is an issue of where we draw the line between "syntactically" incorrect and "syntactically" correct, but "semantically" incorrect outputs. This distinction seems pretty arbitrary and I think it ultimately is highly situational. The same property like the output being a complete graph might be entirely "semantic" for most graph problems, but for things like TSP it is a "syntactic" requirement on the input.
This, together with the issues of wanting to let pydantic handle as much as it possibly can, makes me think that maybe we should drop that distinction and rename check_semantics to something like validate, extra_validation, postprocess, or something similar. Then the intention of just being a post-hook in the creation process of python objects from student program output is more clear. The current split in log messages could still be preserved with some language adjustments.
Another solution would be to completely remove check_semantics. We would then pass the required additional data to .decode and let it handle everything. The current functionality of it acting as an easy post-hook is then taken care of by normal inheritance practices. I personally am not a huge fan of this idea since it makes the decoding step kinda weird and asymmetric.

@Benezivas
Copy link
Collaborator

I agree, the separation between syntax and semantic of a problem instance is very blurry, also in the old format.

The proposed solution of doing the size checks in the "semantics" section is fine, a renaming to validation seems appropriate. I will try to port one or two more problems that I remember to have esoteric behaviour (TSP wanting to give the solving team more slack during verification than the generating team, Path bounded Vertex Cover needing a certificate intermediate solution) and then come back to this PR.

Feel free to do necessary changes meanwhile, I do not mind adapting rewritten problems again.

@Benezivas
Copy link
Collaborator

I am currently trying to implement a problem that is trying to distinguish between a solver solution and a generator solution in order to determine the quality of a solution: The solver is supposed to have an advantage in the calculation.

The current draft of the problem file is the following:

"""The Tsptimewindows problem class."""
import logging
import math
import sys

from typing import ClassVar, TypeAlias, Any, SupportsFloat
from pydantic import Field

from algobattle.problem import ProblemModel, SolutionModel

logger = logging.getLogger('algobattle.problems.tsptimewindows')

_Solution: TypeAlias = Any


class Tsptimewindows(ProblemModel):
    """The Tsptimewindows problem class."""

    name: ClassVar[str] = "Traveling Salesman with Time Windows"
    min_size: ClassVar[int] = 2

    num_vertices: int = Field(ge=0, le=2 ** 63 - 1)
    indices: tuple[int, ...] = Field(ge=0, le=2 ** 63 - 1)
    positions: list[tuple[float, float]] = Field(ge=0, le=100)
    time_windows: list[tuple[float, float]] = Field(ge=0, le=10000)

    def check_semantics(self, size: int) -> bool:
        return (
            self.num_vertices <= size
            and len(self.indices) == len(self.positions) == len(self.time_windows)
            and all(i < self.num_vertices for i in self.indices)
            and all(u <= v for (u, v) in self.time_windows)
            and len(self.indices) == len(set(self.indices))
        )

    def _distance(self, a: int, b: int) -> float:
        return math.sqrt(
            (self.positions[a][0] - self.positions[b][0]) ** 2
            + (self.positions[a][1] - self.positions[b][1]) ** 2
        )

    class Solution(SolutionModel):
        """A solution to a Traveling Salesman with Time Windows problem."""

        direction: ClassVar = "maximize"
        tour: tuple[int, ...] = Field(ge=0, le=2 ** 63 - 1)

        def check_semantics(self, instance: "Tsptimewindows", size: int) -> bool:
            return (
                len(self.tour) == len(instance.indices)
                and len(self.tour) == len(set(self.tour))
                and all(i < instance.num_vertices for i in self.tour)
            )

        def _length_of_tour(self, solving_team: bool) -> float:
            start_node = self.tour[0]
            mult = 0.9 if solving_team else 1.0
            total_time = self.time_windows[start_node][0] * mult

            last_node = self.tour[0]
            for i in range(1, len(self.tour) - 1):
                current_node = self.tour[i]
                travel_time = mult * self._distance(last_node, current_node)
                # Do we arrive in time?
                if total_time + travel_time > self.time_windows[current_node][1]:
                    return sys.float_info.max
                # We may have to wait at the node until it is available
                total_time = max(total_time + travel_time, self.time_windows[current_node][0])
                last_node = current_node

            travel_time = mult * self._distance(last_node, start_node)
            if total_time + travel_time > self.time_windows[start_node][1]:
                return sys.float_info.max
            # We may have to wait at the node until it is available
            total_time = max(total_time + travel_time, self.time_windows[start_node][0])

            return total_time

    def calculate_score(self, solution: _Solution, generator_solution: _Solution | None, size: int) -> SupportsFloat:
        gen_score = generator_solution._length_of_tour(solving_team=False)
        sol_score = solution._length_of_tour(solving_team=True)

        if gen_score == 0:
            return 1
        if sol_score == 0:
            return 0

        return gen_score / sol_score

The main issue lies in the question of how to access the attributes of the Tsptimewindows class, as an instance parameter is not available everywhere: The calculate_score method cannot access the instance and can thus not pass it as a parameter to the determine_length method that is calculating the quality of a solution (the length of a TSP tour). However, the determine_length method does need access to the attributes of the Tsptimewindows class.

Since we are not in a simple subclass relation between Tsptimewindows and Solution, this access does not seem possible either. Is there a simple way to achieve a distinct scoring calculation depending on the role that Is providing the solution?

@Benezivas
Copy link
Collaborator

Nevermind, I can simply pass self to the_determine_length method from inside the calculate_score method and it works as intended.

@Benezivas
Copy link
Collaborator

I think I have played around sufficiently with the new problem design to say that this rework makes it much easier to design and write new problems. The pure reduction of effort regarding parsing makes the problem classes much slimmer and easier to check for correctness. I did not see a loss of expressive power compared to the previous version, which should make adapting the remaining problems rather effortless.

I did not have the opportunity to play around with the new battle wrappers and to implement a new one, but the proposed changes seem reasonable and I do not see an issue in merging them into the rc right now.

Thank you very much for the changes. I will port more problems to the new format over time, for which we should also discuss a standard of how to format the documentation soon. The READMEs currently still emulate the generated pdfs we use for the lab courses and are not very pretty. Since the algobattle-web framework is likely also interested in displaying these descriptions, we should discuss this in our next meeting.

@Benezivas Benezivas merged commit abf4c14 into Algorithmic-Battle:4.0.0-rc Feb 9, 2023
@ImogenBits ImogenBits deleted the problem branch February 12, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants