BiConstraintCollector doesn't

[thimmwork]

Describe the bug
I'm implementing a custom BiConstraintCollector for use in groupBy() constraint streams.
For some reason, the elements are not collected, the provided ResultContainer remains empty, and the constraint doesn't penalize.
It might very well be that I'm using the API wrong (e.g. the two consecutive calls of groupBy come to mind). I'll gladly take advice in any direction.

Expected behavior
Invocation of the Runnables for each tuple. Passing of the test.

Actual behavior
The Runnables are not invoked. The test fails with

java.lang.AssertionError: Broken expectation.
        Constraint: ai.timefold.solver.examples.cloudbalancing.domain/differenceInRequiredCpu
  Expected penalty: 1 (class java.lang.Integer)
    Actual penalty: 0 (class java.lang.Integer)

  Explanation of score (0hard/0soft):
    Constraint matches:
        0: constraint (differenceInRequiredCpu) has 1 matches:
            0: justified with ([{}])
    Indictments:
        0: indicted with ({}) has 1 matches:
            0: constraint (differenceInRequiredCpu)

To Reproduce

My minimal example is based on the CloudBalancing example, and introduces the following additional constraint (which seems odd in this particular example, but for demonstration purposes I hope it'll do):
For each computer, I want to penalize the scheduling of processes that have different cpu requirements. And I want to do this in a per-computer way, so the cost doesn't explode when scheduling lots of small tasks.
So I implemented a MinMaxCollector, with the intention to determine the minimum and maximum cpu requirements of scheduled processes per computer.

I forked the timefold-solver repo and added my BiConstraintCollector, the additional constraint and a unit test here:
https://github.com/thimmwork/timefold-solver/blob/4862eef96575fc061eb5b5cb0727b791790e184a/examples/src/test/java/ai/timefold/solver/examples/cloudbalancing/score/CloudBalancingConstraintProviderTest.java#L84C21-L84C21

This is the constraint I added to CloudBalancingConstraintProvider.
I'm using the computer.id as key and want to collect the min and max requiredCpuPower of all the processes assigned to each computer.

   Constraint differenceInRequiredCpu(ConstraintFactory constraintFactory) {
        return constraintFactory.forEach(CloudProcess.class)
                .filter(cloudProcess -> cloudProcess.getComputer() != null)
                .groupBy(cloudProcess -> cloudProcess.getComputer().getId(), cloudProcess -> cloudProcess.getRequiredCpuPower())
                .groupBy(MinMaxCollector.collector())
                .penalize(HardSoftScore.ONE_HARD,
                        cpuPowerByComputer -> penalizeByCpuDifference(cpuPowerByComputer.values()))
                .asConstraint("differenceInRequiredCpu");
    }

    private Integer penalizeByCpuDifference(Collection<Pair<Integer, Integer>> minAndMaxCpuByComputer) {
        return minAndMaxCpuByComputer.stream()
                .mapToInt(minMax -> minMax.getRight() - minMax.getLeft())
                // use the square to penalize bigger differences way more than smaller differences
                .map(difference -> difference * difference)
                .sum();
    }

This is my Collector (for easier code analysis I'm using anonymous classes here)

public class MinMaxCollector<KEY, COMP extends Comparable<COMP>> {
    private final Map<KEY, Pair<COMP, COMP>> minMaxPerKey = new HashMap<>();

    public void collect(KEY key, COMP other) {
        var minMax = minMaxPerKey.get(key);
        var replace = false;
        if (minMax == null) {
            minMax = ImmutablePair.of(other, other);
            replace = true;
        } else if (minMax.getLeft().compareTo(other) > 0) {
            minMax = ImmutablePair.of(other, minMax.getRight());
            replace = true;
        }
        if (minMax.getRight().compareTo(other) < 0) {
            minMax = ImmutablePair.of(minMax.getLeft(), other);
            replace = true;
        }
        if (replace) {
            minMaxPerKey.put(key, minMax);
        }
    }

    public Map<KEY, Pair<COMP, COMP>> getMinMaxPerKey() {
        return minMaxPerKey;
    }

    public static <KEY, COMP extends Comparable<COMP>>
            BiConstraintCollector<KEY, COMP, MinMaxCollector<KEY, COMP>, Map<KEY, Pair<COMP, COMP>>> collector() {
        return new BiConstraintCollector<>() {
            @Override
            public Supplier<MinMaxCollector<KEY, COMP>> supplier() {
                return MinMaxCollector::new;
            }

            @Override
            public TriFunction<MinMaxCollector<KEY, COMP>, KEY, COMP, Runnable> accumulator() {
                return new TriFunction<MinMaxCollector<KEY, COMP>, KEY, COMP, Runnable>() {
                    @Override
                    public Runnable apply(MinMaxCollector<KEY, COMP> minMaxCollector, KEY key, COMP comp) {
                        return new Runnable() {
                            @Override
                            public void run() {
                                minMaxCollector.collect(key, comp);
                            }
                        };
                    }
                };
            }

            @Override
            public Function<MinMaxCollector<KEY, COMP>, Map<KEY, Pair<COMP, COMP>>> finisher() {
                return MinMaxCollector::getMinMaxPerKey;
            }
        };
    }
}

and a unit test that I added to CloudBalancingConstraintProviderTest:

    @ConstraintProviderTest
    void useThisTestToReproduce(ConstraintVerifier<CloudBalancingConstraintProvider, CloudBalance> constraintVerifier) {
        CloudComputer computer1 = new CloudComputer(1, 1, 1, 1, 2);
        CloudProcess process1 = new CloudProcess(1, 1, 1, 1);
        CloudProcess process2 = new CloudProcess(2, 3, 1, 1);
        process1.setComputer(computer1);
        process2.setComputer(computer1);

        constraintVerifier.verifyThat(CloudBalancingConstraintProvider::differenceInRequiredCpu)
                .given(computer1, process1, process2)
                .penalizesBy(1);
    }

My expectation would be that the above unit test would trigger the same invocations as this test, which tests the collector directly:

    @Test
    public void biconstraintcollector_smoke_test() {
        //given
        Integer computerId = 1;
        BiConstraintCollector<Integer, Integer, MinMaxCollector<Integer, Integer>, Map<Integer, Pair<Integer, Integer>>> biConstraintCollector = MinMaxCollector.collector();

        //when
        MinMaxCollector<Integer, Integer> minMaxCollector = biConstraintCollector.supplier().get();
        TriFunction<MinMaxCollector<Integer, Integer>, Integer, Integer, Runnable> accumulator = biConstraintCollector.accumulator();

        // A: min=1, max=10
        accumulator.apply(minMaxCollector, computerId, 1).run();
        accumulator.apply(minMaxCollector, computerId, 3).run();

        //then
        var finisher = biConstraintCollector.finisher().apply(minMaxCollector);
        assertEquals(1, finisher.get(computerId).getLeft());
        assertEquals(3, finisher.get(computerId).getRight());
    }

This test works, so the problem is either outside of the BiConstraintCollector or in my understanding.

Environment

Timefold Solver Version or Git ref:
tested on solver 1.20, 1.4.0, 1.5.0 and git commit 490490a (current main)

Output of java -version:
openjdk 17.0.9 2023-10-17
OpenJDK Runtime Environment Temurin-17.0.9+9 (build 17.0.9+9)
OpenJDK 64-Bit Server VM Temurin-17.0.9+9 (build 17.0.9+9, mixed mode)

Output of uname -a or ver:
Darwin WYJTH5W7YK 22.6.0 Darwin Kernel Version 22.6.0: Fri Sep 15 13:41:28 PDT 2023; root:xnu-8796.141.3.700.8~1/RELEASE_ARM64_T6000 arm64

Additional information

Provide any and all other information which might be relevant to the issue.

[triceo]

Hey there!

The problem appears to be in your accumulator. You need something like this:

 @Override
 public TriFunction<MinMaxCollector<KEY, COMP>, KEY, COMP, Runnable> accumulator() {
   // Insert code that does the accumulation. This is your `collect(...)` function.
   
   // Now return a new Runnable which can *undo* that accumulation, 
   // returning the solution to the state before the accumulation was made.
 }

In your accumulator, you're only returning the undo Runnable, and that means that nothing gets accumulated. There is no code to actually accumulate anything.

I'll convert this issue to a discussion, as my current understanding indicates this is not a solver problem. If there's still something to explain, we can continue there.