Accumulators with initial value may fire with unbound internal values.
Closed this issue · 32 comments
This behavior isn't defined so it may not be a bug, but I wanted to log this here for reference. Consider this rule, originally from issue #101:
(mk-session [(dsl/parse-rule [[?res <- (acc/all) :from [Temperature (= ?t temperature)]]
[:test (or (< ?t 10)
(< 10 ?t))]]
(println "hello world"))])The all accumulator can fire with no content, which would trigger the test to fire. But since ?t has no defined value, it will be nil when test fires, yield a NullPointerException.
We could argue that the accumulator shouldn't fire unless all variables in its :from clause are populated. This would solve this problem, but at the same time it may not be intuitive to other users that would expect an empty ?res to fire if nothing matches at all.
Getting an idea of how other rule engines handle this situation could inform our behavior here.
This is a tricky case and a good question. I'd like to experiment some, like you said, with how other engines deal with this case.
I think having ?t bound to nil makes some sense to me since the accumulator allows for firing from the :initial-value - by definition. So that means the rest of the rule logic would have to handle the some? checking nil case. However, this is obviously a subtle "gotcha" either way.
The funny part of this issue is I completely did not intend to make an example that exposed this strange case. I just pulled out an arbitrary accumulator to use for the sake of example. Serendipity.
I'd propose that we shouldn't fire an accumulator if its bindings can't be satisfied. This was my initial intention early on as described in this comment [1]. If you bind inside an accumulator, you probably want to join to or do something with the binding, so we should make sure the items are bound. If users don't want this behavior, users skip bindings inside the accumulator and just access fields in the result returned from the accumulator.
In short, I think the current behavior of propagating nil bindings that can't be satisfied is a bug, and the description way back at [1] is what we want.
I'd welcome feedback on this line of reasoning.
[1]
#22 (comment)
The behavior I proposed in the above comment -- to only propagate accumulators if their values can be bound -- does break the test-accumulator-with-init-and-binding-groups test, which was introduced by @mrrodriguez in #89. It also breaks two other tests that happened to rely on propagating initial values with unsatisfied bindings in an accumulator, so this would be a non-passive change. (Although possibly still worth doing.)
I believe I'm on board with not propagating when there are bindings used in the accumulate condition that are not satisfied. What about cases where the binding has a value and it is just a join condition for the accumulate condition?
[Temperature (= ?t temperature)]
[?colds <- (acc/all) :from [Cold (= ?t temperature)]]So ?t has a value here. Should we propagate the empty sequence of the accumulate node when there are no Cold facts inserted?
So ?t has a value here. Should we propagate the empty sequence of the accumulate node when there are no Cold facts inserted?
Yes, I think we should propagate an initial value in this case because everything has a binding. That's actually the behavior the user was looking for in issue #22, and has a well-defined and (I think) intuitive behavior.
That makes sense to me. I was just double checking.
When working on #190 I have done a reasonable amount of work around the semantics of propagating the :initial-value and retracting to it etc.
When I was doing that, I noticed that in the current implementation left-activate of both the AccumulateNode and AccumulateWithJoinFilterNode there is a check of
(has-keys? (:bindings token)
binding-keys)This check actually doesn't make sense to me because I don't think it is accomplishing anything right now. I do think that back when it was added in e14f93d that it did make sense. This was done for #22, which was around this same sort of scenario described in this issue.
Why this made sense before is that the accumulate node considered the binding keys from its "inner condition" to be part of its own binding keys. This was done regardless of whether these were even join binding keys or not. The accumulate had visibility into all the binding keys that it needs to join with incoming tokens, as well as any new bindings it is establishing.
Since then, both accumulate node's binding keys only include keys that are used as join bindings. I actually think this is how other nodes work as well now. So the accumulate nodes do not seem to have any way to "know" what new bindings they may be setting up in their "inner condition".
I'm not sure on what approach you have been wanting to take on this, but I wanted to offer a few insights because I think some of it leads to simplifications for me in #190.
Currently both accumulate nodes check this (I believe) superfluous check of all binding keys "being in place". I don't think a token could propagate to this level with binding keys that were missing from the join binding keys the accumulate node is looking for. In order for these join bindings to be missing, the token would somehow have to match facts that didn't bind to bindings that were introduced in previous conditions. The only tokens that can have this problem, should just be from other accumulate nodes that have this same problem we are facing here.
I tried removing these checks in both nodes and didn't see any tests fail across my suite of tests I have available, as well as Clara's own.
Since this check is checking against the incoming tokens, the :initial-value propagation logic is doing these checks per-token propagating. I do not actually think that whether or not to propagate the :initial-value needs to be something decided at a per-token level. Since this check seems to not be checking anything useful right now, I was wanting to move it out of the per-token loop.
To fix this issue, it seems to me that the accumulate nodes need to have visibility to the bindings being introduced by their "inner conditions". I do not think this is the same thing as the "binding keys" though. The accumulate node should be able to determine whether or not the :initial-value should be propagated to all incoming tokens, just by checking that it does not have any newly introduced binding keys by its inner conditions.
I'm just looking for feedback on your thoughts around this. Hopefully I expressed my thoughts/concerns well enough to be understood.
Clarification on this may help me a little in organizing the :initial-value propagation issues that have came up with my work on #190. I can work around all of this, but I think it may simpler to get this particular issue cleared up at least conceptually - around if the current binding key check per-token makes sense, and if we even need a per-token check at all in the future or not.
Yes, I think we can check this statically and remove the runtime check. If the tests I added for #22 still pass then I think changes in this space are okay. Also, you're right it isn't the binding keys that need to be checked here, it is the inner bindings in the accumulator. And since we know those when assembling the network, we should know when the parent satisfies them.
In short, yes, I think we can remove has-keys? as you suggest, and this is possible because statically. Back in #22 we didn't sort the nodes topologically or do much static analysis of the bindings, so this was done at runtime.
As @mrrodriguez mentioned, I've been giving some thought to how to approach this one.
My thinking here is that we are already generating the information to know which bindings in a condition need to come from right activation binding groups or are its result binding (i.e. are new bindings generated by that condition) and which are coming from its left activations (i.e. from ancestor nodes in the network). This information is currently being discarded. My plan is to change that so that the beta-graph [3] has a new key, something like
{code}
{:node-id-to-new-bindings #{Symbol}}
{code}
Note that this would also need to distinguish between the result bindings and bindings in the constraints. The accumulator nodes would take a new field indicating whether new bindings are created in the constraints; if so they would not propagate initial values if no elements are present.
After this, in combination with the changes in #188 and #189 that @mrrodriguez is working on, the behavior would be:
- If an accumulator has binding groups in its constraints that use a binding that is populated from its elements, it cannot propagate an initial value without elements. Such elements must meet all constraints, not just be of the correct fact type, in order to be considered.
- If an accumulator does not create any new bindings from its constraints, excepting the result bindings, then it should accumulate and fire once per distinct set of bindings.
@mrrodriguez , does the behavior I describe accord with what you are implementing for #189 ? It is a bit different from what you described there, but I believe that is just because you're not fixing this issue there. I'd like to be sure we're on the same page regarding what the final state should be though.
@rbrush , do you have any thoughts on this approach? Obviously, if someone else is working on this one please let me know; no need to duplicate work.
@WilliamParker I think what you are saying makes sense and is something that can layer over #188 and #189 issues. It sounds like it'd just be one additional check on whether or not the :initial-value is to be propagated.
The bigger concern I have is this would be a breaking change. I know we do have code that relies on the current behavior. I can also see why we have this code in cases. I understand why we are semantically trying to fix this issue. I also understand the "workaround" to get the same old behavior with these new proposed changes in place.
However, I would like to consider offering alternatives that are more obvious for the cases that we have right now where people do rely on this behavior.
A common usage of the :initial-value for us is concerning the idea of a condition that may or may not be satisfied, but the rule should fire anyways. This is, in a sense, an "optional" condition of a rule. Often the acc/all accumulator is used to achieve this goal.
e.g.
(defrule make-outcome-for-results
[Person (= ?id id)]
[(acc/all) :from [Result (= ?id person-id) (= ?result-id id)]]
=>
(insert! (->Outcome ?id ?result-id)))The result-id is unique to any any given Result, so it is not a good fact binding for an accumulator. However, in this sense it is syntactically saving time to just bind it like this. To do this correctly after this issue is addressed, you would have to do something like
(defrule make-outcome-for-results
[Person (= ?id id)]
[?results <- (acc/all) :from [Result (= ?id person-id)]]
=>
(insert! (->Outcome ?id (-> ?results first :result-id))))Or you could even go through the extra trouble of using an :or except you'd have to have the ?result-id binding in both branches of the :or for it to be usable in the RHS, which is not practical here.
You could use 2 rules, but that becomes very heavy when the number of optional conditions increases.
I don't think this workaround is particularly difficult, what I think is difficult is making it clear how this change will make this old pattern not work. What is worse, it would be fairly easy to overlook the fact that it doesn't work anymore. If tests were written in my example above always with some Result data present, the rule would always fire and produce outcomes. It may be an edge case that there are no Result data. When that happens, the rule wouldn't fire and may not have been expected by the rule writer.
What I am now thinking is that what may be missing is the ability to syntactically, and perhaps more efficiently, write a rule that has true support of an "optional" condition. This optional condition would have full support for fact bindings. For the sake of demonstration consider:
(defrule make-outcome-for-results
[Person (= ?id id)]
[:maybe [Result (= ?id person-id) (= ?result-id id)]]
=>
(insert! (->Outcome ?id ?result-id)))Perhaps a first-class construct like this could separate this case explicitly instead of trying to conflate rules like this with accumulators. Perhaps this is just implemented as some sort of syntactic sugar as well, but maybe we could do more with it - I haven't thought about it.
Also, a question that should come up is the same on that we have for accumulators prior to fixing this issue,
- what should
?result-idbe in the RHS when there were no facts matching for the Result condition?
I think nil would be an obvious choice. Of course you would have have a little ambiguity on whether nil was the actual value of a Result fact's id field or you had no Result facts matching at all. This ambiguity may be fine here and just something to be aware of.
We could even consider allowing a (optional) sentinel/default perhaps.
(defrule make-outcome-for-results
[Person (= ?id id)]
[:maybe [Result (= ?id person-id) (= ?result-id id)]
:default (= ?result-id ::none)]
=>
(insert! (->Outcome ?id ?result-id))) I'm not sure about this syntax or even the suggestion being solid. I'm just posing the idea here to get the conversation started at least.
To be honest this thread makes me a bit uncomfortable because we seem to be adding a number of caveats to the behavior to address the problem at hand. If it takes more than thirty seconds to explain why a rule will or won't fire, then I think we've missed the mark, giving users too much to reason about. Previously I had tried to keep things really simple:
- Rules fire once for each unique combination of bindings.
- In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound elsewhere in the rule.
The behavior proposed in #189 changes that, so accumulators with bindings only fire if they match some facts...but accumulators without bindings don't. I had +1'd that but am having second thoughts, especially if we have to keep piling on more semantics like the ones described here.
Rather than adjusting the semantics, might it be better to stick with a couple small axioms, and handle these cases in user code...or in generated rules or specialized accumulators that do what we want? I know user code would have to handle things like empty items when (acc/all) is run...but at least we could point to the above simple rules as how to reason about it.
Also, I'm not necessarily opposed to the :maybe proposal, but am a bit reluctant to add further expression types, since it increases the surface area and learning curve of the API. Could we handle this in specialized accumulators or some other generated construct above Clara's core API as well?
I'm becoming confused here. My thinking was that
- Rules fire once for each unique combination of bindings.
- In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound elsewhere in the rule.
is the behavior we want to restore. My problem with the current behavior is not with this goal, but rather that this goal is not actually being achieved. My impression from conversations with @mrrodriguez was that the behavior he describes in #189 is a "midway point" where the end goal is to get back to the behavior we want that @rbrush described above (and then add testing to ensure we have no regressions). Mike, please correct me if this is a misrepresentation of your views.
The only thing that I think was new in #189 was defining the behavior when an accumulator received facts that matched the bindings, but not the actual constraints on the rule. Namely, whether something like
(acc/all) :from [Temperature (= ?location location) (< temperature 20)]
when we have a Temperature that is not less than 20.
Regarding
The behavior proposed in #189 changes that, so accumulators with bindings only fire if they match some facts...but accumulators without bindings don't.
I don't understand the "accumulators without bindings don't" part. If the constraints are not fully satisfied, then the elements should not be given to the accumulator in any case. The ambiguity concerns whether those elements create a binding group despite their failure to match any additional constraints. This is a case that fundamentally doesn't exist for accumulators without binding groups and doesn't seem to me to have a counterpart there.
Regarding an earlier part of the thread, I definitely would not expect the behavior in #189 (comment) to change.
If I'm misunderstanding anyone's thoughts, please correct me/elaborate on those thoughts.
Regarding the :maybe proposal, I haven't thought about it as deeply as the generalized accumulator semantics, but could we accomplish this by providing the option for accumulators to allow nil to propagate and to fire without binding groups existing? I think this probably would need some work in Clara's core API, but perhaps it could be done in a less intrusive and probably more extensible manner as an option on accumulators than as a top-level boolean expression form. To me this would have the benefit of choosing sensible defaults for most cases and then allowing the user the option to deviate from those defaults, but in a way that does not impose a burden on a user that just uses the defaults. Say something like
{:reduce-fn some-function-here
:propagate-without-bindings-satisfied true
:propagate-nil true}
where a user who does not need them can just ignore those last two keys.
Regarding the use-case for :maybe, the main motivation in my mind is that we've had use cases where there are facts in the session that don't actually logically impact the semantics of our rules in any way, but the downstream consumers nevertheless want certain data to be passed along in a final outcome fact if it is present.
This stuff on :maybe is just some preliminary thoughts though. I agree that making too many options part of the core knowledge one needs to effectively read rules could be problematic.
The confusion may be on my side, so perhaps some examples would help. Here's what I would expect to happen:
1 This would match once for temperatures:
(defquery get-all-temps
[]
[?all-temps <- (acc/all) :from [Temperature]]) 2 This would match once per location that exists:
(defquery get-all-temps-by-location
[]
[?all-temps <- (acc/all) :from [Temperature (= ?location location]]) 3 This would match once per location, even if there are no temperatures in that location, since ?location is bound in the Windspeed rule. (This was the issue fixed way in #22).
(defquery get-all-temps-where-its-windy
[]
[Windspeed (= ?location location) (> windspeed 20)])
[?all-temps <- (acc/all) :from [Temperature (= ?location location)]]) 4 Similar to the above item (3), this would match once per location, even if no temperatures match, since the ?location variable is bound in the previous condition.
(defquery get-all-cold-temps-where-its-windy
[]
[Windspeed (= ?location location) (> windspeed 20)])
[?all-temps <- (acc/all) :from [Temperature (= ?location location) (< temperature 32)]]) I'm not sure if we're all on the same page for item 4. If there are no cold temps, should this accumulator fire? I've waffled on this, but the two axioms I stated above suggest that it should.
I'd agree on number 4 that you have above. I think the discussion was more about a case like
(defquery get-all-freezing-temps
[]
[?all-temps <- (acc/all) :from [Temperature (= ?location location) (< temperature 32)]]))
where there are Temperature facts, but the temperature is, say, 40. The ambiguity here (to me) was whether facts that did not actually satisfy the constraint of having a low temperature would still have an impact on rule execution by creating a binding group. I'd say not, since I'd suggest a general axiom of
- If a fact does not satisfy the constraints of a condition, it should not impact downstream logic that depends on that condition.
So in this case of having a Temperature fact with a temperature of 40, I don't believe that query should match.
The main discussion and confusion on #189 has been about case 4 above as well as Will's example after it
I'm not sure if we're all on the same page for item 4. If there are no cold temps, should this accumulator fire? I've waffled on this, but the two axioms I stated above suggest that it should.
The issue is not around there being no temperatures at all. The issue is around when there are some temperatures. And either none of them or some of them match the constraint of being less than 32.
If there are none matching that constraint, but some temperatures do exists for a location that matches a wind speed. It gets really confusing on whether you should propagate the initial value for every location binding group. It is like a "fake match". Temperatures are matching on location thus forming groups. However none of them are satisfying the otherconstraint of the rule.
Similarly if some binding groups do have matches, should the other binding groups with "fake matches" propagate the initial value anyways?
I'm leaning towards the most meaningful and straight forward behavior to just be, do not propagate the initial value when there are actually temperatures present but they are not satisfying the constraints of the rule. For any number of location binding groups that they may potentially be able to join with.
I'm open to these options as long as we can come up with a concise description of the behavior that does have a bunch of caveats. Here's another attempt that covers the behavior your'e looking for in case 5:
- Rules fire once for each unique combination of bindings.
- In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound elsewhere in the rule.
- Variables are bound only if the fact that matches them satisfies all other criteria.
For case 5, the rule won't fire because ?location can't be bound since no fact satisfies it. But this doesn't cover case 4, which I'll re-post here for convenience:
(defquery get-all-cold-temps-where-its-windy
[]
[Windspeed (= ?location location) (> windspeed 20)])
[?all-temps <- (acc/all) :from [Temperature (= ?location location) (< temperature 32)]])In this case, if there is a Windspeed that matches, but only temperatures above 32 degrees, I'd expect for the rule to match with an empty sequence for ?all-temps. The query is to get all cold temperatures where it is windy...so we have a windy location that has no cold temperatures, hence the empty sequence for ?all-temps.
I'm fine with changing this behavior if we have a simple way to describe the behavior to a new user. Is there an amendment or a different description than the one I proposed that captures what your'e looking for?
I can see where you are coming from in the thought process behind
(defquery get-all-cold-temps-where-its-windy
[]
[Windspeed (= ?location location) (> windspeed 20)])
[?all-temps <- (acc/all) :from [Temperature (= ?location location) (< temperature 32)]])I've fought with this one quite a bit. The situation that is most weird to me is like what you described above, but when you have multiple binding groups in play. I'll show a few test data points to demonstrate:
;; fact-1
(->WindSpeed 30 "MCI")
;; fact-2
(->WindSpeed 40 "LAX")
;; fact-3
(->Temperature 20 "MCI")
;; fact-4
(->Temperature 10 "LAX")In this situation, there are 2 binding groups, "MCI" and "LAX". The "MCI" group has a match for both conditions and therefore is straightforward.
The "LAX" group however, does not have a matching pair. It does have a Temperature, but not one that matches all constraints. I believe with your rules above, there is nothing necessarily long with this, so we'd get the empty sequence for ?all-temps.
Also, consider the case where fact-4 doesn't exist at all. Now the "LAX" binding group doesn't have any possible matches, The bindings are all in place though, so perhaps it fires with ?all-temps bound as well.
There may be an arbitrary number of binding groups without matches here, and under these rules, they all get an :initial-value propagated. The groups with actual matches fire as well with actual accumulated values.
This may all be fine and if we decide it is what makes sense, I can incorporate it into my changes for AccumulateWithJoinFilterNode. AccumulateNode would have to be re-worked a little as well to behave like this since it doesn't after #190
As I said previously in #189 , the rule I was following was:
- if no binding group has any matching facts, the
:initial-valueis sent to all binding groups - If some binding group has matching facts, the
:initial-valueis not sent to any binding group that doesn't have matches.
I'm fine with adjusting this if it is what makes the most sense to everyone. I think it may be slightly more complicated in the implementation, but that's besides the point. :)
I think the talk about binding groups may be obscuring the deeper issue here a bit; I'm going to try to take a step back from them and describe what I think an accumulator should do.
- The facts coming into an accumulator are each assigned to a bucket b in B. No fact is assigned to more than one b; some facts may be assigned to no bucket. The accumulator takes all facts assigned to a bucket, reduces over them, and informs downstream consumers (i.e. downstream nodes) of the result. The reducing function should be agnostic to the order in which it reduces over the elements, that is, it should be commutative and associative. If the accumulator has a non-nil initial value, then even buckets b without any assigned fact will produce a value
- For some accumulators, before applying operations on facts, we take B and transform it into a set of buckets B1 using some arbitrary (for now) function that takes B and all facts. The same conditions apply. It is possible that B1 will be an empty set, in which case no results will be produced. The same conditions for reducing and initial value as in
Case 1 corresponds to all cases where bindings used in the constraints are defined prior to the accumulator condition. This would be query examples 1, 3, and 4. Case 2 is the trickier case. We are taking the groupings (binding groups) from previous nodes and transforming them into a new set of groupings.
I think the fundamental dichotomy here is that the idea of creating a group and the idea of an initial value are in conflict. Why create a group for one set of bindings but not another that has no matching facts? I can't think of any rule that seems intuitive and useful that does so. It seems more intuitive for the accumulator to refrain from creating a new bucket unless facts will fall into that bucket. The case where bindings are created in the accumulator and nothing matches corresponds to the case above where B1 is an empty set.
@mrrodriguez , perhaps I'm misunderstanding
- if no binding group has any matching facts, the :initial-value is sent to all binding groups
- If some binding group has matching facts, the :initial-value is not sent to any binding group that doesn't have matches.
but I think the weakness here is that I don't see a firm definition of what a binding group is supposed to be or when one should be created.
Here are some axioms I'd suggest, taking @rbrush 's above as a starting point:
- Rules fire once for each unique combination of bindings.
- In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound in a previous condition.
- Variables are bound only if the fact that matches them satisfies all other criteria.
- For two bindings groups of facts g1 and g2, any change to the set of facts in g1 that does not impact the of facts in g2 or any ancestor conditions of the condition to which g2 belongs should not impact g2. Less formally, binding groups should be independent of each other in a single condition, although this does not exclude the possibility of one binding group impacting another via previous conditions or another rule.
- Inserting a fact and retracting it should, in rules sessions governed by truth maintenance, result in a session with the same logical state as never inserting it at all. A corollary here is that inserting a fact n times and retracting it n times, where at no time the number of times it has been retracted is greater than the number of times it has been inserted, should result in a session with the same state as not performing any of the operations.
I changed
- In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound elsewhere in the rule.
to
- In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound in a previous condition.
The case that inspired this is something like
1.
(defquery get-all-cold-temps-where-its-windy-reversed
[]
[?all-temps <- (acc/all) :from [Temperature (= ?location location) (< temperature 32)]]
[Windspeed (= ?location location) (> windspeed 20)])
This is Ryan's case 4, but with reversed conditions. The question here to me is whether we want ?location to be bound in the accumulator condition by the later WindSpeed condition. My first thought is that it is better for conditions to be agnostic to what is downstream and that simply respecting the order provided by the user is the simplest behavior, but I could be persuaded otherwise.
Regarding @WilliamParker's comment:
I changed
In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound elsewhere in the rule.
to
In this spirit, accumulators that use bindings will fire with no matching results if those bindings are bound in a previous condition.
I don't think this adjustment is necessary. Case 4 should work identically even if conditions are reversed, as shown in Will's example. Today we do a topological sort of bindings, preferring non-accumulator bindings to accumulator bindings, to make this work. Subjectively this seems simpler to me since I don't have to worry about the order in which I write my conjunctions in a rule, and rules won't break if a change that order.
Regarding @mrrodriguez's comment:
I'm in favor of making the adjustment we describe here as it applies to #189 if it doesn't cause too much trouble. (And I know I'm flipping my previous +1.) The example behavior you describe is what I'd expect given that rule and can be relatively easily explained to a user without getting into the details of the nodes and initial values. But I'm happy to discuss further if this will cause a lot of headaches elsewhere.
Using the existing topological sort to create the bindings without relying on the ordering seems reasonable. The part that still seems ambiguous to me with that behavior is what happens when you have something like
(defquery two-temp-conditions
[]
[?t1 <- (acc/all) :from [Temperature (< temperature 20) (= ?location location)]
[?t2 <- (acc/all) :from [Temperature (even? temperature) (= ?location location)])
Which accumulator should be required to create a ?location binding? This sort of case is something I'd like to mull on some tonight.
Will's example above is interesting. Typo'ed some it looks like
However, I do think we need to consider the general idea. This looks like a potential problem.
(defquery cold-or-temps
[]
[?t1 <- (acc/all) :from [Cold (= ?location location)]]
[?t2 <- (acc/all) :from [Temperature (= ?location location)]])I don't think the extra constraints in the rule are even necessary to show the issue.
The way this fires or doesn't fire depends if the Cold or Temperature condition comes first, which should end up being an impl detail of how the Clara compiler sorts the conditions.
However, if there is a Cold, but no Temperature, whether or not this will be able to fire will depend on the order they were sorted in.
We could try to do something to make them independent here. I think it does start to get quite a bit more complex in the Clara engine side, not the semantic side.
@mrrodriguez had an interesting idea here. For cases where one or more accumulators are stacked and it is not clear which should create a binding group, we could extract them into separate rules with each possible ordering. So, for example,
[?t <- (acc/all) : from [Temperature (= ?location location)]]
[?w <- (acc/all) :from [WindSpeed (= ?location location)]]
would propagate a result downstream for any ?location with a corresponding WindSpeed or Temperature.
This conflicts with a created AccumulateNode requiring that facts exist under a binding
group to propagate a result, since either way it is ordered a child would depend on a parent. So, for example, if the WindSpeed AccumulateNode was a child of the TemperatureNode, then if we have only one path the WindSpeed node might need to do something to its parent to cause it to propagate its initial value. This is extremely different from the current engine architecture.
Mike's thought was to extract this into different rules, with each possible ordering. The results of these rules would then be fed into some form of "just-one" accumulator to maintain the expected cardinality from the original rule. If both nodes have results, then assuming that the accumulators obey assumptions regarding order independence (i.e. commutativity and associativity) then I believe the results of these extracted rules should be the same. I believe this would allow us to support this case without needing to fundamentally change the engine. Essentially, it would be a similar idea to the current extraction of complex negations.
I'm planning to start kicking around what fixing this issue (the original one, not this stacked accumulator issue) in the compiler would look like. That part shouldn't be particularly susceptible to merge conflicts with other accumulator work. I do want to ensure we have a viable path forward for the stacked accumulator problem and avoid going in a direction that would prevent from us from fixing it though.
I have a first pass at compiler changes to give AccumulateNode and AccumulateWithJoinFilterNode visibility to what, if any, bindings they create at WilliamParker@243e405 Note that this is a bit different from my (incorrect) initial thoughts; sort-conditions wasn't involved.
Note that this does not actually fix the functional problem yet; it just provides information to the nodes that will be helpful in doing so. It mostly doesn't change anything functionally, with the exception that I think there are probably some very edge-case scenarios where a node could be shared by multiple parents, some of which might pass in all bindings it needs and others of which might not, where it cannot be shared with these changes, since the :new-bindings will be part of the equality check at https://github.com/rbrush/clara-rules/blob/0.11.1/src/main/clojure/clara/rules/compiler.clj#L973
I don't see any problems for correctness, since condition-to-node is called in add-conjunctions [1], which is itself called in add-productions. We'd have problems if the parent-bindings in condition-to-node didn't come from the true parent, but rather a parent from another rule, but I don't immediately see a way for this to happen.
I'll let this sit for a bit and look at it later to see if I see any problems, but I wanted to give some visibility to the path I'm looking at. If I think it is all correct I might go ahead and submit a pull request even without the functional fix to either node. My reason for doing so would be that the actual fixes to either node might be done by multiple people (I know @mrrodriguez is looking at AccumulateWithJoinFilterNode) and dealing with that coordination might be easier if the change to provide the information to the node were in master. @rbrush, your preference on how you want to do the coordination/project management of course.
#208 fixes this issue for AccumulateNode.
#210 fixes this issue for AccumulateWithJoinFilterNode.
I think the only functional problem remaining here is described at [1].
Otherwise, I think the issues described here, in #188, and in #189 have been addressed. My thinking is to log a new issue for the remaining edge case described in [1] and a few following comments and to close the current 3 issues. Does anyone else see anything left to address in these 3 issues?
Sounds like a plan, Will. +1 to tracking that specific edge case separately; the other changes here seem important enough to merit a release by themselves.
Alright, I logged #218 to track the open edge case. Anything left to do on this one, or should we close it?
@rbrush , I don't see anything left to do here. @mrrodriguez , do you agree?
+1 to closing
Closing and tagging for the 0.12 release.