chore: add indictment support to Bavet's Node Network - #2528
Christopher-Chianelli wants to merge 26 commits into
Conversation
There was a problem hiding this comment.
First read-through. An elegant solution, was expecting this to be more code; not happy with some bits, though. Let's try to adjust if we can.
Before we move further though, I believe we need to have some clarity on how stable this is. We have the turtle tests - let's run them on this. (They may need some ad-hoc adjusting to be able to run with indictments.) If at the same time we can gather move speed data from them, we can compare to non-indictment runs and have a decent idea as to how much slower this will be.
| } | ||
| } | ||
|
|
||
| public void setNodeIds(long[] scorerNodeIds) { |
There was a problem hiding this comment.
Not excited that we're effectively creating more and more shared mutable state. Let's try to keep these things final and supplied at construction time. These "half-initialized" lazy objects make reasoning about the functionality/interactions significantly more difficult.
There was a problem hiding this comment.
Node ids are set in reverse order, so we do not know them until after construction of the Scorer.
67be0fa to
b438f8b
Compare
b438f8b to
0b67bca
Compare
|
0b67bca to
028372b
Compare
028372b to
bf744d7
Compare
|
For benchmarking purposes, I added |
cb3e60e to
ee3f751
Compare
ee3f751 to
5b8ebf6
Compare
This allows support infomation to be automatically passed when a new tuple is created (ex: in map/expand).
…d, add notes about stale ifExists indictments
…oesn't update, make PrecomputeUni tests aware of indictments
… if exists, aggregate support for precompute, make BiPrecomputeTest Indictment aware
40c2425 to
7298f9c
Compare
triceo
left a comment
There was a problem hiding this comment.
First read.
Other than the questions inline, I think we should think about what happens with other score directors (easy, incremental) when indictments are enabled. Easy probably fails with justifications already, but incremental does have some support for justifications AFAIK, so we should consider adding support for indictments there as well.
| * are exclusive to Timefold Solver Enterprise Edition. | ||
| */ | ||
| @NullMarked | ||
| public interface Indictable { |
There was a problem hiding this comment.
I am not aware of any such model requirement.
I'm assuming that this class came out of the conversation with Geoff?
There was a problem hiding this comment.
It is not a requirement for a model to have seperate solver and rest types, but I know a quite a few cases where they do use seperate solver and rest types. This class was primary created so the type field (which I later realized was neccessary so a frontend can identify the object that was serialized without the user manually adding it to their object) can be customized if they do not want it to be the canonical name of the class.
| * @return a list of {@link IndictmentAnalysis} linking planning entities and problem facts to their impact | ||
| * in this {@link ScoreAnalysis} | ||
| */ | ||
| List<IndictmentAnalysis<Score_>> indictmentAnalyses(); |
There was a problem hiding this comment.
What is the value when indictments are disabled?
There was a problem hiding this comment.
Currently a empty list.
There was a problem hiding this comment.
As long as it's the same behavior with justifications, I'm good.
| if (!isFiltering) { | ||
| counter.countRight = rightSize(leftTuple, compositeKey); | ||
| initCounterLeft(counter); | ||
| if (leftTuple.getIndictmentSource() != IndictmentSource.DISABLED) { |
There was a problem hiding this comment.
IMO it should be documented somewhere what the behavior of indictments is supposed to be in each node. In ifExists, it is the least obvious.



Each tuple now have an
IndictmentSourcefield. This field is set toIndictmentSource.DISABLEDwhen indictments are disabled, which is used to exit early and avoid work only required for indictments.ConstraintMatchPolicyhave two new values:ENABLED_WITHOUT_JUSTIFICATIONS_AND_INDICTMENTSthis has the previous behaviour ofENABLED_WITHOUT_JUSTIFICATIONS; has constraint matches but no justifications nor indictmentsENABLED_WITHOUT_INDICTMENTSthis includes justifications but not indictmentsENABLEDwas changed to include both justifications and indictments instead of just justifications.ENABLED_WITHOUT_JUSTIFICATIONSwas changed to include indictments.The actual work each node does is pretty simple. Do note that the attach IndictmentSource does not change after an insert:
forEach: if indictments are enabled, set the source of the tuple to aRootIndictmentSource(perhapsLeafIndictmentSourcewould be a better name) on insert. Retract and update do no extra work;retractremoves the tuple (and hence the indictment source and potential constraint match), andupdatedoes not change the instance tracked.filter: not a node, and does not affect what object is indicted.map/expand/flatten: these creates new tuples from a source tuple, and simply copy the source's indictment source on insert. Update and retract do not need to do anything following the same reasoning asforEach`.join: oninsert, set the indictment source of the joined tuple to aJoinedIndictmentSourcethat have a reference to both the left and right tuples' indictment sources.groupBy: oninsert, set the indictment source of the group's tuple to an aggregation containing the indictment sources of the tuples inside the group. Onupdate, remove the tuple's indictment source from the old group aggregation and add it to the new group aggregation. Onretract, remove the tuple's indictment source from the aggregation.distinct: this isgroupByin a trenchcoat and behaves similarly.concat: behaves similarly tomap/expand/flatten, since the left side and right side act independently and don't affect each other.ifExists/ifNotExists: The most complex of the bunch. When indictments are enabled, it takes a drastically different path than normal. In particular, right tuple will propagate updates even if the corresponding left tuple is not update. This is to prevent constraint matches with stale indictments referring to removed/retracted entities/problem facts. Additional tuple iteration occurs for indexedifExistsnodes so the tuple's indictments are up to date when the left tuple changes. Moreover, sinceifExistsdoes not create a new tuple but still adds to the indictment, the same indictment source is used, but the indictment source's support map for the key corresponding to theifExistsnode's id is updated.precompute: Since it uses an independent node network, its node's ids do not correspond to the outer node network's node ids. As such, the indictment support map of the produced tuples must be aggregated along with the original indictment source.Each constraint is now aware of the ids of the nodes that affect the constraint. It uses it when creating constraint matches to iterate through the support map so it does not indict objects from other constraints with
ifExiststhat so happen to share the same tuple. EachIndictmentSourceis iterated in a tree like matter to collect a set of unique indicted objects which is converted to a list inside the actual ConstraintMatch.The vast majority of the changes were making all existing ConstraintStream tests aware of indictments.
assertMatchwas changed so it sets the indictment list to the justifications, which work for some but not all tests. For the other tests,withIndictedObjectsis used to set the expected indicted objects for each match.Note: I am aware of the stale "NOTE: By not propagating here, ..." comment in
IfExistsNodeand will remove it.Note: I am aware that the code under
if (testFiltering(...))inIfExistsNodeshould do a branch depending on if indictments are enabled and call the right method (was written before the change to keep the indictments up to date and the two methods were not too different from each other).Note: I am aware the
carnalityandstoreSizemethods added toTuplecan be removed; they are an artifact from when tuple cloning were used forifExists's indictment enabled path.