feat: Declarative Shadow Variables

[Christopher-Chianelli]

VariableListeners are notoriously difficult to write, especially when you have multiple variable depending on each others.
Declarative shadow variables fixes this by providing a declarative way to define the dependencies of your shadow variables and how to calculate them.

First, annotate your variables as @ProvidedShadowVariable:

@PlanningEntity
public class Visit {
    String id;

    @InvalidityMarker
    boolean isInvalid;   // If true, the visit is in a "loop" and some of its provided shadow variables will be null

    @ProvidedShadowVariable(MyShadowVariableProvider.class)
    LocalDateTime serviceReadyTime;
    
    // ...
}

Next, create a ShadowVariableProvider to define your variables:

public class MyShadowVariableProvider implements ShadowVariableProvider {
    @Override
    public void defineVariables(ShadowVariableFactory variableFactory) {
        // The previous visit; you don't need to use @PreviousElementShadowVariable
        var previousVisit = variableFactory.entity(Visit.class).previous();
        // The vehicle the visit is assignedf to; you don't need to use @InverseRelationShadowVariable
        var vehicle = variableFactory.entity(Visit.class).inverse(Vehicle.class);
        
        // The service ready time; when the technician will arrive
        var serviceReadyTime = variableFactory.newShadow(Visit.class)
               // If a visit has a previous visit, the ready time 
                .compute(previousVisit.fact(Location.class, Visit::getLocation),
                         previousVisit.variable(LocalDateTime.class, "serviceFinishTime"),
                         (visit, previousLocation, previousEndTime) -> previousEndTime.plus(visit.getLocation().travelTimeFrom(previousLocation))
                )
                // If a visit has no previous visit, use the vehicle's start time
                .orCompute(vehicle.fact(Location.class, Vehicle::getLocation),
                        vehicle.fact(LocalDateTime.class, Vehicle::getStartTime),
                        (visit, startLocation, startTime) -> startTime.plus(visit.getLocation().travelTimeFrom(startLocation))
                )
                // If a visit is unassigned, its serviceReadyTime is null
                .as("serviceReadyTime");
        
         // The service start time; when service can start
         // If the visit is a part of a group, service cannot start until all technicians arrive
         // Otherwise, it the same as the service start time
         var visitGroup = variableFactory
                        .entity(Visit.class)
                        .group(Visit.class, Visit::getVisitGroup);
         var serviceStartTime = variableFactory.newShadow(Visit.class)
                // If the visit has a visit group, its start time is the max of the ready times of the visit in its visit group
                .compute(visitGroup.variables(LocalDateTime.class, "serviceReadyTime"),
                        (visit, groupReadyTimes) -> groupReadyTimes.isEmpty() ? null : Collections.max(groupReadyTimes))
                // Otherwise, it the service ready time of this visit
                .orCompute(serviceReadyTime, (visit, readyTime) -> readyTime)
                .as("serviceStartTime");

         // Each service ends 30 minutes after it started
         var serviceFinishTime = variableFactory.newShadow(Visit.class)
                .compute(serviceStartTime, (visit, startTime) -> startTime.plusMinutes(30))
                .as("serviceFinishTime");
    }
}

and finally, write tests to ensure your shadow variables work correctly:

@Test
public void shadowVariables() {
      var sessionFactory = ShadowVariableSessionFactory.create(
              SolutionDescriptor.buildSolutionDescriptor(RoutePlan.class,
                      Vehicle.class, Visit.class),
              new MyShadowVariableProvider());

      var vehicle = Vehicle("v1");
      var visit1 = Visit("c1");
      var visit2 = Visit("c2");
      var visit3 = Visit("c3");

      var session = sessionFactory.forEntities(vehicle, visit1, visit2, visit3);
      session.setInverse(visit1, vehicle);
      session.setPrevious(visit2, visit1);
      session.setPrevious(visit3, visit2);
      session.updateVariables();

      assertThat(visit1.getServiceReadyTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME);
      assertThat(visit1.getServiceStartTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME);
      assertThat(visit1.getServiceFinishTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME.plusMinutes(30L));
      assertThat(visit1.isInvalid()).isFalse();

      assertThat(visit2.getServiceReadyTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME.plusMinutes(60L));
      assertThat(visit2.getServiceStartTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME.plusMinutes(60L));
      assertThat(visit2.getServiceFinishTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME.plusMinutes(90L));
      assertThat(visit2.isInvalid()).isFalse();

      assertThat(visit3.getServiceReadyTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME.plusMinutes(120L));
      assertThat(visit3.getServiceStartTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME.plusMinutes(120L));
      assertThat(visit3.getServiceFinishTime()).isEqualTo(TestShadowVariableProvider.BASE_START_TIME.plusMinutes(150L));
      assertThat(visit3.isInvalid()).isFalse();
}

Behind the scenes, Timefold Solver calculates a valid topological order for each of your shadow variables.
This allows Timefold Solver to:

1. Only recalculate changed shadow variables. A shadow variable is recalculated if any of its declared inputs has changed. Using an undeclared variable input will lead to score corruptions.
2. Determine what variables are in a "loop". Variables are in a "loop" if:

• They directly or indirectly depend on each other.
• They depend on looped variables.

Currently a WIP:

• Write javadoc
• Write documentation
• Write additional tests
• Write additional use cases
• Finalize API

[triceo]

Before I do a proper review, I suggest we do some house-keeping first. (Otherwise comments would get lost when files are being renamed and moved around.)

• Are we happy with the package? Variables generally go into api.domain.variable, so in this case preview.api.domain.variable.declarative?
• Preview features need to be declared in PreviewFeature; features not enabled through this enum need to fail fast when used. (In this case, probably when the new annotations are being processed.)

[triceo]

We need to be careful here. This feature should have no performance impact when disabled. I'll run some numbers to confirm that.

[triceo]

My understanding of this API is still limited, but do we actually need this public API anywhere in the solver? IMO this is internal in core, and only exposed in the tests; therefore I'd make this public in the tests, not in the solver itself.

[Christopher-Chianelli]

It can be moved; it is here for convenience when testing the prototype. (core internally depends on the default implementation which has before/after methods).

[triceo]

Dtto.

[triceo]

Hypothetical scenario:

• I have two different shadow providers.
• But they end up declaring/depending on the same shadow variables.

Do they all become part of the same graph? In other words - is the graph limited to one shadow provider, or is it built from all shadow providers combined, and then split into individual independent components?

[Christopher-Chianelli]

Hypothetical scenario:

* I have two different shadow providers.

* But they end up declaring/depending on the same shadow variables.

Do they all become part of the same graph? In other words - is the graph limited to one shadow provider, or is it built from all shadow providers combined, and then split into individual independent components?

Although untested, it is built from all shadow providers combined, and then split into individual independent components. Defining the same shadow variable twice is an error though; (you can use it multiple time, but cannot declare it multiple times).

[triceo]

Please add this to the list of things that need to be tested.

[ge0ffrey]

This seems similar to ConstraintVerifier, so the naming should be similar.
Does the ConstraintVerifier API have anything that is a Session?

[triceo]

As far as I understand, this does not need to be in the public API. Please remove.

[triceo]

First round of review, surface-level.
Next round will look deeper into the logic.

[triceo]

We call that "preview feature".

[triceo]

Please write all new code using var, unless it'd lead to code that's worse than the alternative. (Typically if it would require nasty casting etc.)

[triceo]

Arguably can be a record, eliminating some boilerplate.
before/after biconsumer can be exposed directly, avoiding indirection.

[triceo]

Please remove.

[triceo]

If I understand this correctly, this serves for testing of the shadow variables. As such, shouldn't this be moved to timefold-solver-test, together with constraint verifier et al.? If it only serves for our own internal purposes, then it should arguably be in test and not main.

[triceo]

As far as I understand, this does not need to be in the public API. Please remove.

[triceo]

Likewise, please remove from public API. The user has no use for this. Maybe this needs to exist for testing, in which case I'd move it to timefold-solver-test. Better yet, let's just hide it, because we have not yet discussed testing of these new shadow variables at all.

[triceo]

TODO: Naming and Javadoc.

Arguably, this is not an "updater". It doesn't update the variable. It's a function that computes its new value, but it doesn't set it.

Calculator? Computer? Value Function? Value Provider? ...

[triceo]

Please remove references to FSR. FSR has no meaning in the context of the solver.

[triceo]

Just as with "FSR", "TA" is also meaningless here. The solver isn't tailored to any particular problem, and these domain classes should therefore carry generic names. We already have dozens of them, and we were able to follow this policy; I don't see the need to divert now.

[triceo]

General comments having read the entire PR in depth:

• The code structure could be improved. There are several very long methods, which (when broken down into smaller, well-named ones) will help the algorithms to be self-documenting.
• The public API of the feature is undocumented.
• The testing infrastructure for the new mechanism should be hidden for now; we haven't discussed it at all, and we should not expose it to the users since we only have a limited amount of time to get this merged.
• The tests are complex enough to warrant comments.

We need to go through a naming excercise for the two new annotations we expose. Also, the feature name itself ("declarative shadow variables") is arguably no longer accurate. Let's leave that for the end, once everything is properly documented, at which point we'll have the most accurate information to infer correct names for concepts.

[triceo]

This approach to shadow variables can IMO no longer be called "declarative". It used to be declarative, when all the handles etc. had to be declared. Now it's just a better variable listener. We need a new name for the feature.

[triceo]

Where are the fail-fasts for when the other fields are used? The Javadoc suggests that they exist, but I didn't notice them.

[Christopher-Chianelli]

Fail fasts are in RootVariableSource, which does the processing of the variable paths.

[triceo]

Considering what the descriptor is used for, it can easily be replaced by VariableMetaModel.

[triceo]

"Accessing a declarative variable from another declarative variable" clearly doesn't mean what I think it means, because accessing one variable for another is the goal of this entire "a.b" syntax.

[Christopher-Chianelli]

The older version the PR was not tested with more exotic cases, such as "previous.previous" or "factProperty.previous" or "myShadowVariable.myOtherShadowVariable".

The old impl made it harder to see the inherent issues that this impl makes obvious by removing all the indirection.

Basically, the three rules are:

• You can only access normal shadow variables from the root entity or from a collection element. Otherwise, the variable listener need to maintain an inverse set to lookup the affected entities for a shadow variable change.
• You cannot chain multiple declarative shadow variables together in a single path. For instance, "closestStation.visitCount"; this has the potential to change the dependency/topology of the graph in the middle of an update.
• Your path must have at least one variable in it, and all names must be the name of members in the class.

[triceo]

Two comments:

• Let's make sure these rules are listed in a Javadoc somewhere. Lately we've been touching decade-old code, and we often wished the authors of that code had the foresight to list all of their thinking, rules and assumptions somewhere. Code archeology is not fun, and in some of my more recent PRs, I have also started adding significantly more comments than in the past. Not so much about behavior (behavior is often obvious), but about motivation, intentions and circumstances.

• "Your path must have at least one variable in it, and all names must be the name of members in the class." What else could be there in the path? I'd argue nothing but variables. Constants do not need to be referenced - we only need to reference what we need to build and maintain the graph.

[triceo]

VariableMetaModel was invented earlier to serve this purpose.
(VariableDescriptor is more of a utility class, and it is a mess. VariableMetaModel is a well-defined carrier of metadata.)

[triceo]

Can we do this without labels? My experience has been that, whenever I felt like I need to use explicit goto, there was a better code structure that avoided it.

[triceo]

If an uncaught exception occurred, why are we even here? The solver would've crashed before getting here. (The undo move issue was already fixed.)

[triceo]

The method is long enough already, even without data types being declared inside of it. Please move this type outside - you can still make it private.

[triceo]

These tests are complex enough that they deserve comments.
What behavior is being tested by each group of lines, why is the expected result what it is.

[triceo]

Let's not overdo it with the tests that run the solver.
I'm fine with having this one, it's a good end-to-end test, but let's test future functionality on a much more granular level.

[triceo]

Variable descriptors should not equal one another, each should be a unique instance. Therefore == ought to be enough here...

[triceo]

... but ordinal has the same function. In either case, arguably we should put this logic inside VariableDescriptor's equals/hashCode and not externalize it here.

[triceo]

And maybe I would add a comment here that this class is often used as key in hashmaps, and therefore this method needs to be optimized.

[triceo]

This method is very long and also - not coincidentally - shows up as a dominator in the flamegraph. Please break this method down into smaller methods, so that we can better understand the logic, and so that the flame graph can actually meaningfully show how much each part contributes to the result.

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE