Add Python API to get all constraints

[gsingh93]

I know solver.dumps will print all of the constraints, but I would find it useful to be able to get a list of Constraint objects in Python. I'm finding debugging unsatisfied constraint issues difficult, and if I had access to the actual constraints I could automate some of the process, i.e. filtering to just the constraints affect the unsatisfied constraint, simplifying some of the output, etc.

[MatthieuDartiailh]

Doing this would require to also extend the C++ API. I know how to do it and I can see the value in the feature but I would like @sccolbert opinion on such a change, before working on it.

[gsingh93]

@sccolbert Any thoughts on this?

[sccolbert]

I don’t think I fully understand what you are asking. You create the constraints yourself before adding them to the solver. Could you just add them to your own list at the same time?

On Fri, May 19, 2023 at 03:18 Gulshan Singh @.***> wrote:

@sccolbert https://github.com/sccolbert Any thoughts on this?

— Reply to this email directly, view it on GitHub https://github.com/nucleic/kiwi/issues/161#issuecomment-1554215606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSITTMZXLH6RU7YH3MLXG4UETANCNFSM6AAAAAAYBJ3A2M . You are receiving this because you were mentioned.Message ID: @.***>

[gsingh93]

Yes, but in my codebase constraints are added in a number of different places, it would be a messy and intrusive change to keep track of all of them myself just for debugging.

That's why if we could get access to the constraints from the solver, which already has a list of them, it would be much more convenient.

[sccolbert]

Can you subclass the solver and override the add_constraint method?

On Fri, May 19, 2023 at 09:50 Gulshan Singh @.***> wrote:

Yes, but in my codebase constraints are added in a number of different places, it would be a messy and intrusive change to keep track of all of them myself just for debugging.

That's why if we could get access to the constraints from the solver, which already has a list of them, it would be much more convenient.

— Reply to this email directly, view it on GitHub https://github.com/nucleic/kiwi/issues/161#issuecomment-1554707463, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSP6AJWG7O4OOKDXEODXG6CC7ANCNFSM6AAAAAAYBJ3A2M . You are receiving this because you were mentioned.Message ID: @.***>

[gsingh93]

I think in the meantime that's what I'll do. But if anyone else needs this functionality in the future, or if I need it for a different project, we'll have to reimplement it in those projects. Since this library already has access to the list of constraints, it makes more sense to me to have this exposed from the library itself than have users reimplement this.

Is there any reason you're thinking of why we shouldn't do this?

[gsingh93]

Related to this, other classes like UnsatisifiableConstraint could also expose the actual constraint object being violated. I'm currently catching the exception, parsing the message string for some variable names, and then searching through the list of constraints in my wrapper class for variables that have that matching name. It would be easier if I could just do e.constraint to get the constraint being violated.

[gsingh93]

If anyone else needs this:

from typing import Optional

import kiwisolver
from kiwisolver import Constraint, UnsatisfiableConstraint, Variable

class Solver:
    def __init__(self):
        self._solver = kiwisolver.Solver()
        self._constraints = set()

    def _find(self, v: str) -> tuple[Optional[Variable], set[Constraint]]:
        res = set()
        res_strs = set()
        var = None
        for c in self._constraints:
            for term in c.expression().terms():
                if term.variable().name() == v:
                    var = term.variable()

                    # Skip duplicate constraints
                    if str(c) in res_strs:
                        continue

                    res_strs.add(str(c))
                    res.add(c)

        return var, res

    def add(self, c: Constraint) -> None:
        self._constraints.add(c)
        self._solver.addConstraint(c)

    def remove(self, c: Constraint) -> None:
        self._constraints.remove(c)
        self._solver.removeConstraint(c)

    def hasConstraint(self, c: Constraint) -> bool:
        return self._solver.hasConstraint(c)

    def update(self):
        self._solver.updateVariables()

    def analyze(
        self, e: UnsatisfiableConstraint
    ) -> list[tuple[Variable, set[Constraint]]]:
        variables = []

        msg = str(e)
        constraint, _ = msg.split(" | ")
        op = None
        for _op in [" <= ", " >= ", " == "]:
            if _op in constraint:
                op = _op
                break

        assert op is not None

        expr, constant = constraint.split(op)
        assert constant == "0"
        terms = expr.split(" + ")
        for term in terms:
            res = term.split(" * ")
            if len(res) == 2:
                _, var = res
                variables.append(self._find(var))

        return variables

Calling the analyze function with an UnsatisfiableConstraint exception produces output that's significantly more easy to debug than calling solver.dumps().

[Qix-]

I think this makes sense, actually - especially with bool violated() now being a thing. Being able to pass a Solver reference to a function and asking it "give me a list of constraints that failed" would be immensely helpful to have without needing to have an extra collection of constraints, thus duplicating memory in a lot of ways.

[sccolbert]

The main reason why this doesn't already exist, is because the solver doesn't actually store the Python constraint object under the hood. It stores the C++ constraint object which is created internally when you create a constraint from Python. So in order to return a list of Python constraints from a method like solver.constraints(), the solver would have to allocate new Python constraint objects to wrap the C++ constraint objects. That would introduce a few gotcha's when it comes to comparing objects via is.

If we were to add this, we would need to update the Python wrapper class to hold an internal list of the constraints, similar to what your sample code above is doing. We wouldn't make a change on the C++ implementation class.

[sccolbert]

@gsingh93 Regarding the UnsatisfiableConstraint. The constraint object should be contained in the .value attribute of the exception:

https://github.com/nucleic/kiwi/blob/main/py/src/solver.cpp#L59

[sccolbert]

At least I always thought it was the .value attribute. Whoops. Turns out it's .args[0].

[gsingh93]

So in order to return a list of Python constraints from a method like solver.constraints(), the solver would have to allocate new Python constraint objects to wrap the C++ constraint objects. That would introduce a few gotcha's when it comes to comparing objects via is.

Got it, thanks for the explanation. I think this could still be valuable if it's disabled by default and enabled with some flag that makes it clear it's only for debugging, but it's also fine to not include it and people can just something similar to the wrapper code I posted above. Alternatively, it would be nice if there was some optional argument to dumps where I could just dump constraints related to the variables I cared about, like in the wrapper code I posted above (the wrapper code only dumps constraints where the variable is directly in the constraint, which is already very helpful, but if this was implemented natively it may be easier to dump all constraints that affect the variable, even if the variable isn't directly in that particular constraint).

Regarding the UnsatisfiableConstraint. The constraint object should be contained in the .value attribute of the exception. At least I always thought it was the .value attribute. Whoops. Turns out it's .args[0].

Awesome, thanks, I'll switch to using this. Would be nice to have a dedicated attribute like .value or .constraint so it's easier to discover (I made an empty UnsatisifiableConstraint object and didn't see anything related in __dir__ so I figured it may not exist).

[sccolbert]

Yeah. Adding a .value attribute would require us to make a full-on subclass for each exception object. The Python C-API has a simple function for creating a "dumb" exception subclass with no new attributes or methods, so that's what I used because it was quick and did the job at the time.

On Sat, May 20, 2023 at 6:42 PM Gulshan Singh @.***> wrote:

So in order to return a list of Python constraints from a method like solver.constraints(), the solver would have to allocate new Python constraint objects to wrap the C++ constraint objects. That would introduce a few gotcha's when it comes to comparing objects via is.

Got it, thanks for the explanation. I think this could still be valuable if it's disabled by default and enabled with some flag that makes it clear it's only for debugging, but it's also fine to not include it and people can just something similar to the wrapper code I posted above. Alternatively, it would be nice if there was some optional argument to dumps where I could just dump constraints related to the variables I cared about, like in the wrapper code I posted above.

Regarding the UnsatisfiableConstraint. The constraint object should be contained in the .value attribute of the exception. At least I always thought it was the .value attribute. Whoops. Turns out it's .args[0].

Awesome, thanks, I'll switch to using this. Would be nice to have a dedicated attribute like .value or .constraint so it's easier to discover (I made an empty UnsatisifiableConstraint object and didn't see anything related in dir so I figured it may not exist).

— Reply to this email directly, view it on GitHub https://github.com/nucleic/kiwi/issues/161#issuecomment-1556043432, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSJ73Q5ZC4GDPMLUGHLXHFJFHANCNFSM6AAAAAAYBJ3A2M . You are receiving this because you were mentioned.Message ID: @.***>