feat: support sourcing declarative shadow variables from planning list variable elements - #2506
Conversation
…t variable elements An entity declaring a @PlanningListVariable can now use a @Shadowsources path such as "values[].endTime" to declare that one of its declarative shadow variables depends on a declarative shadow variable of the list's elements, e.g. a vehicle-level end time aggregated from its visits. Previously, this dependency was inexpressible: "values[].endTime" failed fast (accesses a collection via a variable) and "lastValue.endTime" failed fast as well (declarative accessed from declarative). The closest available spelling, a bare list variable source such as @Shadowsources("values"), was accepted and documented but silently broken: it was computed once when the variable reference graph was built and never retriggered during solving. - RootVariableSource accepts a [] segment on a genuine list variable declared on the root entity, producing the new ParentVariableType.LIST_ELEMENT. The path must end on a declarative shadow variable of the element (optionally reached via a fact). - The initial fan-in edges (element.shadow -> owner.shadow) are created from the list's contents when the graph is built. - List variable change events are now forwarded to the declarative shadow variable session, which incrementally adds/removes the fan-in edges for the changed range and always marks the owner variable changed, so the aggregate also reacts to reorders and removals. - GraphStructure classifies LIST_ELEMENT sources as arbitrary, since the graph's edges change with the list's contents. - Bare list variable sources, e.g. @Shadowsources("values"), now fail fast with a message pointing to the new element syntax, and the mention of @PlanningListVariable as a valid source was removed from the @ShadowVariable javadoc, as they gave stale values that even FULL_ASSERT could not detect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ange The element's declarative shadow variable depends on a genuine basic variable and the element has no ordering shadow variables, proving the aggregate is retriggered through the element edge alone.
Christopher-Chianelli
left a comment
There was a problem hiding this comment.
I will do my own testing of this to verify correctness. Some comments.
- Remove requiresListVariableChangeEvents(); list variable events are forwarded unconditionally, like basic variable events. - Rename ListElementEdgeMaintainer to ListElementSourceLocator, and the related field and method accordingly. - Rename the elements parameter to elementList. - Remove comments that stated the obvious. - Rewrite the incremental tests with MoveTester. - Use forEachIncludingUnassigned/forEach in the test constraint provider. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks @Christopher-Chianelli, I have done the required changes. I have tested that on a simplified model (VRP with sequential vehicles), it works well. |
|
@fodzal As a side-effect of some other changes we made, there is now a conflict in this PR. Can you please resolve it? We'll take a look at this afterwards and provide a final review. |
Upstream TimefoldAI#2525 (refactor: remove variable listeners) reworked the shadow variable machinery this branch builds on: VariableListenerSupport was renamed to ShadowVariableSupport and its `notificationQueuesAreEmpty` flag to `dirty` (with inverted polarity, so `notificationQueuesAreEmpty = false` maps to `dirty = true`). Manual conflict resolution in ShadowVariableSupport: - beforeListVariableChanged: `notificationQueuesAreEmpty = false` becomes `dirty = true`, matching the renamed field. - afterListVariableChanged: fold the two adjacent `shadowVariableSession` guards (one from upstream marking the state dirty, one from this branch forwarding the change) into a single guard. Behaviour is unchanged.
Christopher-Chianelli
left a comment
There was a problem hiding this comment.
Code overall looks good, will do more testing before approval. I have written code similar to this about 1 year ago in my attempt to implement this feature, but it had score corruptions back then. But the feature I was implementing was more varied (i.e. also included Collection based @InverseShadowVariable), and it is possible we fixed a bug that was causing the original implementation to fail.
Christopher-Chianelli
left a comment
There was a problem hiding this comment.
LGTM when green.
|
@fodzal The final review is in. Once you resolve the comments, and once CI is green, we will merge the PR. (The native build is currently failing, but that is likely unrelated to this PR and I won't hold you to fixing that one.) |
|



What
Follow-up to #2469. An entity declaring a
@PlanningListVariablecan now use a@ShadowSourcespath such as"visits[].endTime"to depend on a declarative shadowvariable of the list variable's elements:
This is the capability discussed in the #2469 review: accessing the declarative shadow
variables of entities inside the
@PlanningListVariable, with the edges of the graphcreated and updated as the list variable changes. The fail-fast added by #2469 now points
users to this syntax.
How
RootVariableSourceaccepts a[]segment on a genuine list variable declared on theroot entity, producing the new
ParentVariableType.LIST_ELEMENT. The path must end on adeclarative shadow variable of the element (optionally reached via a fact).
element.shadowVariable -> entity.shadowVariable) are created from thelist variable's contents when the graph is built. They are registered as initial dynamic
edges, not fixed edges, so a dependency loop through them is treated as breakable by the
solver instead of failing fast.
before/afterListVariableChangedevents are forwarded to the declarative shadowvariable session, which incrementally removes/adds the fan-in edges for the changed
range and always marks the target variable as changed (covering removals and
reorderings). Models without list element sources skip this entirely.
once per move, after all its elements' shadow variables have settled.
GraphStructureclassifies models with list element sources as arbitrary, since thegraph's edges change during solving; the grouped-updater decision is static for the same
reason.
Testing
values[].shadowVarforms and the new fail-fasts(non-declarative element variable, list variable reached via another member) in
RootVariableSourceTest.ListElementShadowVariableTest:SolutionManager.updateShadowVariables(includingempty lists and mutations between calls) and two
FULL_ASSERTsolver runs, one from anuninitialized solution and one from an initialized solution (exercising the initial
edge seeding).
testdomain/shadow/list_elementdomain is used instead of extendingsimple_list, because adding a list element source there would reclassify it away fromSINGLE_DIRECTIONAL_PARENTand lose that structure's coverage.coretest suite passes.